Closed Bug 1019595 Opened 5 years ago Closed 5 years ago

Lack of contrast on editing mode's 'X' button when using a lightweight theme

Categories

(Firefox for Android :: Awesomescreen, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 32
Tracking Status
firefox31 --- verified
firefox32 --- verified
fennec 31+ ---

People

(Reporter: lucasr, Assigned: mcomella)

References

Details

Attachments

(5 files, 3 obsolete files)

We need some way to ensure enough contrast between the toolbar's background and the the 'X' button. See screenshot.
Saw this in April, said in IRC "I meant to file a bug", then didn't :D  Fail on my part!
tracking-fennec: --- → ?
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
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.
Whoa. I'm starting to think Finkle and Bugzilla are the same thing.
Mike, can you have a look?
Assignee: nobody → michael.l.comella
Attached image Dark theme, light X
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)
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)
Note to self - uplift to 31 to match the edit cancel button.
Status: NEW → ASSIGNED
Comment on attachment 8434374 [details] [diff] [review]
Use light cancel edit button with dark lightweight themes.

Missing close_edit_mode_selector.xml ?
(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.
Attachment #8434361 - Flags: feedback?(ibarlow) → feedback+
Added close_edit_mode_selector as per comment 10, and spare Lucas another review.
Attachment #8434450 - Flags: review?(bnicholson)
Attachment #8434374 - Attachment is obsolete: true
Attachment #8434374 - Flags: review?(lucasr.at.mozilla)
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?
Attachment #8434450 - Attachment is obsolete: true
Attachment #8434450 - Flags: review?(bnicholson)
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+
tracking-fennec: ? → 31+
Updated for nits (this is the one pushed to fx-team).
Attachment #8434507 - Attachment is obsolete: true
Attachment #8435175 - Flags: review+
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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Attachment #8435175 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed in build 32.0a1 (2014-06-06)
Device: Asus Transformer TF101 (Android 4.0.3)
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)
The cancel button isn't available on tablet so I added some guards; bnich, double-check me please!
Attachment #8435941 - Flags: review?(bnicholson)
Attachment #8435175 - Attachment is obsolete: true
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)
Attachment #8435941 - Flags: review?(bnicholson) → review+
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
Status: RESOLVED → VERIFIED
Verified as fixed in:
Build: Firefox for Android 31 Beta 4
Device: Asus Transformer Pad TF300T (Android 4.2.1).
(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.
You need to log in before you can comment on or make changes to this bug.