Closed Bug 1266611 Opened 5 years ago Closed 5 years ago

Tab prompts can overlap in the same tab

Categories

(Firefox :: General, defect)

defect
Not set
normal

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.
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)
(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)
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: nobody → timdream
Status: NEW → ASSIGNED
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+
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/
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 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+
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.
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/
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...
https://hg.mozilla.org/mozilla-central/rev/2ffaaeccf3d9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.