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)
Tracking
(firefox36 verified, firefox37 verified, firefox38 verified)
VERIFIED
FIXED
Firefox 38
People
(Reporter: u421692, Assigned: mhaigh)
References
Details
Attachments
(6 files)
397.62 KB,
image/png
|
Details | |
6.33 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
37.37 KB,
image/png
|
Details | |
7.84 KB,
patch
|
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
7.85 KB,
patch
|
Details | Diff | Splinter Review | |
7.85 KB,
patch
|
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
Comment 4•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: nobody → mhaigh
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8551736 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 6•10 years ago
|
||
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+
Assignee | ||
Comment 8•10 years ago
|
||
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
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Comment 10•10 years ago
|
||
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: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Reporter | ||
Comment 12•10 years ago
|
||
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)
Assignee | ||
Comment 14•10 years ago
|
||
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?
Updated•10 years ago
|
status-firefox37:
--- → affected
status-firefox38:
--- → fixed
Updated•10 years ago
|
Attachment #8553795 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 15•10 years ago
|
||
Any issues with uplifting to Beta here too?
Flags: needinfo?(mhaigh)
Assignee | ||
Comment 17•10 years ago
|
||
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)
Assignee | ||
Comment 19•10 years ago
|
||
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)
Assignee | ||
Comment 21•10 years ago
|
||
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?
Updated•10 years ago
|
status-firefox36:
--- → affected
Updated•10 years ago
|
Attachment #8557985 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 22•10 years ago
|
||
Comment 23•10 years ago
|
||
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•10 years ago
|
||
Verified as fixed in build 37.0a2 2015-02-03;
Device: Asus Transformer Tab (Android 4.0.3)
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•