1151505 - Doorhanger can appear while browser toolbar is scrolled off screen

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

If you've scrolled the browser toolbar off screen and a doorhanger appears, we do not scroll the toolbar into view, and trying to scroll it into view yourself will dismiss the doorhanger.

Here's a testcase:
http://people.mozilla.org/~mleibovic/test/geo.html

Scroll down to the "Request location" button and press it.

I tested on release, and this isn't a regression, but it's a spoofing vulnerability, and also just looks weird.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Let's see if we can get the Toolbar to appear.

Comment 2

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: bz://1151505/liuche

/r/7417 - Bug 1151505 - Doorhanger can appear while browser toolbar is scrolled off screen. r=margaret

Pull down this commit:

hg pull -r 9dca8538e4bcd90cf35e81110fdc38a97a904aa7 https://reviewboard-hg.mozilla.org/gecko/

Comment 3

[:Margaret Leibovic]

Comment on attachment 8595641 [details]
MozReview Request: bz://1151505/liuche

https://reviewboard.mozilla.org/r/7415/#review6199

::: mobile/android/base/BrowserApp.java:1765
(Diff revision 1)
> +                });

While this is a nice, minimal patch, it makes assumptions about what happens when we get a Doorhanger:Add message. If you follow the logic in DoorhangerPopup, this actually isn't totally correct, since this message may actually be for adding a new doorhanger to a hidden tab.

I think what we should really do here is have a way for DoorhangerPopup to send a message to BrowserApp to show the toolbar when it shows the popup. Or perhaps BrowserApp can just observer whether or not the popup is shown? I know that PopupWindow has support for an OnDismissListener, but maybe we can create our own OnShowListener interface for DoorhangerPopup and set the listener in BrowserApp.

Comment 5

[Chenxia Liu [:liuche]]

Comment on attachment 8595641 [details]
MozReview Request: bz://1151505/liuche

/r/7417 - Bug 1151505 - Doorhanger can appear while browser toolbar is scrolled off screen. r=margaret
/r/7673 - Bug 1151505 - Adjust for tablet anchoring being offscreen. r=margaret

Pull down these commits:

hg pull -r e31c499a6e81ed179b2cef134d6f6fc35548fc75 https://reviewboard-hg.mozilla.org/gecko/

Comment 6

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/7415/#review6449

> While this is a nice, minimal patch, it makes assumptions about what happens when we get a Doorhanger:Add message. If you follow the logic in DoorhangerPopup, this actually isn't totally correct, since this message may actually be for adding a new doorhanger to a hidden tab.
> 
> I think what we should really do here is have a way for DoorhangerPopup to send a message to BrowserApp to show the toolbar when it shows the popup. Or perhaps BrowserApp can just observer whether or not the popup is shown? I know that PopupWindow has support for an OnDismissListener, but maybe we can create our own OnShowListener interface for DoorhangerPopup and set the listener in BrowserApp.

I removed the code that was throwing this into a show() infinite loop; that code was meant to handle the case where the dynamic toolbar is getting shown, but the anchoring at the time of show() would make the doorhanger appear over the toolbar. I moved this code into AnchoredPopup, and we anticipate where the doorhanger will show up and show the popup there.

Comment 7

[:Margaret Leibovic]

https://reviewboard.mozilla.org/r/7417/#review6499

::: mobile/android/base/BrowserApp.java
(Diff revision 2)
> -                    mDoorHangerPopup.updatePopup();

I did a bit of archeology to make sure I feel good about removing this code, and it looks like it was a bit of a hack added to work around the tablet issue that you're addressing more properly in your next patch. So remove away!

https://bugzilla.mozilla.org/show_bug.cgi?id=928800#c9

Comment 8

[:Margaret Leibovic]

Comment on attachment 8595641 [details]
MozReview Request: bz://1151505/liuche

https://reviewboard.mozilla.org/r/7415/#review6501

Ship It!

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/209f11506ce6
https://hg.mozilla.org/integration/fx-team/rev/f5107e422e34

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/209f11506ce6
https://hg.mozilla.org/mozilla-central/rev/f5107e422e34

Comment 11

[Teodora Vermesan (:TeoVermesan)]

Tested using:
Device: Alcatel One Touch (Android 4.1.2) and Asus Transformer (Android 4.0.3)
Build: Firefox for Android 40.0a1 (2015-04-29)

Phone:
Portrait: http://i.imgur.com/c38LoKS.png -> doorhanger is correctly displayed
Landscape: http://i.imgur.com/yivXHX4.png   -> doorhanger is misplaced. Is it expected? 

Tablet:
Portrait: http://i.imgur.com/ymZeGJe.jpg  -> a space is displayed between the URL Bar and doorhanger
Landscape: http://i.imgur.com/ltMC5EH.jpg -> a space is displayed between the URL Bar and doorhanger
Filled Bug 1159696 for this.

Comment 12

[Chenxia Liu [:liuche]]

(In reply to Teodora Vermesan (:TeoVermesan) from comment #11)
> Tested using:
> Device: Alcatel One Touch (Android 4.1.2) and Asus Transformer (Android
> 4.0.3)
> Build: Firefox for Android 40.0a1 (2015-04-29)
> 
> Phone:
> Portrait: http://i.imgur.com/c38LoKS.png -> doorhanger is correctly displayed
> Landscape: http://i.imgur.com/yivXHX4.png   -> doorhanger is misplaced. Is
> it expected? 

Yes, on phones, the doorhanger is expected to be centered on the screen - this differs from tablet layout, where the doorhanger should be anchored under the back button.

> 
> Tablet:
> Portrait: http://i.imgur.com/ymZeGJe.jpg  -> a space is displayed between
> the URL Bar and doorhanger
> Landscape: http://i.imgur.com/ltMC5EH.jpg -> a space is displayed between
> the URL Bar and doorhanger
> Filled Bug 1159696 for this.

Thanks for filing!

Comment 13

[Teodora Vermesan (:TeoVermesan)]

I will mark status-firefox40 to verified based on comment 11 and since Bug 1159696 is also fixed and verified.

Comment 15

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1151505 - Doorhanger can appear while browser toolbar is scrolled off screen. r=margaret

Comment 16

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1151505 - Adjust for tablet anchoring being offscreen. r=margaret