Closed
Bug 1239885
Opened 8 years ago
Closed 8 years ago
Popup blocker notifications don't work on "data:" pages when using window.open() with no URI as arguments
Categories
(Toolkit :: UI Widgets, defect)
Toolkit
UI Widgets
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: arni2033, Assigned: Gijs)
References
()
Details
(Keywords: regression)
Attachments
(1 file)
>>> My Info: Win7_64, Nightly 46, 32bit, ID 20160112030227 STR: 1. Open this "data:" url or click URL in the form above > data:text/html,<script>addEventListener('click',function(){console.log('click');window.open()})</script> 2.A) Middle-click on the page content (white area) 2.B) Right-click on the page content Result: Nothing Expectations: "Blocked popup" notification should appear This never worked in e10s mode; It was broken in non-e10s between 2014-03-21 and 2014-03-22 by bug 933462. Pushlog: > https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c148f0b0c8b4&tochange=72c2f9ebf4bc
Comment 1•8 years ago
|
||
User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0 Build ID 20160126030244 Reproduced on Nightly 47.
Component: Untriaged → Event Handling
Updated•8 years ago
|
Component: Event Handling → General
Product: Core → Firefox
Comment 2•8 years ago
|
||
My guess is that some code in FF code is using node.ownerDocument.documentURI or some such, and not node.nodePrincipal.
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #2) > My guess is that some code in FF code is using > node.ownerDocument.documentURI or some such, and not > node.nodePrincipal. toolkit's browser-content.js is listening for the onPopupBlocked event and throws: TypeError: ev.popupWindowURI is null when trying to access the URI for the popup window. If I change the URI from comment #0 to: data:text/html,<script>addEventListener('click',function(){console.log('click');window.open("about:blank")})</script> then it works fine.
Component: General → XUL Widgets
Product: Firefox → Toolkit
Summary: Popup blocker notifications don't work on "data:" pages → Popup blocker notifications don't work on "data:" pages when using window.open() with no URI as arguments
Comment 4•8 years ago
|
||
I think the event API is a bit silly here, but at least it should have access to the Window object (event.requestingWindow) on which .open() is called and it can get the right requesting URL from that. And then what popupWindowURI is is less interesting. empty string or null could default to about:blank I guess.
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #4) > I think the event API is a bit silly here, but at least it should have > access to the Window object (event.requestingWindow) on which .open() is > called and it can get the right requesting URL from that. It looks like it already does that. > And then what popupWindowURI is is less interesting. Well, we need it to open the popup if the user elects to do that, and for that we need the URI. > empty string or null could default to about:blank I guess. Are you saying browser-content.js should be taught to generate about:blank as a URI for a null popupWindowURI, or that we should change nsGlobalWindow to use about:blank in this case, so the popupWindowURI is never null? (ni for this so I know where we expect to fix this :-) ) To be clear, in pretty much any case where a page opens window.open() with no URI, it'll want to use the return value of the window.open() call to actually manipulate that window (usually synchronously), either by inserting content into about:blank or by navigating it somewhere else. I'm assuming that that return value is null in cases where we block popups (that's certainly the case in this testcase) and so that'll break page script no matter what we do.
Flags: needinfo?(bugs)
Comment 6•8 years ago
|
||
I wouldn't change nsGlobalWindow since it just passes whatever url was given to window.open. But what I meant that toolkit could handle null similarly to about:blank. both those are most probably used so that page script will then populate the content.
Flags: needinfo?(bugs)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40495/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40495/
Attachment #8731322 -
Flags: review?(felipc)
Comment 8•8 years ago
|
||
Comment on attachment 8731322 [details] MozReview Request: Bug 1239885 - fix popup blocking brokenness when no URI is passed to window.open, r?felipe https://reviewboard.mozilla.org/r/40495/#review37189
Attachment #8731322 -
Flags: review?(felipc) → review+
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1c5926c2e71f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 11•8 years ago
|
||
Looks like this should work in 46 (with e10s disabled). The non-e10s regression was only in nightly 48.
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #11) > Looks like this should work in 46 (with e10s disabled). The non-e10s > regression was only in nightly 48. No, this was broken in non-e10s by bug 933462, according to comment #0 . However, given the fact that this is an edgecase with little practical value, I don't think this needs uplift.
status-firefox47:
--- → wontfix
You need to log in
before you can comment on or make changes to this bug.
Description
•