[Tablet] Insert vertical divider between back button and normal browsing button in Tabs Panel

VERIFIED FIXED in Firefox 36

Status

()

enhancement
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: u421692, Assigned: mhaigh)

Tracking

Trunk
Firefox 38
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 verified, firefox37 verified, firefox38 verified)

Details

Attachments

(6 attachments)

To be consistent, we should add a vertical divider between the back button and normal browsing, just like between normal browsing and private browsing. (see attached screenshot)
Hm, interesting. I think an identical vertical divider would be confusing, as if the back button is another category of tabs to click (like "normal" and "private"). Maybe a double vertical divider is more appropriate? Or one that doesn't fill the entire length of the toolbar?

Anthony, any thoughts?
Blocks: 1100464
Flags: needinfo?(alam)
Actually, the mock from bug 1100464 comment 2 [1] has no dividers whatsoever - perhaps that's the solution here (and one that doesn't require :antlam feedback!).

[1]: https://bug1100464.bugzilla.mozilla.org/attachment.cgi?id=8524691
I think Lucas meant to add this divider back but forgot :P Yes, let's keep it consistent and put a divider back up there :)
Flags: needinfo?(alam)
Assignee: nobody → mhaigh
Here's how it looks
Comment on attachment 8551736 [details] [diff] [review]
Bug 1110149 - [Tablet] Insert vertical divider between back button and normal browsing button in Tabs Panel

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

lgtm, w/ nits.

::: mobile/android/base/resources/values/attrs.xml
@@ +181,5 @@
>      </declare-styleable>
>  
> +    <declare-styleable name="TabPanelBackButton">
> +        <attr name="rightDivider" format="reference"/>
> +        <attr name="dividerTopPadding" format="dimension"/>

nit: -> dividerPadding

It's used on the bottom too.

::: mobile/android/base/resources/values/dimens.xml
@@ +22,4 @@
>      <dimen name="new_tablet_nav_button_width_half">21dp</dimen>
>      <dimen name="new_tablet_nav_button_width_plus_half">63dp</dimen>
>  
> +    <dimen name="new_tablet_tab_panel_divider_padding">12dp</dimen>

You mentioned you got this from the system, right? Add a comment to that effect.

nit: -> `..._vertical_padding`

