Closed Bug 1151505 Opened 6 years ago Closed 6 years ago

Doorhanger can appear while browser toolbar is scrolled off screen

Categories

(Firefox for Android :: General, defect)

35 Branch
All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 40
Tracking Status
firefox37 --- affected
firefox38 --- affected
firefox39 --- affected
firefox40 --- verified
fennec 40+ ---

People

(Reporter: Margaret, Assigned: liuche)

References

()

Details

Attachments

(2 files, 1 obsolete file)

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.
Let's see if we can get the Toolbar to appear.
Assignee: nobody → liuche
tracking-fennec: ? → 40+
Attached file MozReview Request: bz://1151505/liuche (obsolete) —
/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/
Attachment #8595641 - Flags: review?(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.
Attachment #8595641 - Flags: review?(margaret.leibovic)
Duplicate of this bug: 858483
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/
Attachment #8595641 - Flags: review?(margaret.leibovic)
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.
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 on attachment 8595641 [details]
MozReview Request: bz://1151505/liuche

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

Ship It!
Attachment #8595641 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/209f11506ce6
https://hg.mozilla.org/mozilla-central/rev/f5107e422e34
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
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.
(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!
Blocks: 1159696
No longer depends on: 1159696
I will mark status-firefox40 to verified based on comment 11 and since Bug 1159696 is also fixed and verified.
Attachment #8595641 - Attachment is obsolete: true
Attachment #8619978 - Flags: review+
Attachment #8619979 - Flags: review+
You need to log in before you can comment on or make changes to this bug.