1266611 - Tab prompts can overlap in the same tab

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 2

[:Gijs (out for now; he/him)]

(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...

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: MozReview Request: Bug 1266611 - Multiple tab prompts should not overlap, r=gijs

Review commit: https://reviewboard.mozilla.org/r/54180/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54180/

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

https://reviewboard.mozilla.org/r/54180/#review50882

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 6

[:Gijs (out for now; he/him)]

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? :-)

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 9

[:Gijs (out for now; he/him)]

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.

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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/

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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...

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2ffaaeccf3d9

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2ffaaeccf3d9

Comment 15

[Cornel Ionce [:noni] [Hubs QA]]

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.