Closed
Bug 1266611
Opened 8 years ago
Closed 8 years ago
Tab prompts can overlap in the same tab
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
VERIFIED
FIXED
Firefox 49
Tracking | Status | |
---|---|---|
firefox49 | --- | verified |
People
(Reporter: timdream, Assigned: timdream)
References
Details
Attachments
(1 file)
STR: Run test case in bug 1266353. Actual: The new tab prompt show up on top of the existing alert. The semi-transparent mask gets darker and darker. Note: Splitting this cosmetic issue from bug 1266353 to make things more understandable. However, this bug can be WONFIXED if it turned out to be simply a side-effect of bug 1266353 and we should be fixing the event loop there.
Assignee | ||
Comment 1•8 years ago
|
||
Gijs, given the fact bug 1266353 will not be fixed by Platform any time soon, should we be fixing tab prompts here by creating a queue so that prompts come later can never overlap with the previous ones? That would bring our behavior closer to Chrome, per my finding in bug 1266353 comment 0.
Flags: needinfo?(gijskruitbosch+bugs)
Comment 2•8 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #1) > Gijs, given the fact bug 1266353 will not be fixed by Platform any time > soon, should we be fixing tab prompts here by creating a queue so that > prompts come later can never overlap with the previous ones? That would > bring our behavior closer to Chrome, per my finding in bug 1266353 comment 0. I don't know how difficult it will be to get this right, but yes, that sounds like the correct thing to do. Whether it is worth spending time on right now, I don't know...
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/54180/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/54180/
Attachment #8754692 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 4•8 years ago
|
||
https://reviewboard.mozilla.org/r/54180/#review50882
Assignee | ||
Comment 5•8 years ago
|
||
https://reviewboard.mozilla.org/r/54180/#review50882 This is a quick patch. It simply reverse the order of XUL prompt elements (the siblings of <browser>) and set hidden = true to the ones that behind the current showing prompt.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Comment 6•8 years ago
|
||
Comment on attachment 8754692 [details] MozReview Request: Bug 1266611 - Multiple tab prompts should not overlap, r=gijs https://reviewboard.mozilla.org/r/54180/#review50916 This looks OK in and of itself... can you also write a test that this behaves properly? :-)
Attachment #8754692 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8754692 [details] MozReview Request: Bug 1266611 - Multiple tab prompts should not overlap, r=gijs Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54180/diff/1-2/
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8754692 [details] MozReview Request: Bug 1266611 - Multiple tab prompts should not overlap, r=gijs I moved one test out of test/general/ and wrote the new test in the newly created test/tabPrompts/ dir.
Attachment #8754692 -
Flags: review+ → review?(gijskruitbosch+bugs)
Comment 9•8 years ago
|
||
Comment on attachment 8754692 [details] MozReview Request: Bug 1266611 - Multiple tab prompts should not overlap, r=gijs https://reviewboard.mozilla.org/r/54180/#review50970 Yay for splitting up tests more! Thanks a lot. :-) ::: browser/base/content/test/tabPrompts/browser_multiplePrompts.js:23 (Diff revision 2) > + } > + alert("Alert countdown #" + i); > + }); > + window.postMessage("ping", "*"); > + }; > + let url = "data:text/html,<script>(" + encodeURIComponent(contentScript.toSource()) + ")();</script>" FWIW, from the try push it looks like this races with the scripts all having run and 5 prompts having been created. You could probably fix it by waiting for the prompts to be created using whatever observer notification we fire when they do get created. ::: browser/base/content/test/tabPrompts/browser_multiplePrompts.js:27 (Diff revision 2) > + }; > + let url = "data:text/html,<script>(" + encodeURIComponent(contentScript.toSource()) + ")();</script>" > + > + let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, url, true); > + > + let promptsCount = 5; The magic number '5' comes back a few times - define as `const PROMPTCOUNT` at the top? ::: browser/base/content/test/tabPrompts/browser_multiplePrompts.js:33 (Diff revision 2) > + while (promptsCount--) { > + let prompts = tab.linkedBrowser.parentNode.querySelectorAll("tabmodalprompt"); > + is(prompts.length, promptsCount + 1, "There should be " + (promptsCount + 1) + " prompt(s)."); > + // The oldest should be the first. > + let i = 0; > + for (var prompt of prompts) { Nit: let ::: browser/base/content/test/tabPrompts/head.js:3 (Diff revision 2) > +XPCOMUtils.defineLazyModuleGetter(this, "Promise", > + "resource://gre/modules/Promise.jsm"); > + > +function pushPrefs(...aPrefs) { > + let deferred = Promise.defer(); > + SpecialPowers.pushPrefEnv({"set": aPrefs}, deferred.resolve); > + return deferred.promise; > +} I expect you basically just copied this, but can I request that you instead replace the callsite (I presume in the other test?) With just: yield SpecialPowers.pushPrefEnv({set: <arg>}); ? Matt recently landed a change so that pushPrefEnv now returns a promise, so this code is no longer necessary, so then we can omit head.js in this directory altogether.
Attachment #8754692 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 10•8 years ago
|
||
https://reviewboard.mozilla.org/r/54180/#review50970 > The magic number '5' comes back a few times - define as `const PROMPTCOUNT` at the top? There are not duplications -- The `contentScript` has no access to the scope of the test script.
Assignee | ||
Comment 11•8 years ago
|
||
Comment on attachment 8754692 [details] MozReview Request: Bug 1266611 - Multiple tab prompts should not overlap, r=gijs Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54180/diff/2-3/
Assignee | ||
Comment 12•8 years ago
|
||
Another try based on the current central: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2287f716835c Not entire sure about the results of previous try runs...
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 13•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/2ffaaeccf3d9
Keywords: checkin-needed
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2ffaaeccf3d9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Updated•8 years ago
|
Flags: qe-verify+
Comment 15•8 years ago
|
||
Confirming that this issue is now fixed using Fx 49 beta 10, buid ID 20160902105049 on: * Windows 10x64 * Ubuntu 12.04 x86 * Mac OS X 10.11.6. Verified using the testcase from bug 1266353.
You need to log in
before you can comment on or make changes to this bug.
Description
•