Closed Bug 1110149 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox for Android Graveyard :: General, enhancement)

All
Android
enhancement
Not set
normal

Tracking

(firefox36 verified, firefox37 verified, firefox38 verified)

VERIFIED FIXED
Firefox 38
Tracking Status
firefox36 --- verified
firefox37 --- verified
firefox38 --- verified

People

(Reporter: u421692, Assigned: mhaigh)

References

Details

Attachments

(6 files)

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