::: mobile/android/base/tabs/TabPanelBackButton.java
@@ +10,5 @@
> +import android.widget.ImageButton;
> +import android.widget.LinearLayout;
> +
> +/**
> + * Created by martyn on 16/01/15.

Well, we know who owns *this*, don't we!

nit: I haven't seen this anywhere else so for consistency, ax it! (or at least @author)

@@ +14,5 @@
> + * Created by martyn on 16/01/15.
> + */
> +public class TabPanelBackButton extends ImageButton {
> +
> +    private int mDividerWidth = 0;

nit: Our convention is that new files do not use Hungarian notation - remove the `m`.

nit: Unnecessary initialization - also (disclaimer: my idiosyncrasy) use `final`!

@@ +15,5 @@
> + */
> +public class TabPanelBackButton extends ImageButton {
> +
> +    private int mDividerWidth = 0;
> +    Drawable mDivider;

nit: private final

@@ +16,5 @@
> +public class TabPanelBackButton extends ImageButton {
> +
> +    private int mDividerWidth = 0;
> +    Drawable mDivider;
> +    private int mDividerPadding = 0;

nit: Same as mDividerWidth.

@@ +26,5 @@
> +        mDividerPadding = (int) a.getDimension(R.styleable.TabPanelBackButton_dividerTopPadding, 0);
> +        a.recycle();
> +        if (mDivider != null) {
> +            mDividerWidth = mDivider.getIntrinsicWidth();
> +        }

nit: I'd like a little more vertical whitespace in this constructor.

@@ +39,5 @@
> +    @Override
> +    protected void onDraw(Canvas canvas) {
> +        super.onDraw(canvas);
> +        if (mDivider != null) {
> +            final LinearLayout.LayoutParams lp = (LinearLayout.LayoutParams) getLayoutParams();

nit: You should be able to use MarginLayoutParams - it's more flexible.

@@ +40,5 @@
> +    protected void onDraw(Canvas canvas) {
> +        super.onDraw(canvas);
> +        if (mDivider != null) {
> +            final LinearLayout.LayoutParams lp = (LinearLayout.LayoutParams) getLayoutParams();
> +            final int position = getRight() - lp.rightMargin - mDividerWidth;

nit: position -> left (or similar)
Attachment #8551736 - Flags: review?(michael.l.comella) → review+
Ah - forgot I'd installed the EAP version of IntelliJ and it reverted all my config.  Anyway, that file is mine - HANDS OFF!

> nit: -> dividerPadding

Can't call it dividerPadding as there's already a attribute named that - going for dividerVerticalPadding.

Other nits addressed
Note - remember to add new files to moz.build when developing in IntelliJ w/ Gradle!

https://hg.mozilla.org/integration/fx-team/rev/30a86cc4e277
https://hg.mozilla.org/mozilla-central/rev/e6d438f2b646
https://hg.mozilla.org/mozilla-central/rev/30a86cc4e277
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Verfied as fixed on latest Nightly on Nexus 7 (Android 5.0.1) and Sony Xperia Z2 SGP511 (Android 4.4.4)
Status: RESOLVED → VERIFIED
Don't forget to uplift!
Flags: needinfo?(mhaigh)
Approval Request Comment
[Feature/regressing bug #]:
Insert a vertical divider in the tablet tabs tray between the back button and the normal tab

[User impact if declined]:
The back button and the normal tabs button will be visually indistinguishable 

[Describe test coverage new/current, TreeHerder]:
Landed on nightly a while ago with no fallout

[Risks and why]: 
We introduce a new file for the back button - if this file can't be found then we risk a crash.  We also change some styles which could cause the tabs panel button to have graphical artifacts around the dividers if something goes wrong.

[String/UUID change made/needed]:
None
Flags: needinfo?(mhaigh)
Attachment #8553795 - Flags: approval-mozilla-aurora?
Attachment #8553795 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Any issues with uplifting to Beta here too?
Flags: needinfo?(mhaigh)
I haven't requested uplift to beta here as we moved all the resources from newtablet/res to resources between alpha and beta, hence only uplifting to alpha atm.  

I can craft a new patch for uplift to beta, but when we spoke about it, it wasn't a high priority.
Flags: needinfo?(mhaigh) → needinfo?(michael.l.comella)
(In reply to Martyn Haigh (:mhaigh) from comment #17)
> I can craft a new patch for uplift to beta, but when we spoke about it, it
> wasn't a high priority.

Perhaps a miscommunication - can you craft that branch patch?
Flags: needinfo?(michael.l.comella) → needinfo?(mhaigh)
Hey - here's the patch for beta.  My beta build is broken at the moment - I don't suppose you'd sanity check this for me, would you?

ta
Flags: needinfo?(mhaigh) → needinfo?(michael.l.comella)
(In reply to Martyn Haigh (:mhaigh) from comment #19)
> My beta build is broken at the moment

Mine too: "RuntimeWarning: couldn't determine platform's TOTAL_PHYMEMX"

Maybe check out an earlier revision (hopefully after all of our uplifts) and build on top of there?
Flags: needinfo?(michael.l.comella) → needinfo?(mhaigh)
Approval Request Comment
[Feature/regressing bug #]:
Insert a vertical divider in the tablet tabs tray between the back button and the normal tab

[User impact if declined]:
The back button and the normal tabs button will be visually indistinguishable 

[Describe test coverage new/current, TreeHerder]:
Landed on nightly a while ago with no fallout

[Risks and why]: 
We introduce a new file for the back button - if this file can't be found then we risk a crash.  We also change some styles which could cause the tabs panel button to have graphical artifacts around the dividers if something goes wrong.

[String/UUID change made/needed]:
None
Flags: needinfo?(mhaigh)
Attachment #8557985 - Flags: approval-mozilla-beta?
Attachment #8557985 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
A vertical divider is now present between back button and normal browsing button in Tabs Panel, so:
Verified fixed on:
Device: Nexus 7 (Android 5.0.1)
Build: Firefox for Android beta 6
Verified as fixed in build 37.0a2 2015-02-03;
Device: Asus Transformer Tab (Android 4.0.3)
You need to log in before you can comment on or make changes to this bug.