Closed Bug 1148390 Opened 6 years ago Closed 6 years ago
Share icon from action bar is misaligned on gingerbread devices
54.65 KB, image/png
51.45 KB, image/png
54.18 KB, image/png
39 bytes, text/x-review-board-request
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.
[Tracking Requested - why for this release]: regression/polish
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.
Capellla, have you touched this action bar recently? Would you be interested in taking this bug?
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?
ping mcomella: ... is this related at all to Bug 1116910 ?
(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
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?
Assignee: nobody → michael.l.comella
Michael, are you going to fix this issue during the 38 cycle? Thanks
(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.
tracking-fennec: 38+ → ?
Looks like we use menuItemActionModeStyle  = 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. : https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/TextSelection.java?rev=1ee3f4cdae47#280
/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/
Attachment #8591973 - Flags: review?(markcapella)
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?
Attachment #8591973 - Flags: review?(markcapella) → feedback+
Attachment #8591973 - Flags: review?(wjohnston)
Attachment #8591973 - Flags: review?(wjohnston) → review+
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 on attachment 8591973 [details] MozReview Request: bz://1148390/mcomella Should be in 38 beta 6.
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)
tracking-fennec: ? → 38+
Verified as fixed in Firefox 38 Beta 8 build 3; Device: Samsung Galaxy R (Android 2.3.4).
You need to log in before you can comment on or make changes to this bug.