1148390 - Share icon from action bar is misaligned on gingerbread devices

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Flaviu Cos]

Attachment: misaligned share icon.png

Environment: 
Device: Samsung Galaxy R (Android 2.3.4);
Build: Nightly 39.0a1 (2015-03-26);

Steps to reproduce:
1. Load a page (e.g.: en.wikipedia.org/wiki/Mozilla);
2. Long tap on a word.

Expected result:
The icons form action bar are correctly aligned.

Actual result:
Share icon is misaligned.

Notes:
Please check the attached screenshot.

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: regression/polish

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 38 and 39 since this is a regression. 
Sylvestre, I figured you would probably want to track and maybe take this in early beta if there is a simple fix.

Comment 3

[:Margaret Leibovic]

Capellla, have you touched this action bar recently? Would you be interested in taking this bug?

Comment 4

[Mark Capella [:capella]]

I can look. I've reviewed the code on a number of occasions, but I haven't changed anything. Is it actually a regression (tied to recent share-icon resource changes?), or a specific Gingerbread thing?

Comment 5

[Mark Capella [:capella]]

ping mcomella: ... is this related at all to Bug 1116910 ?

Comment 6

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

(In reply to Mark Capella [:capella] from comment #5)
> ping mcomella: ... is this related at all to Bug 1116910 ?

I think it is to blame, yes - we replaced the assets there to work better on tablet, but it seems it's missing some style elements for GB. I wonder if there is an easy way to change the styles in the ActionBar?

Mark, if you want to take a stab, please go ahead, but I'll jump on this when I finish fixing up a few more regressions. :P

Comment 7

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

Attachment: Tapped icons

Looks like the problem is because the button housing the share icon does not fill the space (see screenshot). Notably, the other assets have padding in the image so that they fill the space.

> GB doesn't have this problem depsite the padding - perhaps the size of the buttons are explicitly set?

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Michael, are you going to fix this issue during the 38 cycle? Thanks

Comment 9

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

(In reply to Sylvestre Ledru [:sylvestre] from comment #8)
> Michael, are you going to fix this issue during the 38 cycle? Thanks

Trying, but I think it's low priority and wouldn't mind a slip to 39.

Comment 10

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

Looks like we use menuItemActionModeStyle [1] = GeckoActionBar.Button for the individual items. I tried setting explicit heights and widths to no avail - probably a quality of the underlying ImageButton - I'm not sure why v11+ works correctly though.

I noticed I can change padding to reduce the size of the images (and perhaps can make all of the icons the same size by using that), but I think the proper solution is to remove the whitespace in the other images, ala bug 1122752.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/TextSelection.java?rev=1ee3f4cdae47#280

Comment 11

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

Attachment: Screenshot (post commit)

Comment 12

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

Attachment: MozReview Request: bz://1148390/mcomella

/r/7019 - Bug 1148390 - Dynamically add padding to share icon on GB devices. r=capella

Pull down this commit:

hg pull -r 4750f2deab2340d26ef2d4a86df35c0aefef1ea5 https://reviewboard-hg.mozilla.org/gecko/

Comment 13

[Mark Capella [:capella]]

Comment on attachment 8591973 [details]
MozReview Request: bz://1148390/mcomella

This looks fine to me, but I don't have a GB device to test properly. Can you run this past wesj for proper approval?

Comment 14

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

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

Comment 15

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

Comment on attachment 8591973 [details]
MozReview Request: bz://1148390/mcomella

Approval Request Comment
[Feature/regressing bug #]: bug 1116910
[User impact if declined]:
  Users on GB devices see a mis-aligned share icon in the action bar (which appears when long-press selecting text)

[Describe test coverage new/current, TreeHerder]:
  Tested locally on my Android L N4 and a GB device

[Risks and why]:
  Low - dynamically change the padding on only the share icon on GB. Worst case, we incorrectly check the device configuration and we apply this change to other devices. We can also get the padding amount incorrect, particularly on different device configurations (e.g. different screen sizes) but I think it should be the same across devices.
 
[String/UUID change made/needed]: None

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/73daa8f0f5f0

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8591973 [details]
MozReview Request: bz://1148390/mcomella

Should be in 38 beta 6.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/098c9a1c1390

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Teodora Vermesan (:TeoVermesan)]

Verified as fixed using:
Device: HTC Desire HD (Android 2.3.5)
Builds: Firefox for Android 39.0a2 (2015-04-16) and Firefox for Android 40.0a1 (2015-04-16)

Comment 21

[Flaviu Cos]

Verified as fixed in Firefox 38 Beta 8 build 3;
Device: Samsung Galaxy R (Android 2.3.4).

Comment 23

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

Attachment: MozReview Request: Bug 1148390 - Dynamically add padding to share icon on GB devices. r=capella