1353980 - Permission panel rapidly blinks and is attached to current tab after switching to fullscreen

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P1)

Description

[684sigma]

Attachment: Permission panel rapidly blinks after fullscreen, by reporter.webm

I have a problem with Firefox Beta 53. It also happens in Nightly 55, doesn't happen in Beta 52, ESR 45.
Permission panel rapidly blinks and is attached to current tab after switching to fullscreen.
Here's how to reproduce the bug:

1. Open https://www.youtube.com/watch?v=ZGqhhx8cxMg
2. Execute in console/scratchpad code "navigator.geolocation.getCurrentPosition(function(){})"
3. Click on fullscreen button in the video
4. Click "Exit fullscreen"
5. Scroll the tab strip to the left

Result: when switching to fullscreen, permission panel starts rapidly blinking (faster than visible on the video); after exiting fullscreen permission panel is attached to current tab; after scrolling tab strip permission panel goes off-screen
Expected: the panel shouldn't blink; should be attached to its utton; shouldn't go off-sceen

Comment 1

[684sigma]

Just verified that it indeed started from Beta 53 and Nightly 55 (2017-04-03) is affected.

Comment 2

[684sigma]

As suggested in Bug 1353769 comment 6, I tried to use Mozregression-gui.

It generated this regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=fc50ad3760aacd2475a834bba04ff1d37a6cf62e&tochange=57759203e1f77431f7916eb72b9129e26eb100b2

Regressed by Bug 1340538.

Comment 3

[Johann Hofmann [:johannh]]

Ouch, this is really bad. Thanks for reporting.

The changes from bug 1340538 make me believe this was actually regressed by bug 1109868. Neil, can you please look into this?

Comment 4

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce this issue on Firefox 55.0a1 (2017-04-05), Firefox 54.0a2 (2017-04-05) and Firefox 53.0b9 (20170403072723) under Windows 10 64-bit and Ubuntu 16.04 32-bit and I confirm that this issue is a side effect from Bug 1340538.

But, I suspect that this bug might be a duplicate of Bug 1345429.

Comment 5

[Neil Deakin]

The notification is being hidden because the anchor has disappeared when in fullscreen mode.

Some code in PopupNotifications.jsm is immediately reopening it again.

I would assume that a geolocation notification should not display while a fullscreen video is playing, so the popup code from 1109868 appears to working correctly here.

Comment 6

[Johann Hofmann [:johannh]]

I think we do want to show permission prompts though, in case it's a full screen app that needs permissions. In any case, this was the original behavior and I would like to avoid a discussion around that considering this should make it into beta. I'll try to fix the regression.

Comment 7

[Johann Hofmann [:johannh]]

Attachment: Bug 1353980 - Don't try to hide popup notifications in fullscreen mode.

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

Comment 8

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1b696c3309d0316f704564c7a8adccdc8f63b51

Comment 9

[:Paolo Amadini]

Comment on attachment 8855731 [details]
Bug 1353980 - Don't try to hide popup notifications in fullscreen mode.

https://reviewboard.mozilla.org/r/127630/#review130356

r+ because this fixes the bad flickering in fullscreen mode, although this doesn't fix the issue for which, after closing full screen mode, the panel is anchored to the tab.

As far as possible regressions go, the only thing I can think of right now is that we might end up with the notification still showing in some circumstances after closing full screen mode. This might happen if the "_update" method is not called for any reason.

::: toolkit/modules/PopupNotifications.jsm:1072
(Diff revision 1)
> +    // Bug 1353980: There are no anchor icons in DOM fullscreen
> +    // mode, but we would still like to show the popup notification.
> +    // To avoid an infinite loop of showing and hiding, we have to
> +    // disable followanchor (which hides the element without an anchor).

No need to put the bug number where this was fixed. If you have a plan for better fix, file a separate bug and put the bug number here.

Comment 10

[:Paolo Amadini]

Comment on attachment 8855731 [details]
Bug 1353980 - Don't try to hide popup notifications in fullscreen mode.

https://reviewboard.mozilla.org/r/127630/#review130358

