Discuss new tablet browser toolbar height

RESOLVED FIXED in Firefox 35

Status

()

Firefox for Android
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mcomella, Assigned: mcomella)

Tracking

unspecified
Firefox 35
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 5 obsolete attachments)

Android's standard height is 56dp, but 60dp is our other option.
Created attachment 8494694 [details]
56dp toolbar

Attaching image of 56 dp (which we will probably land in V1 as per our convo)
Created attachment 8494695 [details]
60dp toolbar

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).
Created attachment 8496308 [details]
60dp toolbar
Attachment #8494695 - Attachment is obsolete: true
Created attachment 8496310 [details]
56dp toolbar
Attachment #8494694 - Attachment is obsolete: true
Created attachment 8496311 [details] [diff] [review]
Change new tablet toolbar height to 60dp

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)
Created attachment 8496891 [details]
60dp toolbar (60 dp width buttons)
Attachment #8496308 - Attachment is obsolete: true
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.
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?

Updated

4 years ago
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+
Created attachment 8497709 [details] [diff] [review]
Change new tablet toolbar height to 60dp and menu buttons to 56x60dp

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+
Attachment #8496311 - Attachment is obsolete: true
Created attachment 8498453 [details] [diff] [review]
Change new tablet toolbar height to 60dp and menu buttons to 56x60dp

(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.
Attachment #8497709 - Attachment is obsolete: true
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
Created attachment 8499054 [details] [diff] [review]
Part 2: Call super in ActionBarViewFlipper.onAttachedToWindow

Patch to land on top of the previous patch.
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
You need to log in before you can comment on or make changes to this bug.