Closed
Bug 1132747
Opened 10 years ago
Closed 10 years ago
Fix Android L "share" list item in long press context menu
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox38 verified, firefox39 verified, firefox40 verified, fennec38+)
VERIFIED
FIXED
Firefox 40
People
(Reporter: antlam, Assigned: mcomella)
References
Details
Attachments
(5 files, 1 obsolete file)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
antlam
:
feedback+
|
Details |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
antlam
:
feedback-
|
Details |
MozReview Request: Bug 1132747 - Set the padding for share in the context menu on Lollipop. r=mhaigh
(deleted),
text/x-review-board-request
|
mhaigh
:
review+
|
Details |
We could simply fix the padding on the left and top to match here.
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Updated•10 years ago
|
tracking-fennec: --- → ?
Updated•10 years ago
|
Assignee: nobody → michael.l.comella
tracking-fennec: ? → 38+
Assignee | ||
Comment 2•10 years ago
|
||
Anthony, does this look about right?
Attachment #8584829 -
Flags: feedback?(alam)
Assignee | ||
Comment 3•10 years ago
|
||
Problem: Android L increases the padding to the sides of menu items to 17dp (16dp according to [1]).
By convention, this should also affect the toolbar menu, but since we only use our own styles, it is unaffected.
Here is a screenshot with the values used in the Android menu convention - we don't use their menu width conventions so it looks wonky but I just wanted to bring this to your awareness, Anthony.
[1]: http://www.google.com/design/spec/components/menus.html#menus-specs
Flags: needinfo?(alam)
Reporter | ||
Comment 4•10 years ago
|
||
Comment on attachment 8584829 [details]
17dp padding
I think it needs + 1 dp margin on the left still. The S is just a bit out of line with the O.
Flags: needinfo?(alam) → needinfo?(michael.l.comella)
Attachment #8584829 -
Flags: feedback?(alam) → feedback-
Reporter | ||
Comment 5•10 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #3)
> Created attachment 8584831 [details]
> Toolbar menu, 17dp padding (by convention)
>
> Problem: Android L increases the padding to the sides of menu items to 17dp
> (16dp according to [1]).
>
> By convention, this should also affect the toolbar menu, but since we only
> use our own styles, it is unaffected.
>
> Here is a screenshot with the values used in the Android menu convention -
> we don't use their menu width conventions so it looks wonky but I just
> wanted to bring this to your awareness, Anthony.
>
> [1]: http://www.google.com/design/spec/components/menus.html#menus-specs
Nice catch. This is something I've been mentioning in bug 1140169. Originally, I wanted to define the left padding to be 15dp (same as our outer gutters app wide) but I didn't split it off to be tracked in it's own bug - for the same width-to-convention wonky-ness you mentioned.
I agree, it has to do with our custom written menu and it's UI like "width". I'm also concerned about localization and how it might be affected if we pad the text labels in (to 16dp).
Will keep this in mind moving forward, but nothing to do here about that for the time being I think. I'll file it under the menu polish if needed.
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8584900 -
Flags: feedback?(alam)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(michael.l.comella)
Assignee | ||
Comment 7•10 years ago
|
||
/r/6237 - Bug 1132747 - Set the padding for share in the context menu on Lollipop. r=mhaigh
Pull down this commit:
hg pull review -r 71e753e2447d30af975734100c1bddccd1dfb52e
Attachment #8584903 -
Flags: review?(mhaigh)
Assignee | ||
Comment 8•10 years ago
|
||
This code is awful, but I did the best I could - explanatory comment in the changeset.
Reporter | ||
Comment 9•10 years ago
|
||
Comment on attachment 8584900 [details]
18dp padding
Gah - sorry, it looks like 17 was better. Let's go back to that, heh :)
Flags: needinfo?(michael.l.comella)
Attachment #8584900 -
Flags: feedback?(alam) → feedback-
Reporter | ||
Updated•10 years ago
|
Attachment #8584829 -
Flags: feedback- → feedback+
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Anthony Lam (:antlam) from comment #9)
> Gah - sorry, it looks like 17 was better. Let's go back to that, heh :)
Changed locally.
Flags: needinfo?(michael.l.comella)
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Comment 11•10 years ago
|
||
Comment on attachment 8584903 [details]
MozReview Request: bz://1132747/mcomella
https://reviewboard.mozilla.org/r/6235/#review5387
Yeah, it's not pretty but it looks alright. Maybe an idea to file a bug to try and tweeze apart MenuItemActionView from the GeckoActionProvider and add a TODO with the bug number.
::: mobile/android/base/widget/GeckoActionProvider.java
(Diff revision 1)
> + CONTEXT_MENU,
I don't know if this is Moz style, but I'm not a fan of the hanging comma.
::: mobile/android/base/widget/GeckoActionProvider.java
(Diff revision 1)
> +
nit: Insert new lines before each case statement (not the first!) or get rid of the new line before default.
Attachment #8584903 -
Flags: review?(mhaigh) → review+
Assignee | ||
Comment 12•10 years ago
|
||
https://reviewboard.mozilla.org/r/6235/#review5415
> I don't know if this is Moz style, but I'm not a fan of the hanging comma.
Not moz style but it's better for version control hisotry - when you add a new element, you only have to change one line, not two, so it becomes more apparent where new enum values may have been added.
Assignee | ||
Comment 13•10 years ago
|
||
Comment 14•10 years ago
|
||
Assignee | ||
Comment 15•10 years ago
|
||
Assignee | ||
Comment 16•10 years ago
|
||
I tried to fix a broken inheritance chain (I filed bug 1151147 for this) by using a different constructor, but it backfired. The push in comment 15 removes this change in inheritance.
Assignee | ||
Comment 17•10 years ago
|
||
Comment 18•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Assignee | ||
Comment 19•10 years ago
|
||
Comment on attachment 8584903 [details]
MozReview Request: bz://1132747/mcomella
Approval Request Comment
[Feature/regressing bug #]: 1097337
[User impact if declined]:
Users will see "share" misaligned in the long-press context menu when they have not shared ever before (i.e. polish).
[Describe test coverage new/current, TreeHerder]: None
[Risks and why]:
Essentially, we detect when the view created is to be used in the context menu and, if so, dynamically change the styles such that the padding is correct. The biggest risk comes from this detection - we use a method called PromptListAdapter.getActionView. PromptListAdapter is used by the context menus but the name doesn't seem specific to the context menu and it is used elsewhere. However, getActionView is only used for "action views" (views like the quick share) and the other uses of PromptListAdapter probably don't access this method.
Worth noting: this menu code is notoriously confusing and poorly named so it can be difficult to work in.
In the worst case, we add some extra padding to the left of a view that shouldn't have had this extra padding.
[String/UUID change made/needed]: None
Attachment #8584903 -
Flags: approval-mozilla-beta?
Attachment #8584903 -
Flags: approval-mozilla-aurora?
Comment 20•10 years ago
|
||
Michael,
[Describe test coverage new/current, TreeHerder]: None
=> You haven't done any tests?
Flags: needinfo?(michael.l.comella)
Assignee | ||
Comment 21•10 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] from comment #20)
> Michael,
> [Describe test coverage new/current, TreeHerder]: None
> => You haven't done any tests?
I thought this section described automated testing. I have tested this locally and visually on my N4 Lollipop to verify the change and on my N7 4.4 to confirm the padding did not change.
Flags: needinfo?(michael.l.comella)
Updated•10 years ago
|
Flags: qe-verify+
Updated•10 years ago
|
status-firefox38:
--- → affected
status-firefox39:
--- → affected
Updated•10 years ago
|
Attachment #8584903 -
Flags: approval-mozilla-beta?
Attachment #8584903 -
Flags: approval-mozilla-beta+
Attachment #8584903 -
Flags: approval-mozilla-aurora?
Attachment #8584903 -
Flags: approval-mozilla-aurora+
Comment 22•10 years ago
|
||
Assignee | ||
Comment 23•10 years ago
|
||
Updated•10 years ago
|
Comment 25•10 years ago
|
||
Verified as fixed on Beta 38.0b3 on nexus 5 with Android 5.1
Comment 26•10 years ago
|
||
Verified as fixed using:
Device: Nexus 5 (Android 5.0)
Build: Firefox for Android 40.0a1 (2015-04-22) and Firefox for Android 39.0a2 (2015-04-22)
Assignee | ||
Comment 27•9 years ago
|
||
Attachment #8584903 -
Attachment is obsolete: true
Attachment #8619471 -
Flags: review+
Assignee | ||
Comment 28•9 years ago
|
||
Updated•6 years ago
|
Flags: qe-verify+
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
•