::: toolkit/modules/PopupNotifications.jsm:1076
(Diff revision 1)
>  
> +    // Bug 1353980: There are no anchor icons in DOM fullscreen
> +    // mode, but we would still like to show the popup notification.
> +    // To avoid an infinite loop of showing and hiding, we have to
> +    // disable followanchor (which hides the element without an anchor).
> +    if (!!this.window.document.fullscreenElement) {

ESLint isn't happy about the double negation.

Comment 11

[Johann Hofmann [:johannh]]

(In reply to :Paolo Amadini from comment #9)
> Comment on attachment 8855731 [details]
> Bug 1353980 - Don't try to hide popup notifications in fullscreen mode.
> 
> https://reviewboard.mozilla.org/r/127630/#review130356
> 
> r+ because this fixes the bad flickering in fullscreen mode, although this
> doesn't fix the issue for which, after closing full screen mode, the panel
> is anchored to the tab.

Yeah, though the current state is even worse. When you exit fullscreen in my Nightly the notification is just hidden forever. We can make a followup. :)

> As far as possible regressions go, the only thing I can think of right now
> is that we might end up with the notification still showing in some
> circumstances after closing full screen mode. This might happen if the
> "_update" method is not called for any reason.

Is there a way to listen for entering DOM fullscreen mode? I'll check that. If it's too hard we could make a followup bug out of it.

Comment 12

[Johann Hofmann [:johannh]]

Comment on attachment 8855731 [details]
Bug 1353980 - Don't try to hide popup notifications in fullscreen mode.

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

Comment 13

[Johann Hofmann [:johannh]]

Comment on attachment 8855731 [details]
Bug 1353980 - Don't try to hide popup notifications in fullscreen mode.

This solves both issues for me! :amaze:

Comment 14

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68edd613749004836dfecc0cc7e857ec6e3d69ae

Comment 15

[:Paolo Amadini]

Comment on attachment 8855731 [details]
Bug 1353980 - Don't try to hide popup notifications in fullscreen mode.

Nice!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17e4fd366b5f
Don't try to hide popup notifications in fullscreen mode. r=Paolo

Comment 17

[Marcia Knous [:marcia]]

[Tracking Requested - why for this release]: Wont fix for 53 but we can revisit for 54.

I think we need to won't fix for 53, since it is pretty late for a fix for 53, as we are heading into the last week of the beta 53 cycle.

Comment 18

[Johann Hofmann [:johannh]]

(In reply to Marcia Knous [:marcia - use ni] from comment #17)
> [Tracking Requested - why for this release]: Wont fix for 53 but we can
> revisit for 54.
> 
> I think we need to won't fix for 53, since it is pretty late for a fix for
> 53, as we are heading into the last week of the beta 53 cycle.

That's unfortunate, this is a major regression in 53 and if there is any chance of getting it into late beta I would love if you could reconsider your decision. It was not explicitly mentioned in this bug, but I found during fixing that once this bug occurred (which is bad enough in itself) it prevents users from getting _any_ permission prompt until the browser restarts, even in normal, non-fullscreen mode.

I wanted to make that clear in my uplift request. Please let me know if that's still an option. The fix is really simple.

Thanks!

Comment 19

[Marcia Knous [:marcia]]

ni on Liz to weigh in on Comment 18.

Comment 20

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

Ah, ok. From the initial description it sounded like an edge case. We can try it for Monday's RC build.

Comment 21

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

Can you request uplift to aurora and beta then? Thanks.

Comment 22

[Johann Hofmann [:johannh]]

Of course, thank you.

Comment 23

[Johann Hofmann [:johannh]]

Comment on attachment 8855731 [details]
Bug 1353980 - Don't try to hide popup notifications in fullscreen mode.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1340538
[User impact if declined]: Users will experience heavily blinking == unusable permission prompts in fullscreen mode, plus there is the chance that all permission prompts will be entirely hidden even when exiting fullscreen, until browser restart (it happens intermittently for me, I think it's a timing issue)
[Is this code covered by automated tests?]: No, it's a visual issue
[Has the fix been verified in Nightly?]: Not yet. I'll make sure this gets tested ASAP.
[Needs manual test from QE? If yes, steps to reproduce]: See comment 0 (also works on http://permission.site/ when selecting "Fullscreen"
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: A few self-contained lines of JS that revert the one-line change in bug 1340538 in case we're in DOM fullscreen. The change from that bug was never needed in fullscreen, we just didn't think about that back then.
[String changes made/needed]: None

Comment 24

[Johann Hofmann [:johannh]]

Hi Brindusa,

can your team please make sure this gets tested ASAP when it hits the channels? Since we want to get this into late Beta it would be great if you could test not only that this is fixed on all platforms, but also that we didn't regress the existing main permission test cases and bug 1340538 (which is quite unlikely IMO).

Thank you! Sorry for the last-minute patch, we got aware of this quite late.

Comment 25

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

Comment on attachment 8855731 [details]
Bug 1353980 - Don't try to hide popup notifications in fullscreen mode.

Fix blinking fullscreen issues - let's avoid shipping this new issue.

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/17e4fd366b5f

Comment 27

[Hani Yacoub, Desktop QA]

Attachment: Recording #5.mp4

The permission panel doesn't blink any more and this issue is not reproducible using the steps from comment 0.

However, this issue can be reproducible and the permission panel is attached to the tab instead of the notification center.(Please see the screen cast attached)

Comment 28

[Johann Hofmann [:johannh]]

Yup, there's already bug 1355000 for it, though the behavior was definitely worse before this patch and I don't think the issue is bad enough to track for beta. I didn't know it also happens when opening a new permission prompt from within fullscreen. We might want to look into this for 54 or 55 then. Please add your screencast and STR to bug 1355000. Thanks!

Comment 29

[Hani Yacoub, Desktop QA]

Build ID: 20170409123541
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 30

[Carsten Book [:Tomcat]]

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

Comment 31

[Brindusa Tot, Desktop QA]

Based on comment 29 remove the NI.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f770bcf06b7b

Comment 33

[Hani Yacoub, Desktop QA]

Build ID: 20170411004009
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 34

[Alexandru Simonca [:asimonca], Release Desktop QA]

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

Verified as fixed on Firefox RC 53.0 on Windows 10 x64, Windows 7 x64, Mac OS X 10.12 and Ubuntu 16.04 x64.