Closed
Bug 1155819
Opened 10 years ago
Closed 10 years ago
Tablet: anchor doorhanger under back button
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox38 unaffected, firefox39 verified, firefox40 verified)
VERIFIED
FIXED
Firefox 40
Tracking | Status | |
---|---|---|
firefox38 | --- | unaffected |
firefox39 | --- | verified |
firefox40 | --- | verified |
People
(Reporter: liuche, Assigned: liuche)
References
Details
Attachments
(6 files, 1 obsolete file)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
antlam
:
feedback+
|
Details |
(deleted),
patch
|
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details | |
(deleted),
text/x-review-board-request
|
liuche
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
liuche
:
review+
|
Details |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
Wow! This does exactly the right thing - this has no offsets applied, do you want any, antlam, or does this look fine?
Attachment #8594272 -
Flags: feedback?(alam)
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
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.
Assignee: nobody → liuche
Comment 5•10 years ago
|
||
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?
Flags: needinfo?(liuche)
Updated•10 years ago
|
Attachment #8594272 -
Flags: feedback?(alam) → feedback+
Assignee | ||
Comment 6•10 years ago
|
||
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.
Assignee | ||
Comment 7•10 years ago
|
||
/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/
Attachment #8594895 -
Flags: review?(ally)
Comment 8•10 years ago
|
||
looks good!
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(liuche)
Comment 9•10 years ago
|
||
Comment 10•10 years ago
|
||
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•10 years ago
|
||
Comment 12•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ab85ee309a19
https://hg.mozilla.org/mozilla-central/rev/4bda09d4350c
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Assignee | ||
Comment 13•10 years ago
|
||
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.
Flags: needinfo?(ally)
Attachment #8594895 -
Flags: review?(ally) → review+
Comment 14•10 years ago
|
||
bah! new fangled technology. Thanks for letting me know.
Flags: needinfo?(ally)
Assignee | ||
Comment 15•10 years ago
|
||
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
Attachment #8595519 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
status-firefox38:
--- → unaffected
status-firefox39:
--- → affected
Comment 16•10 years ago
|
||
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+
Attachment #8595519 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 17•10 years ago
|
||
Comment 18•10 years ago
|
||
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?
Updated•10 years ago
|
QA Contact: ioana.chiorean
Assignee | ||
Comment 19•10 years ago
|
||
Ioana: Ah, good catch! I forgot that Site Identity handles anchoring differently, so I filed bug 1157865.
Comment 20•10 years ago
|
||
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•10 years ago
|
||
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.
Assignee | ||
Comment 22•10 years ago
|
||
Attachment #8594895 -
Attachment is obsolete: true
Attachment #8620086 -
Flags: review+
Attachment #8620087 -
Flags: review+
Assignee | ||
Comment 23•10 years ago
|
||
Assignee | ||
Comment 24•10 years ago
|
||
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•