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

VERIFIED FIXED in Firefox 31

Status

()

VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: lucasr, Assigned: mcomella)

Tracking

unspecified
Firefox 32
All
Android
Points:
---

Firefox Tracking Flags

(firefox31 verified, firefox32 verified, fennec31+)

Details

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 8433309 [details]
Screenshot_2014-06-03-14-13-03.png

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!
(Reporter)

Updated

5 years ago
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.
(Reporter)

Comment 5

5 years ago
Mike, can you have a look?
Assignee: nobody → michael.l.comella
Created attachment 8434361 [details]
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)
Created attachment 8434366 [details]
Dark theme, light X tablet
Created attachment 8434374 [details] [diff] [review]
Use light cancel edit button with dark lightweight themes.

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+
Created attachment 8434450 [details] [diff] [review]
Use light cancel edit button with dark lightweight themes.

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?
Created attachment 8434507 [details] [diff] [review]
Use light cancel edit button with dark lightweight themes.

Updated as per comment 13.
Attachment #8434507 - Flags: review?(bnicholson)
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+
Created attachment 8435175 [details] [diff] [review]
Use light cancel edit button with dark lightweight themes.

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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
status-firefox31: --- → affected
status-firefox32: --- → fixed
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)
status-firefox32: fixed → verified
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
Created attachment 8435941 [details] [diff] [review]
(aurora) Use light cancel edit button with dark lightweight themes.

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+
For aurora.
Keywords: branch-patch-needed → checkin-needed
https://hg.mozilla.org/releases/mozilla-aurora/rev/d9edba5338ff
status-firefox31: affected → fixed
Keywords: checkin-needed
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).
status-firefox31: fixed → verified
(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.