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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[u421692]

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)

Comment 1

[u421692]

Attachment: device-2014-12-11-163230.png

Comment 2

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

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?

Comment 3

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

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

Comment 4

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

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 :)

Comment 5

[Martyn Haigh (:mhaigh)]

Attachment: Bug 1110149 - [Tablet] Insert vertical divider between back button and normal browsing button in Tabs Panel

Comment 6

[Martyn Haigh (:mhaigh)]

Attachment: Screenshot_2015-01-20-11-48-34.png

Here's how it looks

Comment 7

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

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)

Comment 8

[Martyn Haigh (:mhaigh)]

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

Comment 9

[Martyn Haigh (:mhaigh)]

https://hg.mozilla.org/integration/fx-team/rev/e6d438f2b646

Comment 10

[Martyn Haigh (:mhaigh)]

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

https://hg.mozilla.org/integration/fx-team/rev/30a86cc4e277

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e6d438f2b646
https://hg.mozilla.org/mozilla-central/rev/30a86cc4e277

Comment 12

[u421692]

Verfied as fixed on latest Nightly on Nexus 7 (Android 5.0.1) and Sony Xperia Z2 SGP511 (Android 4.4.4)

Comment 13

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Don't forget to uplift!

Comment 14

[Martyn Haigh (:mhaigh)]

Attachment: Bug 1110149 - [Tablet] Insert vertical divider between back button and normal browsing button in Tabs Panel (r=mcomella)

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a6bc0837b6d3

Comment 16

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Any issues with uplifting to Beta here too?

Comment 17

[Martyn Haigh (: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.

Comment 18

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

(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?

Comment 19

[Martyn Haigh (:mhaigh)]

Attachment: Bug 1110149 - [Tablet] Insert vertical divider between back button and normal browsing button in Tabs Panel (r=mcomella)

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

Comment 20

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

(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?

Comment 21

[Martyn Haigh (:mhaigh)]

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

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f308adc4e97c

Comment 23

[Teodora Vermesan (:TeoVermesan)]

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

Comment 24

[Flaviu Cos]

Verified as fixed in build 37.0a2 2015-02-03;
Device: Asus Transformer Tab (Android 4.0.3)