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

VERIFIED FIXED in Firefox 53

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: 684sigma, Assigned: johannh)

Tracking

(Depends on 1 bug, {regression})

Trunk
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

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

Details

(Whiteboard: [fxprivacy])

Attachments

(3 attachments)

Reporter

Description

2 years ago
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
Reporter

Comment 1

2 years ago
Just verified that it indeed started from Beta 53 and Nightly 55 (2017-04-03) is affected.
Has STR: --- → yes
Keywords: regression
Reporter

Comment 2

2 years ago
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.
Blocks: 1340538
Flags: needinfo?(jhofmann)
Assignee

Comment 3

2 years ago
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?
Flags: needinfo?(jhofmann) → needinfo?(enndeakin)
Priority: -- → P1
Whiteboard: [fxprivacy]
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Unspecified → All
Hardware: Unspecified → All
Version: 53 Branch → Trunk

Comment 5

2 years ago
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.
Flags: needinfo?(enndeakin)
Assignee

Comment 6

2 years ago
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.
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 9

2 years ago
mozreview-review
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.
Attachment #8855731 - Flags: review?(paolo.mozmail) → review+

Comment 10

2 years ago
mozreview-review
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.
Assignee

Comment 11

2 years ago
(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 hidden (mozreview-request)
Assignee

Comment 13

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

This solves both issues for me! :amaze:
Attachment #8855731 - Flags: review+ → review?(paolo.mozmail)

Comment 15

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

Nice!
Attachment #8855731 - Flags: review?(paolo.mozmail) → review+
Assignee

Updated

2 years ago
Flags: qe-verify+

Comment 16

2 years ago
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/17e4fd366b5f
Don't try to hide popup notifications in fullscreen mode. r=Paolo
[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.
Assignee

Comment 18

2 years ago
(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!
Flags: needinfo?(mozillamarcia.knous)
ni on Liz to weigh in on Comment 18.
Flags: needinfo?(mozillamarcia.knous) → needinfo?(lhenry)
Ah, ok. From the initial description it sounded like an edge case. We can try it for Monday's RC build.
Flags: needinfo?(lhenry)
Can you request uplift to aurora and beta then? Thanks.
Flags: needinfo?(jhofmann)
Assignee

Comment 22

2 years ago
Of course, thank you.
Flags: needinfo?(jhofmann)
Assignee

Comment 23

2 years ago
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
Attachment #8855731 - Flags: approval-mozilla-beta?
Attachment #8855731 - Flags: approval-mozilla-aurora?
Assignee

Comment 24

2 years ago
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.
Flags: needinfo?(brindusa.tot)
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.
Attachment #8855731 - Flags: approval-mozilla-beta?
Attachment #8855731 - Flags: approval-mozilla-beta+
Attachment #8855731 - Flags: approval-mozilla-aurora?
Attachment #8855731 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/17e4fd366b5f
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Reporter

Updated

2 years ago
Depends on: 1355000

Comment 27

2 years ago
Posted video 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)

Updated

2 years ago
Flags: needinfo?(jhofmann)
Assignee

Comment 28

2 years ago
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!
Flags: needinfo?(jhofmann)

Comment 29

2 years ago
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.
Status: RESOLVED → VERIFIED
Based on comment 29 remove the NI.
Flags: needinfo?(brindusa.tot)

Comment 33

2 years ago
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.
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.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.