Closed
Bug 834525
Opened 11 years ago
Closed 11 years ago
Update Tab thumbnail styling
Categories
(Firefox for Android Graveyard :: Theme and Visual Design, defect)
Tracking
(firefox20 verified, firefox21 verified)
VERIFIED
FIXED
Firefox 21
People
(Reporter: sriram, Assigned: sriram)
References
Details
Attachments
(4 files)
40.51 KB,
patch
|
mfinkle
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.98 KB,
patch
|
mfinkle
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
2.53 KB,
patch
|
mfinkle
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1009 bytes,
patch
|
mfinkle
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
With the new design changes to Tabs UI, only the thumbnails get highlighted and not the entire row.
Assignee | ||
Comment 1•11 years ago
|
||
(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 2•11 years ago
|
||
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+
Assignee | ||
Comment 3•11 years ago
|
||
(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)
Assignee | ||
Comment 4•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/baa055cb9043
Assignee | ||
Comment 5•11 years ago
|
||
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?
Comment 6•11 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c9bba628aeb3 - robocop was unhappy in testNewTab
Assignee | ||
Comment 7•11 years ago
|
||
This suppresses the tests for now.
Attachment #708229 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 8•11 years ago
|
||
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 9•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #708244 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 10•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/59b689e1d999 https://hg.mozilla.org/integration/mozilla-inbound/rev/7697e0d73d95 https://hg.mozilla.org/integration/mozilla-inbound/rev/fc9b2b7ebfbd
Comment 11•11 years ago
|
||
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: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Assignee | ||
Comment 12•11 years ago
|
||
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?
Assignee | ||
Comment 13•11 years ago
|
||
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?
Updated•11 years ago
|
Attachment #706177 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 years ago
|
Attachment #708229 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 years ago
|
Attachment #708244 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 14•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/4c2ef2454be2 https://hg.mozilla.org/releases/mozilla-aurora/rev/78d15d0bdb0d https://hg.mozilla.org/releases/mozilla-aurora/rev/52c22881fcc4
Updated•11 years ago
|
status-firefox20:
--- → fixed
status-firefox21:
--- → fixed
Assignee | ||
Comment 15•11 years ago
|
||
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 16•11 years ago
|
||
Comment on attachment 709811 [details] [diff] [review] Patch: Remove log O_o
Attachment #709811 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 17•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/674484570fd5
Assignee | ||
Comment 18•11 years ago
|
||
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?
Updated•11 years ago
|
Attachment #709811 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 19•11 years ago
|
||
In Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/4c0a3876f717
Assignee | ||
Comment 21•11 years ago
|
||
We re-added the close button in bug 837113.
Flags: needinfo?(ibarlow)
Comment 22•11 years ago
|
||
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
Updated•11 years ago
|
Updated•3 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
•