1347503 - Geolocation and Notification prompts do not swap to a new window

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P1)

Description

[Johann Hofmann [:johannh]]

This was discovered in bug 1347170

STR:

- Open permission.site
- Click Notifications and/or Location
- Right click on the tab and selected "Move to New Window"

The permission popup should correctly transfer to the new window.

Florian mentioned on IRC that the fix is likely just handling the swapping event in the way that webrtcUI does:

http://searchfox.org/mozilla-central/source/browser/modules/webrtcUI.jsm#460

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1347503 - Handle "swapping" event for permission notifications.

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

Comment 2

[Johann Hofmann [:johannh]]

Comment on attachment 8848806 [details]
Bug 1347503 - Handle "swapping" event for permission notifications.

https://reviewboard.mozilla.org/r/121694/#review123894

r=me with the comments below addressed

::: browser/modules/PermissionUI.jsm:355
(Diff revision 1)
>      // Permission prompts are always persistent; the close button is controlled by a pref.
>      options.persistent = true;
>      options.hideClose = !Services.prefs.getBoolPref("privacy.permissionPrompts.showCloseButton");
> +    options.eventCallback = (topic) => {
> +      if (topic == "swapping")
> +        return true;

Add a comment what this does, please.

Also, this could technically be reduced to

topic => topic == "swapping"

but that might have unintended side effects if anything checks != null for the return type. What do you think?

::: browser/modules/test/browser/browser_PermissionUI.js:408
(Diff revision 1)
> + * is transferred to the new window.
> + */
> +add_task(function* test_window_swap() {
> +  yield BrowserTestUtils.withNewTab({
> +    gBrowser,
> +    url: "http://example.com",

Nit: You can just use:

BTU.withNewTab("http://example.com", function(){...

::: browser/modules/test/browser/browser_PermissionUI.js:437
(Diff revision 1)
> +
> +    let newWindowOpened = BrowserTestUtils.waitForNewWindow();
> +    gBrowser.replaceTabWithWindow(gBrowser.selectedTab);
> +    let newWindow = yield newWindowOpened;
> +    yield BrowserTestUtils.waitForEvent(newWindow.PopupNotifications.panel, "popupshown");
> +    yield BrowserTestUtils.closeWindow(newWindow);

Can you please check some content of the popup to assert that we've shown the correct notification?

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Johann Hofmann [:johannh] from comment #2)
> Comment on attachment 8848806 [details]
> Bug 1347503 - Handle "swapping" event for permission notifications.
> 
> https://reviewboard.mozilla.org/r/121694/#review123894
> 
> Also, this could technically be reduced to
> 
> topic => topic == "swapping"
> 
> but that might have unintended side effects if anything checks != null for
> the return type. What do you think?

Nice. https://dxr.mozilla.org/mozilla-central/rev/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/toolkit/modules/PopupNotifications.jsm#357 suggests that we should be fine with reducing it.

> ::: browser/modules/test/browser/browser_PermissionUI.js:408
> (Diff revision 1)
> > + * is transferred to the new window.
> > + */
> > +add_task(function* test_window_swap() {
> > +  yield BrowserTestUtils.withNewTab({
> > +    gBrowser,
> > +    url: "http://example.com",
> 
> Nit: You can just use:
> 
> BTU.withNewTab("http://example.com", function(){...

Hmm, I'd rather stay consistent with the rest of this file.

> ::: browser/modules/test/browser/browser_PermissionUI.js:437
> (Diff revision 1)
> > +
> > +    let newWindowOpened = BrowserTestUtils.waitForNewWindow();
> > +    gBrowser.replaceTabWithWindow(gBrowser.selectedTab);
> > +    let newWindow = yield newWindowOpened;
> > +    yield BrowserTestUtils.waitForEvent(newWindow.PopupNotifications.panel, "popupshown");
> > +    yield BrowserTestUtils.closeWindow(newWindow);
> 
> Can you please check some content of the popup to assert that we've shown
> the correct notification?

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8848806 [details]
Bug 1347503 - Handle "swapping" event for permission notifications.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121694/diff/1-2/

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8848806 [details]
Bug 1347503 - Handle "swapping" event for permission notifications.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121694/diff/2-3/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4e2d53d22566
Handle "swapping" event for permission notifications. r=johannh

Comment 7

[Carsten Book [:Tomcat]]

backed this out since this might have caused failures like https://treeherder.mozilla.org/logviewer.html#?job_id=85832935&repo=autoland

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5e304691d84b
Backed out changeset 4e2d53d22566 for crashes in windows a11y tests

Comment 9

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Carsten Book [:Tomcat] from comment #7)
> backed this out since this might have caused failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=85832935&repo=autoland

That's really weird, I can't think why this would cause a11y failures. It doesn't change any existing behavior, it simply makes sure that the popup is correctly migrated when its tab changes windows. :(

I'll do another try push and investigate.

Comment 10

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to :Nihanth Subramanya [:nhnt11] from comment #9)
> (In reply to Carsten Book [:Tomcat] from comment #7)
> > backed this out since this might have caused failures like
> > https://treeherder.mozilla.org/logviewer.html#?job_id=85832935&repo=autoland
> 
> That's really weird, I can't think why this would cause a11y failures. It
> doesn't change any existing behavior, it simply makes sure that the popup is
> correctly migrated when its tab changes windows. :(
> 
> I'll do another try push and investigate.

BTW, M-a11y was green on the try push that I did before landing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9528844eafe3&selectedJob=85484243

Can you confirm that this patch could still be the cause of the failure you linked?

In the meantime, I've triggered a bunch of retries.

Comment 11

[(inactive) Nihanth Subramanya [:nhnt11]]

M-a11y is green on a fresh try push as well: https://treeherder.mozilla.org/#/jobs?repo=try&revision=02fda760296a18dd23c525ac3546d08a0627e23b&selectedJob=86086425

Comment 12

[Carsten Book [:Tomcat]]

hi nihanth, seems this also could be caused by the patch that landed before you and where this tests didn't run. sorry!

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Want to try landing this again? 
Once it's on m-c, Nihanth do you want to request uplift at least to aurora?

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6aff41084a96
Handle "swapping" event for permission notifications. r=johannh

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6aff41084a96

Comment 16

[Hani Yacoub, Desktop QA]

Build ID: 20170326030204
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

Verified as fixed on Firefox Nightly 55.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.

Comment 17

[Julien Cristau [:jcristau]]

See comment 13.

Comment 18

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8848806 [details]
Bug 1347503 - Handle "swapping" event for permission notifications.

Approval Request Comment
[Feature/Bug causing the regression]: Permission prompts!
[User impact if declined]: Minor, this patch saves a click (and adds a bit of polish)
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: Verified in comment 16
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: Simply handles an event (by returning true). Covered by automated tests.
[String changes made/needed]: none

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Looks like this needs a Beta approval request as well?

Comment 20

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8848806 [details]
Bug 1347503 - Handle "swapping" event for permission notifications.

Hmm, might as well.

See comment 18 for approval request comment.

Comment 21

[Gerry Chang [:gchang]]

Comment on attachment 8848806 [details]
Bug 1347503 - Handle "swapping" event for permission notifications.

Fix a regression related to permission prompt and was verified. Aurora54+ & Beta53+.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f83069cf61ed

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/68f40432b9a3

Comment 24

[Hani Yacoub, Desktop QA]

Build ID: 20170329004027
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0

Verified as fixed on Firefox Aurora 54.0a2 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.

Comment 25

[Andrei Vaida [:avaida]]

Let's make sure this works as intended on Beta 53 as well. Instructions in Comment 0.

Comment 26

[Hani Yacoub, Desktop QA]

Build ID: 20170330120906
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0

Verified as fixed on Firefox Beta 53.0b8 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.