Closed Bug 1116910 Opened 6 years ago Closed 6 years ago

Share button is larger than other buttons in ActionBar action mode on new tablet

Categories

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

All
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 38
Tracking Status
firefox36 --- verified
firefox37 --- verified
firefox38 --- verified

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(8 files, 4 obsolete files)

7.31 KB, application/zip
Details
66.88 KB, image/png
antlam
: feedback+
Details
5.46 KB, patch
capella
: review+
Details | Diff | Splinter Review
76.63 KB, image/png
Details
70.97 KB, image/png
Details
236.59 KB, image/png
Details
165.09 KB, image/png
Details
269.62 KB, image/png
Details
Found from bug 1111598. See [1] for screenshot.

Low priority as it doesn't look too terrible.

[1]: https://bug1111598.bugzilla.mozilla.org/attachment.cgi?id=8536567
Duplicate of Bug 1026436?
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Seems the issue here is mis-matched icon sizes. On xhdpi, expected:

  drawable-xhdpi/ab_copy.png PNG 64x64 64x64+0+0 8-bit sRGB 185B 0.000u 0:00.000

Actual:

  drawable-xhdpi/ic_menu_share.png PNG 84x84 84x84+0+0 8-bit sRGB 711B 0.000u 0:00.000

Anthony, can I get some assets for the share button? [1]

Notably, this issue does not affect phone, probably because the toolbar buttons centerInside [2] but the tablet toolbar is a larger height than on phones so the icon appears to grow. Alternatively, I can add some padding, which would prevent the icon from scaling too much.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/drawable-xhdpi/ic_menu_share.png
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/values-v11/styles.xml?rev=7be62fa11c20#96
Flags: needinfo?(alam)
Attached file icon_AB_share.zip (obsolete) —
These should be good.
Flags: needinfo?(alam)
I think we got trolled! The action bar assets contain whitespace (e.g. [1]). I filed bug 1122752 to remove this whitespace but it'd be faster and easier just to match the whitespace in the existing assets for this share asset.

Anthony, do you mind providing some new assets?
Flags: needinfo?(alam)
Adding padding!
Attachment #8550492 - Attachment is obsolete: true
Flags: needinfo?(alam)
Attachment #8550602 - Attachment is obsolete: true
Comment on attachment 8550602 [details]
Hamburger menu

Note that the hamburger menu's share is thus a different style than the action bar - are you okay with this, Anthony?

If so, I filed bug 1122826 to consolidate these styles (because it could require some work).
Attachment #8550602 - Attachment description: (Pre-patch) menu → Hamburger menu
Attachment #8550602 - Attachment is obsolete: false
Attachment #8550602 - Flags: feedback?(alam)
We can't actually use a different icon for the action bar on phone and tablet so let's just put it everywhere (i.e. bug 1122826).

f? for this and the next five images - you can see the existing icons in your local builds.
Attachment #8550602 - Attachment is obsolete: true
Attachment #8550616 - Attachment is obsolete: true
Attachment #8550602 - Flags: feedback?(alam)
Attachment #8550632 - Flags: feedback?(alam)
capella pointed out via IRC that the old patch breaks on phones - let's just replace the old icon then.
Attachment #8550634 - Flags: review?(markcapella)
Attachment #8550617 - Attachment is obsolete: true
Attachment #8550617 - Flags: review?(markcapella)
I can confirm that my 9" tablet (N9) looks similar to the 7" version.
Comment on attachment 8550634 [details] [diff] [review]
Add new share icons in the action bar for tablet

Review of attachment 8550634 [details] [diff] [review]:
-----------------------------------------------------------------

wfm - the difference is noticeable on my tablet, but on my phone I don't see it :-/
Attachment #8550634 - Flags: review?(markcapella) → review+
(In reply to Michael Comella (:mcomella) from comment #4)
> I think we got trolled! The action bar assets contain whitespace (e.g. [1]).
> I filed bug 1122752 to remove this whitespace but it'd be faster and easier
> just to match the whitespace in the existing assets for this share asset.

Thank you for filing bug 1122752. Ideally, we don't want padding.
Comment on attachment 8550632 [details]
(Post-patch) Phone action bar

+ nice work!

But I think it's slightly larger than the other icons in the AB.. does that matter with all the other work we're doing around removing the white space?
Attachment #8550632 - Flags: feedback?(alam) → feedback+
Flags: needinfo?(michael.l.comella)
We decided in our weekly tablet meeting that the in-asset padding on the share button is probably not large enough on the top causing a misalignment, but it's not that noticeable so we're going to land as is.

On holiday - please check in for me! :D Just an asset swap, no try run should be necessary.
Flags: needinfo?(michael.l.comella)
Keywords: checkin-needed
Comment on attachment 8550634 [details] [diff] [review]
Add new share icons in the action bar for tablet

Approval Request Comment
[Feature/regressing bug #]:
  New tablet UI (bug 1014156)

[User impact if declined]:
  Users will see a large share icon in the action bar on tablet, which looks unpolished

[Describe test coverage new/current, TreeHerder]: None

[Risks and why]: 
  In the worst case, we swap the wrong assets and the share button is completely broken in all contexts (action bar, context menu, hamburger menu). However, it's otherwise low risk - we're just swapping some assets for some new ones.

[String/UUID change made/needed]: N/A
Attachment #8550634 - Flags: approval-mozilla-beta?
Attachment #8550634 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/bc6777a08328
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Attachment #8550634 - Flags: approval-mozilla-beta?
Attachment #8550634 - Flags: approval-mozilla-beta+
Attachment #8550634 - Flags: approval-mozilla-aurora?
Attachment #8550634 - Flags: approval-mozilla-aurora+
Duplicate of this bug: 1122826
Verified as fixed on latest Aurora and Nightly on Nexus 7 (Android 5.0.1) and Sony Xperia Z2 SGP511 (Android 4.4.4). The share button looks fine.
Duplicate of this bug: 1026436
Verified as fixed in Firefox for Android 36 Beta 4;
Device: Asus Transformer Pad TF300T (Android 4.2.1)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.