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

VERIFIED FIXED in Firefox 38

Status

()

Firefox for Android
General
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: antlam, Assigned: mcomella)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 40
x86
Android
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox38 verified, firefox39 verified, firefox40 verified, fennec38+)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8563759 [details]
N6 Screenshot_2015-02-12-15-08-38.png

We could simply fix the padding on the left and top to match here.
Blocks: 1030897
No longer blocks: 1098596
tracking-fennec: --- → ?
Assignee: nobody → michael.l.comella
tracking-fennec: ? → 38+
Duplicate of this bug: 1146809
Created attachment 8584829 [details]
17dp padding

Anthony, does this look about right?
Attachment #8584829 - Flags: feedback?(alam)
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
Flags: needinfo?(alam)
(Reporter)

Comment 4

3 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

3 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.
Created attachment 8584900 [details]
18dp padding
Attachment #8584900 - Flags: feedback?(alam)
Flags: needinfo?(michael.l.comella)
Created attachment 8584903 [details]
MozReview Request: bz://1132747/mcomella

/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.
(Reporter)

Comment 9

3 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

3 years ago
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
Last Resolved: 3 years ago
status-firefox40: --- → fixed
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+
status-firefox38: --- → affected
status-firefox39: --- → affected
Attachment #8584903 - Flags: approval-mozilla-beta?
Attachment #8584903 - Flags: approval-mozilla-beta+
Attachment #8584903 - Flags: approval-mozilla-aurora?
Attachment #8584903 - Flags: approval-mozilla-aurora+
status-firefox38: affected → fixed
Duplicate of this bug: 1148497
Verified as fixed on Beta 38.0b3 on nexus 5 with Android 5.1
status-firefox38: fixed → verified
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
status-firefox39: fixed → verified
status-firefox40: fixed → verified
Comment on attachment 8584903 [details]
MozReview Request: bz://1132747/mcomella
Attachment #8584903 - Attachment is obsolete: true
Attachment #8619471 - Flags: review+
Created attachment 8619471 [details]
MozReview Request: Bug 1132747 - Set the padding for share in the context menu on Lollipop. r=mhaigh
You need to log in before you can comment on or make changes to this bug.