Add back button to top left of the tabs panel header for new tablet

VERIFIED FIXED in Firefox 36

Status

()

P1
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: mhaigh, Assigned: mhaigh)

Tracking

unspecified
Firefox 37
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 verified, firefox37 verified)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Comment hidden (empty)

Updated

4 years ago
Priority: -- → P1
Anthony, any mockups?
Flags: needinfo?(alam)
Created attachment 8524691 [details]
prev_tabtray_vert_mock5.png

Attaching preview. 

Assets to follow.
Created attachment 8524701 [details]
icon_navback.zip

Icons!
Flags: needinfo?(alam)
Might be worth mentioning..

Totally my fault but I hadn't noticed that we didn't resize our buttons on top to use the purple and yellow specs here: https://bugzilla.mozilla.org/attachment.cgi?id=8508946&action=edit

So it might look different from my mock (since my mock does use that sizing). I'll file a follow up bug to see if we can adjust the tabs tray button sizes I mentioned above.
(Assignee)

Comment 5

4 years ago
Created attachment 8529069 [details] [diff] [review]
Add back button to top left of the tabs panel header for new tablet
Attachment #8529069 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8529069 [details] [diff] [review]
Add back button to top left of the tabs panel header for new tablet

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

Looks good overall. I thought we only want the back button in the new tablet UI? This patch will add the back button everywhere, unless I'm missing something.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +295,4 @@
>  <!ENTITY page "Page">
>  <!ENTITY tools "Tools">
>  <!ENTITY new_tab "New Tab">
> +<!ENTITY nav_back "Back">

Don't we have the "Back" string already?
Attachment #8529069 - Flags: review?(lucasr.at.mozilla) → feedback+
(Assignee)

Comment 7

4 years ago
Created attachment 8529737 [details] [diff] [review]
Add back button to top left of the tabs panel  header for new tablet

(In reply to Lucas Rocha (:lucasr) from comment #6)

> 
> Looks good overall. I thought we only want the back button in the new tablet
> UI? This patch will add the back button everywhere, unless I'm missing
> something.

Ah yes - the layout.xml file caught me out (again!)
Attachment #8529069 - Attachment is obsolete: true
Attachment #8529737 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8529737 [details] [diff] [review]
Add back button to top left of the tabs panel  header for new tablet

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

Yeah, stubbing is the way to go here. This needs some changes:
1. Move new_tablet_back_button (rename it to new_tablet_tabs_panel_back_button for clarity) to mobile/android/base/newtablet/res/layout-large-v11 and add a matching entry for this new in mobile/android/base/resources/values/layout.xml (take new_tablet_tab_strip.xml as an example)
2. The arrow is vertically misaligned with the icons in IconTabWidget and has difference width. Make sure it has same width and is vertically centered.
Attachment #8529737 - Flags: review?(lucasr.at.mozilla) → feedback+
(Assignee)

Comment 9

4 years ago
Created attachment 8530261 [details] [diff] [review]
Add back button to top left of the tabs panel header for new tablet
Attachment #8529737 - Attachment is obsolete: true
Attachment #8530261 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8530261 [details] [diff] [review]
Add back button to top left of the tabs panel header for new tablet

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

Awesome.

::: mobile/android/base/tabs/TabsPanel.java
@@ +167,5 @@
>          });
> +
> +        if(NewTabletUI.isEnabled(getContext())) {
> +            ViewStub backButtonStub = (ViewStub) findViewById(R.id.nav_back_stub);
> +            mNavBackButton = (ImageButton) backButtonStub.inflate( );

nit: remove space between ()'s
Attachment #8530261 - Flags: review?(lucasr.at.mozilla) → review+
Drive-by: Martyn, did you run trimage on these new assets?
Flags: needinfo?(mhaigh)
I ran trimage on the images, just in case.

https://hg.mozilla.org/integration/fx-team/rev/383988745eb2
Comment on attachment 8530261 [details] [diff] [review]
Add back button to top left of the tabs panel header for new tablet

Approval Request Comment
[Feature/regressing bug #]: New tablet UI (bug 1014156)
[User impact if declined]: Key part of the new interaction with the tabs panel. We need this in 36.
[Describe test coverage new/current, TBPL]: local tests only, looks fine.
[Risks and why]: Low, simply adds a back button to the tabs panel when the new tablet UI is active. Let's bake it in Nightly for a couple days and then uplift it.
[String/UUID change made/needed]: n/a
Attachment #8530261 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/383988745eb2
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37

Comment 15

4 years ago
Verified as fixed on latest Nightly on Nexus 7 (Android 5.0.1) and Asus Transformer Pad TF300T (Android 4.2.1)
Status: RESOLVED → VERIFIED
(Assignee)

Updated

4 years ago
Flags: needinfo?(mhaigh)
status-firefox36: --- → affected
status-firefox37: --- → fixed
Attachment #8530261 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1087219

Comment 18

4 years ago
Verified as fixed on latest Aurora on Nexus 7 (Android 5.0.1)
status-firefox36: fixed → verified
status-firefox37: fixed → verified
Target Milestone: Firefox 37 → Firefox 36
Target Milestone: Firefox 36 → Firefox 37
You need to log in before you can comment on or make changes to this bug.