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?

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

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)

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

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

Don't forget to uplift!

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

Any issues with uplifting to Beta here too?

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.

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

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

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

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

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)