Closed Bug 834525 Opened 7 years ago Closed 7 years ago

Update Tab thumbnail styling

Categories

(Firefox for Android :: Theme and Visual Design, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 21
Tracking Status
firefox20 --- verified
firefox21 --- verified

People

(Reporter: sriram, Assigned: sriram)

References

(Depends on 1 open bug)

Details

Attachments

(4 files)

With the new design changes to Tabs UI, only the thumbnails get highlighted and not the entire row.
Blocks: 817675
Attached patch PatchSplinter Review
(Did I just complete styling the tabs? Phew!)

This patch changes the styles for phones and tablets. Basically we used to switch between two different backgrounds earlier. But, if we treat it as a "Single choice list view", only one of them is going to be in "checked" state. The same principle is used here.

In post-honeycomb devices, a call to setItemChecked() will set a state of "state_activated". However, this breaks in gingerbread. Instead, a new TabRow is created that would implement Checkable behavior and set a state of "state_checked". Android's ListView will flip the value on calling setItemChecked(), and a new state change causes a new drawable.

Notes:
1. This needs new about:home thumbnails.
2. My hands are itching to move the ViewHolder to the TabRow as its a class by itself now!
Attachment #706177 - Flags: review?(mark.finkle)
Comment on attachment 706177 [details] [diff] [review]
Patch

>diff --git a/mobile/android/base/widget/TabRow.java b/mobile/android/base/widget/TabRow.java

>+    public void setChecked(boolean checked) {
>+        android.util.Log.i("Sriram", "setting the state to: " + checked);

Remove this Log

This code removes the "close" button. Does this create an accessibility issue?
Attachment #706177 - Flags: review?(mark.finkle) → review+
(In reply to Mark Finkle (:mfinkle) from comment #2)
> This code removes the "close" button. Does this create an accessibility
> issue?

I'm not sure. I was told that we would have a long press on these items to show a close button.
Flags: needinfo?(ibarlow)
Comment on attachment 706177 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New UI.
User impact if declined: Old bland styling.
Testing completed (on m-c, etc.): Yet to be in m-c (Being a little proactive ;) ).
Risk to taking this patch (and alternatives if risky): Very low. This just changes the styling.
String or UUID changes made by this patch: None.
Attachment #706177 - Flags: approval-mozilla-aurora?
Depends on: 834902
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c9bba628aeb3 - robocop was unhappy in testNewTab
Attached patch Patch: TestsSplinter Review
This suppresses the tests for now.
Attachment #708229 - Flags: review?(mark.finkle)
Somehow I had missed the "choiceMode" for private tabs. That's fixed in this patch.
Also, there was a focusing issue. When the selected tab is a private tab, and we navigate to "normal tabs", one of the normal tabs is always focused. Since the distinction between focused and selected tabs is very small, we had to remove this focus. Unfortunately "android:focusableInTouchMode" didn't work with the view. Hence, force "un-checking" every row other than selected row. (This won't cause a performance regression).
Attachment #708244 - Flags: review?(mark.finkle)
Comment on attachment 708229 [details] [diff] [review]
Patch: Tests

Make sure we fix up this test when we add back the long-press Close menu
Attachment #708229 - Flags: review?(mark.finkle) → review+
Attachment #708244 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/59b689e1d999
https://hg.mozilla.org/mozilla-central/rev/7697e0d73d95
https://hg.mozilla.org/mozilla-central/rev/fc9b2b7ebfbd
Assignee: nobody → sriram
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Comment on attachment 708229 [details] [diff] [review]
Patch: Tests

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New UI.
User impact if declined: This is not user facing. Try server will fail as we removed the close button.
Testing completed (on m-c, etc.): Landed in m-c on 01/31
Risk to taking this patch (and alternatives if risky): Very low. This suppresses a test for close tabs. The supported will be added back later.
String or UUID changes made by this patch: None.
Attachment #708229 - Flags: approval-mozilla-aurora?
Comment on attachment 708244 [details] [diff] [review]
Patch: Focus issue

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New UI.
User impact if declined: If a private tab is a selected tab, the normal tabs will have on row that's focused by default.
Testing completed (on m-c, etc.): Landed in m-c on 01/31
Risk to taking this patch (and alternatives if risky): Very low. This enforces it not have a focused state set.
String or UUID changes made by this patch: None.
Attachment #708244 - Flags: approval-mozilla-aurora?
Attachment #706177 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #708229 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #708244 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
The first time I pushed the patch, I removed the log. When it was backed out and pushed again, I forgot to remove it. :(
Attachment #709811 - Flags: review?(mark.finkle)
Comment on attachment 709811 [details] [diff] [review]
Patch: Remove log

O_o
Attachment #709811 - Flags: review?(mark.finkle) → review+
Comment on attachment 709811 [details] [diff] [review]
Patch: Remove log

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Poor code checking :(
User impact if declined: None. They won't even notice this.
Testing completed (on m-c, etc.): Landed in m-i on 02/04. No major testing needed.
Risk to taking this patch (and alternatives if risky): None.
String or UUID changes made by this patch: None.
Attachment #709811 - Flags: approval-mozilla-aurora?
Attachment #709811 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
We re-added the close button in bug 837113.
Flags: needinfo?(ibarlow)
Verified fixed on:
-build: Firefox for Android 20.0a2 (2013-02-18), Firefox for Android 21.0a1 (2013-02-18)
-device: Samsung Galaxy Nexus
-OS: Android 4.2.1
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.