1377802 - Permission prompt remains open after page navigation

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P1)

Description

[Justin Dolske [:Dolske]]

Attachment: screencast.mov

I was able to get a permission prompt to remain open after navigating away from the page that triggered it.

0) New profile.
1) Load maps.google.com
2) Click the location  button (bottom right) to trigger a location permission
3) Click in the URL bar, type "cnn.com<enter>"
4) CNN loads, permission prompt jumps to the bottom of the window

Curiously, if I then navigate again to a 3rd site, the permission prompt does get closed.

I can reproduce this with https://permission.site/ (so it's not a GMaps quirk).

Comment 1

[Florian Quèze [:florian]]

Seems very similar to bug 1340538.

Comment 2

[Johann Hofmann [:johannh]]

Yes we verified fixed this in bug 1340538. It's a little worrying that this is happening again. We need a regression range for this. I can reproduce on Windows Nightly (it's in the top right corner there).

Comment 3

[Johann Hofmann [:johannh]]

[Tracking Requested - why for this release]:
Serious permission UI regression.

A quick run of mozregression shows https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e7bf9443be2c4a5187c37440e35f3526148d7fa8&tochange=ff83fde8be946eabcf27ea97d4676f601c122194 is the pushlog. The most obvious choice here is bug 1358713 which got uplifted right into (now) release. :((((

Timothy, can you help us with this? What has to happen so that the permission doorhangers get followanchor again, i.e. disappear when their anchor element does?

Comment 4

[Timothy Nikkel (:tnikkel)]

What I see happening is the following. A panel gets opened anchor to this node

XUL* image@0x131a2ea10 id="geo-notification-icon" class="notification-anchor-icon geo-icon" role="button" tooltiptext="Open location request panel" showing="true" state=[20000000] flags=[00000104] primaryframe=0x1323c86d0 refcount=6<>

Then a panel is opened anchored to this node

XUL* image@0x131a2e1a0 id="identity-icon" consumeanchor="identity-box" onclick="PageProxyClickHandler(event);" tooltiptext="Show site information" state=[20000000] flags=[00000104] primaryframe=0x12dd0aa70 refcount=6<>

Then a panel is opened anchored to the geo-notification-icon node again, but now it is no longer visible (it does not have a frame), so that's where bug 1358713 comes in. Because the anchor had no frame when the panel was opened we don't consider it a follow anchor panel, so it stays visible.

In bug 1358713 we encounter the same situation (an anchor that is not visible when the panel is opened) except in that case we want the opposite to happen (the panel should be visible).

So the frontend code is going to have to pick one behaviour and stick with it.

In bug 1358713 I'm assuming that the frontend is trying to open a panel anchored to the bookmark icon, but the bookmark icon isn't visible, so the panel is not shown. This seems like a bug in the frontend code, so we should fix it in the frontend code and back out the platform fix for bug 1358713. Does that seem right?

Comment 5

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

Tracking since this sounds like an annoying recent regression from 53.

Comment 6

[Gerry Chang [:gchang]]

There is no plan to have another dot release for 54. Track 54- and won't fix in 54. Let' see if we can fix it in 55.

Comment 7

[Johann Hofmann [:johannh]]

Attachment: Bug 1377802 - Check if bookmarking UI is visible before anchoring bookmarking popup.

This also avoids flushing when bounds checking the identity popup for visibility.

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

Comment 8

[Johann Hofmann [:johannh]]

Attachment: Backed out changeset 4488424e14ae for causing Bug 1377802.

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

Comment 9

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8885486 [details]
Backed out changeset 4488424e14ae for causing Bug 1377802.

https://reviewboard.mozilla.org/r/156342/#review161470

Thanks!

Comment 10

[Johann Hofmann [:johannh]]

Comment on attachment 8885485 [details]
Bug 1377802 - Check if bookmarking UI is visible before anchoring bookmarking popup.

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

Comment 11

[Johann Hofmann [:johannh]]

Comment on attachment 8885486 [details]
Backed out changeset 4488424e14ae for causing Bug 1377802.

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

Comment 12

[Johann Hofmann [:johannh]]

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

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8885485 [details]
Bug 1377802 - Check if bookmarking UI is visible before anchoring bookmarking popup.

https://reviewboard.mozilla.org/r/156340/#review161572

off-hand it looks good, provided you have tested that this fixes the original bug, I'm fine with it.

Comment 14

[Johann Hofmann [:johannh]]

(In reply to Marco Bonardo [::mak] from comment #13)
> Comment on attachment 8885485 [details]
> Bug 1377802 - Check if bookmarking UI is visible before anchoring
> bookmarking popup.
> 
> https://reviewboard.mozilla.org/r/156340/#review161572
> 
> off-hand it looks good, provided you have tested that this fixes the
> original bug, I'm fine with it.

Thanks! It does, and it makes it much nicer (properly anchoring at the identity icon instead of somewhere random).

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/657535622b20
Check if bookmarking UI is visible before anchoring bookmarking popup. r=mak
https://hg.mozilla.org/integration/autoland/rev/632562f2d03b
Backed out changeset 4488424e14ae for causing Bug 1377802. r=tnikkel

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/632562f2d03b

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/657535622b20

Comment 18

[Sayed [:LiquidO2]]

I can reproduce this issue following the STR from comment #0 in both maps.google.com and in https://permission.site/ with Nightly 56.0a1 (2017-07-02) in 64-bit Linux

I can verify that this issue is fixed in latest Nightly 56.0a1.

Build ID 	20170714100217
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0

Comment 19

[Sajedul Islam]

I have reproduced this bug with Nightly 56.0a1 (2017-07-02) on Windows 8.1 , 64 Bit ! 

This bug's fix is Verified with latest Nightly 56.0a1 !

Build   ID    20170714030205
User Agent    Mozilla/5.0 (Windows NT 6.3; WOW64; rv:56.0) Gecko/20100101 Firefox/56.0

[bugday-20170712]

Comment 20

[Johann Hofmann [:johannh]]

Thank you both!

Comment 21

[Johann Hofmann [:johannh]]

Comment on attachment 8885485 [details]
Bug 1377802 - Check if bookmarking UI is visible before anchoring bookmarking popup.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1358713
[User impact if declined]: Permission prompt stays open and jumps to the bottom or the top of the window after navigating away from a page
[Is this code covered by automated tests?]: No 
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: See comment 0
[List of other uplifts needed for the feature/fix]: Both patches in this bug need to be uplifted
[Is the change risky?]: Low to medium
[Why is the change risky/not risky?]: It mostly backs out the platform patch that regressed this. The backout was clean and shouldn't cause any problems. The issue was fixed with a very simple frontend change.
[String changes made/needed]: None

Comment 22

[Johann Hofmann [:johannh]]

Comment on attachment 8885486 [details]
Backed out changeset 4488424e14ae for causing Bug 1377802.

See comment 21

Comment 23

[Julien Cristau [:jcristau]]

Comment on attachment 8885485 [details]
Bug 1377802 - Check if bookmarking UI is visible before anchoring bookmarking popup.

fix ui regression with permission prompts, beta55+

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f7250aab6183
https://hg.mozilla.org/releases/mozilla-beta/rev/c7cdbecbfc60

Comment 25

[Emil Ghitta, QA [:emilghitta]]

I have managed to reproduce the issue mentioned in comment 0 using Firefox 56.0a1 (BuildId:20170702030204).

This issue is verified fixed on Firefox 55.0b11 (BuildId:20170720171353) using Windows 10 64bit, Ubuntu 16.04 64bit and macOS 10.11.6.

This issue is also verified fixed on Firefox Nightly 56.0a1 (BuildId:20170723030206) using Windows 10 64 bit, Ubuntu 16.04 64 bit and macOS 10.11.6.