Closed
Bug 1019595
Opened 9 years ago
Closed 9 years ago
Lack of contrast on editing mode's 'X' button when using a lightweight theme
Categories
(Firefox for Android Graveyard :: Awesomescreen, defect)
Tracking
(firefox31 verified, firefox32 verified, fennec31+)
VERIFIED
FIXED
Firefox 32
People
(Reporter: lucasr, Assigned: mcomella)
References
Details
Attachments
(5 files, 3 obsolete files)
119.10 KB,
image/png
|
Details | |
114.23 KB,
image/png
|
ibarlow
:
feedback+
|
Details |
278.15 KB,
image/png
|
Details | |
10.73 KB,
patch
|
mcomella
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
9.31 KB,
patch
|
bnicholson
:
review+
|
Details | Diff | Splinter Review |
We need some way to ensure enough contrast between the toolbar's background and the the 'X' button. See screenshot.
Comment 1•9 years ago
|
||
Saw this in April, said in IRC "I meant to file a bug", then didn't :D Fail on my part!
Reporter | ||
Updated•9 years ago
|
tracking-fennec: --- → ?
Comment 2•9 years ago
|
||
Looking at Desktop, they seem to be able to detect whether a theme is light or dark, and actually reverses the icon colours when the background is dark. http://cl.ly/image/3Z372B2Z140g
Comment 3•9 years ago
|
||
Turns out, this has all happened before: bug 816114, bug 822421 and bug 822422. Bug 822422 deals with flipping resources (images) based on the theme.
Comment 4•9 years ago
|
||
Whoa. I'm starting to think Finkle and Bugzilla are the same thing.
Assignee | ||
Comment 6•9 years ago
|
||
I converted the X button to #FFFFFF - what do you think, Ian? I suppose this can still be a problem on both light and dark backgrounds though.
Attachment #8434361 -
Flags: feedback?(ibarlow)
Assignee | ||
Comment 7•9 years ago
|
||
Assignee | ||
Comment 8•9 years ago
|
||
I'm not sure if the white assests should be called "light" for their color, or "dark" for the themes they're expected to be used on.
Attachment #8434374 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 9•9 years ago
|
||
Note to self - uplift to 31 to match the edit cancel button.
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Comment 10•9 years ago
|
||
Comment on attachment 8434374 [details] [diff] [review] Use light cancel edit button with dark lightweight themes. Missing close_edit_mode_selector.xml ?
Comment 11•9 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #6) > Created attachment 8434361 [details] > Dark theme, light X > > I converted the X button to #FFFFFF - what do you think, Ian? This looks good > I suppose this can still be a problem on both light and dark backgrounds > though. No solution will be perfect here. And I don't want to get into using crazy icon borders or drop shadows to compensate for this.
Updated•9 years ago
|
Attachment #8434361 -
Flags: feedback?(ibarlow) → feedback+
Assignee | ||
Comment 12•9 years ago
|
||
Added close_edit_mode_selector as per comment 10, and spare Lucas another review.
Attachment #8434450 -
Flags: review?(bnicholson)
Assignee | ||
Updated•9 years ago
|
Attachment #8434374 -
Attachment is obsolete: true
Attachment #8434374 -
Flags: review?(lucasr.at.mozilla)
Comment 13•9 years ago
|
||
Comment on attachment 8434450 [details] [diff] [review] Use light cancel edit button with dark lightweight themes. >diff --git a/mobile/android/base/resources/drawable/close_edit_mode_selector.xml b/mobile/android/base/resources/drawable/close_edit_mode_selector.xml >+ <item gecko:state_dark="true" >+ android:drawable="@drawable/close_edit_mode_light"/> >+ >+ <item android:drawable="@drawable/close_edit_mode"/> Can we rename the close_edit_mode -> close_edit_mode_light Can we rename the close_edit_mode_light ->close_edit_mode_dark so the suffix matches the state?
Assignee | ||
Comment 14•9 years ago
|
||
Updated as per comment 13.
Attachment #8434507 -
Flags: review?(bnicholson)
Assignee | ||
Updated•9 years ago
|
Attachment #8434450 -
Attachment is obsolete: true
Attachment #8434450 -
Flags: review?(bnicholson)
Comment 15•9 years ago
|
||
Comment on attachment 8434507 [details] [diff] [review] Use light cancel edit button with dark lightweight themes. Review of attachment 8434507 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. ::: mobile/android/base/toolbar/BrowserToolbar.java @@ +1453,5 @@ > StateListDrawable stateList = new StateListDrawable(); > stateList.addState(PRIVATE_STATE_SET, getColorDrawable(R.color.background_private)); > stateList.addState(EMPTY_STATE_SET, drawable); > > + editCancel.onLightweightThemeChanged(); Nit: put this under `setBackgroundDrawable(stateList);` to keep the stateList code together.
Attachment #8434507 -
Flags: review?(bnicholson) → review+
Updated•9 years ago
|
tracking-fennec: ? → 31+
Assignee | ||
Comment 16•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/82cdfd1b3914
Assignee | ||
Comment 17•9 years ago
|
||
Updated for nits (this is the one pushed to fx-team).
Attachment #8434507 -
Attachment is obsolete: true
Attachment #8435175 -
Flags: review+
Assignee | ||
Comment 18•9 years ago
|
||
Comment on attachment 8435175 [details] [diff] [review] Use light cancel edit button with dark lightweight themes. [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 965548 User impact if declined: Users with dark lightweight themes will not be able to see the cancel editing mode button clearly Testing completed (on m-c, etc.): Local Risk to taking this patch (and alternatives if risky): Low - we've essentially just casted some views so in the worst (and unlikely) case, these casts fail, throw an exception, and the browser dies (but it's an easy fix). String or IDL/UUID changes made by this patch: None
Attachment #8435175 -
Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/82cdfd1b3914
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Updated•9 years ago
|
status-firefox31:
--- → affected
status-firefox32:
--- → fixed
Updated•9 years ago
|
Attachment #8435175 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 20•9 years ago
|
||
Verified as fixed in build 32.0a1 (2014-06-06) Device: Asus Transformer TF101 (Android 4.0.3)
Comment 21•9 years ago
|
||
Needs a branch patch for Aurora uplift. Note that the merge to beta is on Monday, so this needs to land ASAP.
Flags: needinfo?(michael.l.comella)
Keywords: branch-patch-needed
Assignee | ||
Comment 22•9 years ago
|
||
The cancel button isn't available on tablet so I added some guards; bnich, double-check me please!
Attachment #8435941 -
Flags: review?(bnicholson)
Assignee | ||
Updated•9 years ago
|
Attachment #8435175 -
Attachment is obsolete: true
Assignee | ||
Comment 23•9 years ago
|
||
Comment on attachment 8435175 [details] [diff] [review] Use light cancel edit button with dark lightweight themes. bzexport made me do it.
Attachment #8435175 -
Attachment is obsolete: false
Flags: needinfo?(michael.l.comella)
Updated•9 years ago
|
Attachment #8435941 -
Flags: review?(bnicholson) → review+
Comment 25•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/d9edba5338ff
Keywords: checkin-needed
Comment 26•9 years ago
|
||
Verified as fixed in 31.0a2 (2014-06-09) on phones. On tablets (aurora), the cancel edit mode isn't available, can we consider this verified and fixed?
Depends on: 997477
Updated•9 years ago
|
Status: RESOLVED → VERIFIED
Comment 27•9 years ago
|
||
Verified as fixed in: Build: Firefox for Android 31 Beta 4 Device: Asus Transformer Pad TF300T (Android 4.2.1).
Comment 28•9 years ago
|
||
(In reply to cristina.madaras from comment #27) > Verified as fixed in: > Build: Firefox for Android 31 Beta 4 > Device: Asus Transformer Pad TF300T (Android 4.2.1). Sorry! My mistake, I tested this on Motorola Razr (Android 4.0.4), not on Asus. On tablets (Beta), the cancel edit mode isn't available, Bug 997477.
Updated•2 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
•