1477294 - The permissions doorhanger is no longer dismissed when the page is refreshed right after clearing the media autoplay permission

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P2)

Description

[Ciprian Muresan [:cmuresan], Ecosystem QA]

Attachment: block autoplay doorhanger does not dismiss itself.gif

[Affected versions]:
- Nightly v63.0a1, Build ID 20180720101508

[Affected Platforms]:
- All Windows
- All Mac
- All Linux

[Steps to reproduce]:
1. Open the browser with a clean new profile.
2. Navigate to www.youtube.com and click a video thumbnail.
3. Click the "Allow Autoplay" button from the doorhanger.
4. Click the circled "i" button from the left part of the address bar.
5. Clear the "Play media with sound" permission.
6. Press F5 and observe the behavior.

[Expected results]:
- The doorhanger is dismissed while the page is refreshed. The media autoplay doorhanger appears again. 

[Actual results]:
- The page is refreshed, but the doorhanged is not dismissed and an additional media autoplay doorhanger is displayed over the previous one. (in some cases the first doorhanger will also appear cutoff)

[Regression window]:
 5:13.25 INFO: Last good revision: 0bce1e5b252a300d14ee22b408f76574c01553cd
 5:13.25 INFO: First bad revision: 8dcdd881fd09a220150f2931fda326b01996627e
 5:13.26 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0bce1e5b252a300d14ee22b408f76574c01553cd&tochange=8dcdd881fd09a220150f2931fda326b01996627e

[Notes]:
- The issue is also reproducible if you block the autoplay permission and then clear it.
- Attached a screen recording of the issue.

Comment 1

[Paul Adenot (:padenot)]

Dale, Chris, this one is probably of interest to one of you.

Comment 2

[Dale Harvey (:daleharvey)]

So I can reproduce this with any permission (https://harmonious-wombat.glitch.me/ does a geolocation on load), I dont believe any of the autoplay code will have affected this. Johannh do you know whats up here?

Comment 3

[Johann Hofmann [:johannh]]

Yeah I think that's just a panel issue, nothing too dramatic though...

Do you still want to have this block bug 1376321?

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Johann Hofmann [:johannh] - PTO July 24th - 31st from comment #3)
> Yeah I think that's just a panel issue, nothing too dramatic though...
> 
> Do you still want to have this block bug 1376321?

I think we should try to fix this before shipping block autoplay. Lots of sites use media playback, so the permissions panel will be getting used a lot.

Comment 5

[David Durst [:ddurst]]

Should this remain P3 if it's blocking shipping?

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

I'll go ahead and call this P2, will see if johannf disagrees....

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Chris Pearce (:cpearce) from comment #6)
> I'll go ahead and call this P2, will see if johannf disagrees....

I meant johann*h*!

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Johann: Are you able to take this bug? We think that the control center should disappear when the page is refreshed.

Comment 9

[Johann Hofmann [:johannh]]

I can't commit to fixing this in the 63 cycle. Prathiksha may be interested in taking it, though. Or anyone from your team, see below for instructions:

Note that we do not want to hide the control center on any reload, just user initiated ones. In this case that just means when the user used the shortcut, since we hide the control center on mouse interaction outside anyway.

This is done in these two locations, I'm not sure if the shortcut actually uses both of them, though (that would have to be looked into):

https://searchfox.org/mozilla-central/rev/f0c15db995198a1013e1c5f5b5bea54ef83f1049/browser/base/content/browser.js#3208
https://searchfox.org/mozilla-central/rev/f0c15db995198a1013e1c5f5b5bea54ef83f1049/browser/base/content/tabbrowser.js#3314

Comment 10

[David Durst [:ddurst]]

cpearce -- have you found someone to take this, since soft freeze is next week?

Comment 11

[Dale Harvey (:daleharvey)]

I will see if I can get a patch up tomorrow

Comment 12

[Dale Harvey (:daleharvey)]

Attachment: 0001-Bug-477294-Hide-identity-popup-when-the-user-reloads.patch

Comment 13

[Johann Hofmann [:johannh]]

Comment on attachment 9001684 [details] [diff] [review]
0001-Bug-477294-Hide-identity-popup-when-the-user-reloads.patch

Review of attachment 9001684 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think test really covers what you're changing here, but I don't think it blocks review either. Please take another look at the test, though.

Thanks!

::: browser/base/content/browser.js
@@ +3225,5 @@
>    // Reset temporary permissions on the current tab. This is done here
>    // because we only want to reset permissions on user reload.
>    SitePermissions.clearTemporaryPermissions(gBrowser.selectedBrowser);
> +  PanelMultiView.hidePopup(gIdentityHandler._identityPopup);
> +

nit: does this need two newlines below it?

::: browser/base/content/tabbrowser.js
@@ +3353,4 @@
>      // Reset temporary permissions on the current tab. This is done here
>      // because we only want to reset permissions on user reload.
>      SitePermissions.clearTemporaryPermissions(browser);
> +    PanelMultiView.hidePopup(gIdentityHandler._identityPopup);

nit: please add a newline below ;)

::: browser/base/content/test/permissions/browser_permissions.js
@@ +277,5 @@
> +
> +    let reloadButton = document.getElementById("reload-button");
> +    let reloaded = BrowserTestUtils.browserLoaded(browser, false, PERMISSIONS_PAGE);
> +
> +    EventUtils.synthesizeMouseAtCenter(reloadButton, {});

As I mentioned in the bug, this should have worked already, since we hide the control center when the user clicks anywhere outside. Ideally we could synthesize a refresh key (F5?) and make sure that it also hides the popup, since that's what this bug is about.

Comment 14

[Dale Harvey (:daleharvey)]

Attachment: 0001-Bug-1477294-Hide-identity-popup-when-the-user-reload.patch

Switched the test to do an f5 reload, cheers

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/936a20fefe03
Hide identity popup when the user reloads. r=johannh

Comment 16

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/936a20fefe03

Comment 17

[Gyula Palko[:gpalko]]

Verified, that the issue is no longer reproducible on Nightly 63.0a1(20180819221915).