Closed Bug 1132747 Opened 5 years ago Closed 5 years ago

Fix Android L "share" list item in long press context menu

Categories

(Firefox for Android :: General, defect)

x86
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 40
Tracking Status
firefox38 --- verified
firefox39 --- verified
firefox40 --- verified
fennec 38+ ---

People

(Reporter: antlam, Assigned: mcomella)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

We could simply fix the padding on the left and top to match here.
tracking-fennec: --- → ?
Assignee: nobody → michael.l.comella
tracking-fennec: ? → 38+
Duplicate of this bug: 1146809
Attached image 17dp padding
Anthony, does this look about right?
Attachment #8584829 - Flags: feedback?(alam)
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)
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-
(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.
Attached image 18dp padding
Attachment #8584900 - Flags: feedback?(alam)
Flags: needinfo?(michael.l.comella)
/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)
This code is awful, but I did the best I could - explanatory comment in the changeset.
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-
Attachment #8584829 - Flags: feedback- → feedback+
(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)
Status: NEW → ASSIGNED
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+
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.
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.
https://hg.mozilla.org/mozilla-central/rev/a9738e5f636a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
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?
Michael,
[Describe test coverage new/current, TreeHerder]: None
=> You haven't done any tests?
Flags: needinfo?(michael.l.comella)
(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)
Flags: qe-verify+
Attachment #8584903 - Flags: approval-mozilla-beta?
Attachment #8584903 - Flags: approval-mozilla-beta+
Attachment #8584903 - Flags: approval-mozilla-aurora?
Attachment #8584903 - Flags: approval-mozilla-aurora+
Duplicate of this bug: 1148497
Verified as fixed on Beta 38.0b3 on nexus 5 with Android 5.1
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)
Status: RESOLVED → VERIFIED
Attachment #8584903 - Attachment is obsolete: true
Attachment #8619471 - Flags: review+
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.