Geolocation and Notification prompts do not swap to a new window

VERIFIED FIXED in Firefox 53

Status

()

Firefox
Site Identity and Permission Panels
P1
normal
VERIFIED FIXED
3 months ago
3 months ago

People

(Reporter: johannh, Assigned: nhnt11)

Tracking

({regression})

Trunk
Firefox 55
regression
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox52 unaffected, firefox53+ verified, firefox54+ verified, firefox55 verified, firefox-esr52 unaffected)

Details

(Whiteboard: [fxprivacy])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

3 months ago
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 hidden (mozreview-request)
(Reporter)

Updated

3 months ago
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
(Reporter)

Comment 2

3 months ago
mozreview-review
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?
Attachment #8848806 - Flags: review?(jhofmann) → review+

Updated

3 months ago
Iteration: --- → 55.2 - Apr 3
(Assignee)

Comment 3

3 months ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 6

3 months ago
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4e2d53d22566
Handle "swapping" event for permission notifications. r=johannh
backed this out since this might have caused failures like https://treeherder.mozilla.org/logviewer.html#?job_id=85832935&repo=autoland
Flags: needinfo?(nhnt11)

Comment 8

3 months ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5e304691d84b
Backed out changeset 4e2d53d22566 for crashes in windows a11y tests
(Assignee)

Comment 9

3 months ago
(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.
(Assignee)

Comment 10

3 months ago
(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.
Flags: needinfo?(nhnt11) → needinfo?(cbook)
(Assignee)

Comment 11

3 months ago
M-a11y is green on a fresh try push as well: https://treeherder.mozilla.org/#/jobs?repo=try&revision=02fda760296a18dd23c525ac3546d08a0627e23b&selectedJob=86086425
hi nihanth, seems this also could be caused by the patch that landed before you and where this tests didn't run. sorry!
Flags: needinfo?(cbook)
Want to try landing this again? 
Once it's on m-c, Nihanth do you want to request uplift at least to aurora?
status-firefox53: affected → fix-optional
tracking-firefox53: ? → +
tracking-firefox54: --- → +
Flags: needinfo?(cbook)
Keywords: regression

Comment 14

3 months ago
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6aff41084a96
Handle "swapping" event for permission notifications. r=johannh

Comment 15

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6aff41084a96
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

3 months ago
Flags: needinfo?(cbook)

Comment 16

3 months ago
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.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
See comment 13.
Flags: needinfo?(nhnt11)
(Assignee)

Comment 18

3 months ago
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
Flags: needinfo?(nhnt11)
Attachment #8848806 - Flags: approval-mozilla-aurora?
Looks like this needs a Beta approval request as well?
status-firefox52: --- → unaffected
status-firefox-esr52: --- → unaffected
Flags: needinfo?(nhnt11)
(Assignee)

Comment 20

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

Hmm, might as well.

See comment 18 for approval request comment.
Flags: needinfo?(nhnt11)
Attachment #8848806 - Flags: approval-mozilla-beta?
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+.
Attachment #8848806 - Flags: approval-mozilla-beta?
Attachment #8848806 - Flags: approval-mozilla-beta+
Attachment #8848806 - Flags: approval-mozilla-aurora?
Attachment #8848806 - Flags: approval-mozilla-aurora+

Comment 22

3 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/f83069cf61ed
status-firefox54: affected → fixed

Comment 23

3 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/68f40432b9a3
status-firefox53: fix-optional → fixed

Comment 24

3 months ago
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.
status-firefox54: fixed → verified
Let's make sure this works as intended on Beta 53 as well. Instructions in Comment 0.
Flags: qe-verify+

Comment 26

3 months ago
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.
status-firefox53: fixed → verified
You need to log in before you can comment on or make changes to this bug.