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

Status: VERIFIED FIXED

Description

[Lucas Rocha (:lucasr)]

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.

Comment 1

[Richard Newman [:rnewman]]

Saw this in April, said in IRC "I meant to file a bug", then didn't :D  Fail on my part!

Comment 2

[Ian Barlow (:ibarlow)]

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

[Mark Finkle (:mfinkle) (use needinfo?)]

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

[Ian Barlow (:ibarlow)]

Whoa. I'm starting to think Finkle and Bugzilla are the same thing.

Comment 5

[Lucas Rocha (:lucasr)]

Mike, can you have a look?

Comment 6

[Michael Comella (:mcomella) [needinfo or I won't see it]]

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.

Comment 7

[Michael Comella (:mcomella) [needinfo or I won't see it]]

Created attachment 8434366 [details]
Dark theme, light X tablet

Comment 8

[Michael Comella (:mcomella) [needinfo or I won't see it]]

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.

Comment 9

[Michael Comella (:mcomella) [needinfo or I won't see it]]

Note to self - uplift to 31 to match the edit cancel button.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8434374 [details] [diff] [review]
Use light cancel edit button with dark lightweight themes.

Missing close_edit_mode_selector.xml ?

Comment 11

[Ian Barlow (:ibarlow)]

(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.

Comment 12

[Michael Comella (:mcomella) [needinfo or I won't see it]]

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.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

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?

Comment 14

[Michael Comella (:mcomella) [needinfo or I won't see it]]

Created attachment 8434507 [details] [diff] [review]
Use light cancel edit button with dark lightweight themes.

Updated as per comment 13.

Comment 15

[Brian Nicholson (: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.

Comment 16

[Michael Comella (:mcomella) [needinfo or I won't see it]]

https://hg.mozilla.org/integration/fx-team/rev/82cdfd1b3914

Comment 17

[Michael Comella (:mcomella) [needinfo or I won't see it]]

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).

Comment 18

[Michael Comella (:mcomella) [needinfo or I won't see it]]

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

Comment 19

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/82cdfd1b3914

Comment 20

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in build 32.0a1 (2014-06-06)
Device: Asus Transformer TF101 (Android 4.0.3)

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Needs a branch patch for Aurora uplift. Note that the merge to beta is on Monday, so this needs to land ASAP.

Comment 22

[Michael Comella (:mcomella) [needinfo or I won't see it]]

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!

Comment 23

[Michael Comella (:mcomella) [needinfo or I won't see it]]

Comment on attachment 8435175 [details] [diff] [review]
Use light cancel edit button with dark lightweight themes.

bzexport made me do it.

Comment 24

[Michael Comella (:mcomella) [needinfo or I won't see it]]

For aurora.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d9edba5338ff

Comment 26

[Cristina Madaras, QA [:CristinaM]]

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?

Comment 27

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in:
Build: Firefox for Android 31 Beta 4
Device: Asus Transformer Pad TF300T (Android 4.2.1).

Comment 28

[Cristina Madaras, QA [:CristinaM]]

(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.