1155819 - Tablet: anchor doorhanger under back button

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

Attachment: Mock: Doorhanger anchoring on tablet

Comment 1

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Doorhanger anchored under back button

Wow! This does exactly the right thing - this has no offsets applied, do you want any, antlam, or does this look fine?

Comment 2

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/7243/

Comment 4

[Chenxia Liu [:liuche]]

Whoops, Ally, I didn't see had already filed the other bug - I think you blocked the wrong bug with it. I threw this patch together on Friday before I left.

Comment 5

[Allison Naaktgeboren :ally]

Er, so then what would you like to do here since we collided? I guess I can review it for you if antlam likes the screenshot?

Comment 6

[Chenxia Liu [:liuche]]

Yep, that sounds right - sorry about that, I wanted to get the patch done in case we can actually uplift it. Let me know if you have any questions about the patch/commit, since you mentioned you ran into some problems before.

What you were doing was trying to find a back button *in the doorhanger*'s content view, which definitely does not exist. If you look at where AnchoredPopup is created, we set the layout resource (the xml that describes the layout of the class, in this case, AnchoredPopup), so if you open up that xml file, you'll see that it's just the layout of the doorhanger - not the main app (and therefore does not contain a back button).

To find the back button, you should be looking at the toolbar code: since we have a significantly different layout on tablet than we do on phones, we definitely have different code paths.

http://mxr.mozilla.org/mozilla-central/find?string=browsertoolbar&tree=mozilla-central&hint=

One way to try to nail down where the code you're looking for is to mxr the Android resource id (in this case R.id.back) and then see where that layout file is used.

Comment 7

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: bz://1155819/liuche

/r/7241 - Bug 1155819 - Clean up phone code more. r=ally
/r/7243 - Bug 1155819 - Set anchor as back button for tablets. r=ally

Pull down these commits:

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

Comment 8

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

looks good!

Comment 9

[Allison Naaktgeboren :ally]

https://reviewboard.mozilla.org/r/7241/#review6081

Ship It!

Comment 10

[Allison Naaktgeboren :ally]

https://reviewboard.mozilla.org/r/7243/#review6079

Ship It!

::: mobile/android/base/toolbar/BrowserToolbarTabletBase.java:130
(Diff revision 1)
> +    @Override

oo, I didnt know we had a file like this! very nice

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ab85ee309a19
https://hg.mozilla.org/integration/fx-team/rev/4bda09d4350c

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ab85ee309a19
https://hg.mozilla.org/mozilla-central/rev/4bda09d4350c

Comment 13

[Chenxia Liu [:liuche]]

Comment on attachment 8594895 [details]
MozReview Request: bz://1155819/liuche

"Ship it" from reviewboard didn't transfer over.

Ally (needinfo-ing so you see this): to r+ something, you actually need to go to the top level review and _then_ click Ship It! or Review or whatever. The changes to the top level review request and the changes get pushed to bugzilla.

Comment 14

[Allison Naaktgeboren :ally]

bah! new fangled technology. Thanks for letting me know.

Comment 15

[Chenxia Liu [:liuche]]

Attachment: Aurora patch: Anchor tablet under back button (folded the two commits into one)

Approval Request Comment
[Feature/regressing bug #]: Doorhanger redesign, feature is shipping with 39
[User impact if declined]: Tablets will have phone doorhanger UI
[Describe test coverage new/current, TreeHerder]: local, Nightly, Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5791c2917d0
[Risks and why]: low, simplifies code path, adds switch for tablet
[String/UUID change made/needed]: none

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8595519 [details] [diff] [review]
Aurora patch: Anchor tablet under back button (folded the two commits into one)

Looks good for Aurora. Aurora+

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/54478022f462

Comment 18

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot from 2015-04-23 10:42:17.png

Tested with: 
Device: Nexus 7 (Android 5.0)
Build; Firefox for Android 40.0a1 (2015-04-22)

Verified as fixed for password, geolocation and pop-up doorhanger.
But is it expected for mixed content blocking doorhanger not to be displayed
under the back button?

Comment 19

[Chenxia Liu [:liuche]]

Ioana: Ah, good catch! I forgot that Site Identity handles anchoring differently, so I filed bug 1157865.

Comment 20

[Teodora Vermesan (:TeoVermesan)]

Tested with: 
Device: Nexus 7 (Android 5.0)
Build; Firefox for Android 39.0a2 (2015-04-24)

Verified as fixed for password, geolocation and pop-up doorhanger.

Comment 21

[Teodora Vermesan (:TeoVermesan)]

I will mark this bug as VERIFIED FIXED based on comment 18 and comment 20 for password, geolocation and pop-up doorhangers and also that Bug 1157865 is fixed and verified for the site identity popup.

Comment 23

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1155819 - Clean up phone code more. r=ally

Comment 24

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1155819 - Set anchor as back button for tablets. r=ally