Closed Bug 1072469 Opened 10 years ago Closed 10 years ago

Discuss new tablet browser toolbar height

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 35

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(5 files, 5 obsolete files)

56dp toolbar
538.55 KB, image/png
Details
60dp toolbar (60 dp width buttons)
540.46 KB, image/png
Details
60dp toolbar (56 dp width buttons)
540.21 KB, image/png
antlam
: feedback+
lucasr
: feedback+
Details
Change new tablet toolbar height to 60dp and menu buttons to 56x60dp
14.65 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
Part 2: Call super in ActionBarViewFlipper.onAttachedToWindow
1.14 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
Android's standard height is 56dp, but 60dp is our other option.
Attached image 56dp toolbar (obsolete) —
Attaching image of 56 dp (which we will probably land in V1 as per our convo)
Attached image 60dp toolbar (obsolete) —
for comparison, 60 dp.
WIP patches from bug 1058909: * 60dp: https://people.mozilla.com/~mcomella/apks/mcomella-1072469_01.apk * 56dp: https://people.mozilla.com/~mcomella/apks/mcomella-1072469_02.apk What do you think, guys?
Flags: needinfo?(lucasr.at.mozilla)
Flags: needinfo?(alam)
Attachment #8494694 - Attachment description: prev_tabletui_vert_mock9.png → 56dp toolbar
Attachment #8494695 - Attachment description: prev_tabletui_vert_mock7.png → 60dp toolbar
I have to say, the 60dp version looks more balanced :-)
Flags: needinfo?(lucasr.at.mozilla)
I'm assuming this decision is based on having the correct assets so I'm going to wait on bug 1072466 before we make a decision.
Depends on: 1072466
Good point Michael! But yeah, I think with content actually in the page, and on a device, it feels very different. Let's see how this new 9 patch sways our final decision now. Leaving NI on for now.
For the builds in comment 7, note that the reload button size is still incorrect (bug 1072466).
Attached image 60dp toolbar (obsolete) —
Attachment #8494695 - Attachment is obsolete: true
Attached image 56dp toolbar —
Attachment #8494694 - Attachment is obsolete: true
If we agree to go on with 60dp...
Attachment #8496311 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8496311 [details] [diff] [review] Change new tablet toolbar height to 60dp Review of attachment 8496311 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/moz.build @@ +409,5 @@ > 'TelemetryContract.java', > 'TextSelection.java', > 'TextSelectionHandle.java', > 'ThumbnailHelper.java', > + 'toolbar/ActionBarViewFlipper.java', Forgot to include ActionBarViewFlipper?
Attachment #8496311 - Flags: review?(lucasr.at.mozilla)
Also, the toolbar buttons are not properly centered in the 60dp version. Is this fixed in the patch?
Flags: needinfo?(lucasr.at.mozilla)
After centering the buttons vertically, I realized we didn't change the width of the buttons from the 56dp height toolbar - should the width of these buttons remain 56dp or jump up to 60dp, to match the toolbar height? I prefer 56dp.
Attachment #8496894 - Flags: feedback?(lucasr.at.mozilla)
Attachment #8496894 - Flags: feedback?(alam)
(In reply to Lucas Rocha (:lucasr) from comment #14) > Also, the toolbar buttons are not properly centered in the 60dp version. Is > this fixed in the patch? Fixed the tab panel button, reload will be fixed in bug 1072466 because it's a bit more complicated.
(In reply to Michael Comella (:mcomella) from comment #16) > Created attachment 8496894 [details] > 60dp toolbar (56 dp width buttons) > > After centering the buttons vertically, I realized we didn't change the > width of the buttons from the 56dp height toolbar - should the width of > these buttons remain 56dp or jump up to 60dp, to match the toolbar height? I > prefer 56dp. Yeah, 56dp width looks alright to me. Anthony?
Attachment #8496894 - Flags: feedback?(lucasr.at.mozilla) → feedback+
Cleared up via bug 1070087 :)
Flags: needinfo?(alam)
(In reply to Lucas Rocha (:lucasr) from comment #18) > Yeah, 56dp width looks alright to me. Anthony? Note that the design in bug 1070087 comment 5 specifies 48dp, rather than 56dp.
Comment on attachment 8496894 [details] 60dp toolbar (56 dp width buttons) + for the width of the buttons
Attachment #8496894 - Flags: feedback?(alam) → feedback+
Making the buttons have a smaller visual tap area will be bug 1070087.
Attachment #8497709 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8497709 [details] [diff] [review] Change new tablet toolbar height to 60dp and menu buttons to 56x60dp Review of attachment 8497709 [details] [diff] [review]: ----------------------------------------------------------------- Good. ::: mobile/android/base/resources/values-large-v11/styles.xml @@ +6,5 @@ > <resources> > > + <style name="UrlBar.ImageButton.NewTablet"> > + <item name="android:layout_width">@dimen/new_tablet_browser_toolbar_menu_item_width</item> > + <item name="android:layout_height">@dimen/new_tablet_browser_toolbar_height</item> Just use layout_height=match_parent from UrlBar.ImageButton instead? @@ +39,5 @@ > <style name="UrlBar.ImageButton.TabCount.NewTablet"> > <item name="android:background">@drawable/new_tablet_tabs_count</item> > + > + <!-- From UrlBar.ImageButton.NewTablet because we can't inherit directly. --> > + <item name="android:layout_width">@dimen/new_tablet_browser_toolbar_height</item> Why not new_tablet_browser_toolbar_menu_item_width? ::: mobile/android/base/toolbar/ActionBarViewFlipper.java @@ +18,5 @@ > + * A temporary view that sets its height based on whether we are on new tablet or not. > + * Note that this view should be removed when the old tablet is removed and replaced with using > + * browser_toolbar_height directly. > + */ > +public class ActionBarViewFlipper extends GeckoViewFlipper { I realize GeckoViewFlipper looks very general-purpose but, in practice, it was written to be used in the toolbar. So, feel free to just add this code directly to GeckoViewFlipper as it's just a temporary change anyway.
Attachment #8497709 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Lucas Rocha (:lucasr) from comment #23) > Just use layout_height=match_parent from UrlBar.ImageButton instead? Done. > Why not new_tablet_browser_toolbar_menu_item_width? Done. > I realize GeckoViewFlipper looks very general-purpose but, in practice, it > was written to be used in the toolbar. So, feel free to just add this code > directly to GeckoViewFlipper as it's just a temporary change anyway. Just going to leave it as the separate class - easier to rip out, and then I don't have to put in extra effort now for arguable gain later.
Ignore comment 25, bzpost & closed tree.
Backed out in https://hg.mozilla.org/integration/fx-team/rev/8931a5073b27 - robopan and robocheck2 were both angry about java.lang.IllegalArgumentException: Receiver not registered: android.widget.ViewFlipper$1@4124e930, a la https://tbpl.mozilla.org/php/getParsedLog.php?id=49367925&full=1&branch=fx-team
Comment on attachment 8499054 [details] [diff] [review] Part 2: Call super in ActionBarViewFlipper.onAttachedToWindow Review of attachment 8499054 [details] [diff] [review]: ----------------------------------------------------------------- r+ by :nalexander on IRC.
Attachment #8499054 - Flags: review+
Not sure why the try push in comment 31 would fail from these changes and S4 passed, so I pushed - yolo: remote: https://hg.mozilla.org/integration/fx-team/rev/ec342eaecda3 remote: https://hg.mozilla.org/integration/fx-team/rev/8e3e47367465
https://hg.mozilla.org/mozilla-central/rev/ec342eaecda3 https://hg.mozilla.org/mozilla-central/rev/8e3e47367465
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: