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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Attachment: N6 Screenshot_2015-02-12-15-08-38.png

We could simply fix the padding on the left and top to match here.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 17dp padding

Anthony, does this look about right?

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 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

Comment 4

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

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.

Comment 5

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

(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.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 18dp padding

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 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

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

This code is awful, but I did the best I could - explanatory comment in the changeset.

Comment 9

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

Comment on attachment 8584900 [details]
18dp padding

Gah - sorry, it looks like 17 was better. Let's go back to that, heh :)

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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.

Comment 11

[Martyn Haigh (:mhaigh)]

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.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/4458dea727ed

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Backed out for crashes.
https://hg.mozilla.org/integration/fx-team/rev/d35c3eb5eed8

https://treeherder.mozilla.org/logviewer.html#?job_id=2540465&repo=fx-team

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=73188995644b

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/a9738e5f636a

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a9738e5f636a

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Michael,
[Describe test coverage new/current, TreeHerder]: None
=> You haven't done any tests?

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6f8b5fba706c

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cbe44fd0d2fc

Comment 25

[Catalin Suciu [:csuciu]]

Verified as fixed on Beta 38.0b3 on nexus 5 with Android 5.1

Comment 26

[Teodora Vermesan (:TeoVermesan)]

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)

Comment 28

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1132747 - Set the padding for share in the context menu on Lollipop. r=mhaigh