1008730 - Customization palette doesn't obey High Contrast settings

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P3)

Description

[BJ]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140506152807

Steps to reproduce:

Open Windows 7 Display colour settings.
Select the Window object and change font colour to white and background to black.
Right click on the FF menu bar.
Click on Customize.


Actual results:

In the left hand page only icons are visible. Text associated with the icons is not visible.


Expected results:

Please see W3C WAI Accessibility Guideline F24 for a full description of the problem:
http://www.w3.org/TR/WCAG-TECHS/F24.html

Do not test with the Windows High Contrast themes. These themes are not usable in the wrold world since they also strip out graphics and navigation objects. They also, somehow, sometimes resolve this problem and change colours on the fly.

For a fuller test go through all Windows objects and change all font/background settings to white on black. To aid troubleshooting it can help to use unusual colours such as lime green text on a red background - such colours make it easier to see which colours have not been set in the user interface and which have defaulted to the OS colour settings.

It is also impossible to see the text on Tabs too.

Comment 1

[BJ]

Attachment: Screen shot showing absense of text in left pane

Uploaded screen shot

Comment 2

[BJ]

Attachment: Screen shot with test colours set

It has been suggested that because the background colours have been set that the text colour too should be set. This fits well with the accessibility guideline. But please see the second screen shot which is taken with test colours set as I suggested ie. lime green text on red.

It can be seen that the left hand pane has lime green text but not on a red background. This confirms that the text colour has not been set but the background has. However. The right hand pane shows lime green text on a red background. This too fits the guuideline but the approaches taken differ. Would not a consistent approach be preferable?

The complication is that different parts of the interrace are takng a different approach. The proposed solution would ensure text and background are set for the left hand pane. Yet the right hand pane is defaulting to OS settings. This results in one pane displaying as black on white, and the other white on black. A consisten approach would ensure both use the same which is preferable. Having large chunks of the screen in black and others in white can be a problem for the visually impaired.

You can also see other parts of the interface eg. tab text where either forefround or background have been set but not both. But note. I have nto set test  colours for all Windows objects, only for Window font and background.

I recommend setting the colur of text on tabs to ensure sufficient contrast with the hard coded background.

Comment 3

[BJ]

I've just done a little more testing. My suggestion to also set the colour of tab text is not quite so straight forward.

The backgroun colour of the tab bar seems to be taken from the Windows 3D object. If I am correct then to be consistent the tab text colour should be taken from the font colour setting for this object.

But the situation is complicated due to the tab bar background colouor being graduated ie. darker at the top than at the bottom. Doing this makes it impossible to ensure the text colour will have sufficient contrast to stand out from the background. It's best to avoid graduating colours for a basic theme designed to suit the majority of users. I think there is a WAI Guieline along these lines too.

Comment 5

[Dão Gottwald [:dao]]

This appears to have been fixed. The labels are visible now, but the palette just doesn't obey high contrast mode at all.

Comment 6

[Dão Gottwald [:dao]]

Bug 1297806 made this "worse" again in that the palette item icons now use a platform color (-moz-fieldtext); the rest of the palette needs to do follow suit.

Comment 7

[Dão Gottwald [:dao]]

Attachment: patch

Comment 8

[:Gijs (out for now; he/him)]

Comment on attachment 8791222 [details] [diff] [review]
patch

Review of attachment 8791222 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ +122,4 @@
>    padding: 10px;
>  }
>  
> +%if defined(XP_MACOSX) || defined(XP_WIN)

Can't we just do %ifndef XP_LINUX ?

@@ +387,5 @@
>  
>  .customization-lwtheme-menu-theme[active="true"],
>  .customization-lwtheme-menu-theme:hover {
> +  background-color: var(--arrowpanel-dimmed);
> +  border-color: var(--panel-separator-color);

It makes me uncomfortable that we're using this variable on things which are clearly not panel separators. Can we rename it to something more sensible? 'arrowpanel-dimmed' (further) also doesn't seem like it really covers what the lwtheme button's hover (or active) state is...

@@ +413,5 @@
>  #customization-lwtheme-menu-header,
>  #customization-lwtheme-menu-recommended,
>  #customization-lwtheme-menu-footer {
> +  background-color: var(--arrowpanel-dimmed);
> +  color: GrayText;

There doesn't seem to be any guarantee this combination works for the arrowpanel-dimmed values. It doesn't on the theme I'm using, as this renders light grey on slightly darker grey.

@@ +430,4 @@
>  }
>  
>  #customization-lwtheme-menu-footer {
> +  background: linear-gradient(var(--arrowpanel-dimmed) 60%, transparent) border-box;

This gradient looks like it's the wrong way around on dark themes.

Comment 9

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #8)
> > +%if defined(XP_MACOSX) || defined(XP_WIN)
> 
> Can't we just do %ifndef XP_LINUX ?

I could but I prefer to be explicit about which OSes I'm targeting here.

> >  .customization-lwtheme-menu-theme[active="true"],
> >  .customization-lwtheme-menu-theme:hover {
> > +  background-color: var(--arrowpanel-dimmed);
> > +  border-color: var(--panel-separator-color);
> 
> It makes me uncomfortable that we're using this variable on things which are
> clearly not panel separators. Can we rename it to something more sensible?

Suggestions welcome (in another bug though).

> 'arrowpanel-dimmed' (further) also doesn't seem like it really covers what
> the lwtheme button's hover (or active) state is...

Dimmed on hover, dimmed further on active makes sense to me and is consistent with how this is used elsewhere.

> >  #customization-lwtheme-menu-header,
> >  #customization-lwtheme-menu-recommended,
> >  #customization-lwtheme-menu-footer {
> > +  background-color: var(--arrowpanel-dimmed);
> > +  color: GrayText;
> 
> There doesn't seem to be any guarantee this combination works for the
> arrowpanel-dimmed values. It doesn't on the theme I'm using, as this renders
> light grey on slightly darker grey.

Have to use the default color then, I suppose.

> >  #customization-lwtheme-menu-footer {
> > +  background: linear-gradient(var(--arrowpanel-dimmed) 60%, transparent) border-box;
> 
> This gradient looks like it's the wrong way around on dark themes.

The gradient isn't supposed go any particular way except that it connects the footer with the panel arrow (which uses -moz-field).

Comment 10

[Dão Gottwald [:dao]]

Attachment: patch v2

Comment 11

[:Gijs (out for now; he/him)]

Comment on attachment 8791272 [details] [diff] [review]
patch v2

(In reply to Dão Gottwald [:dao] from comment #9)
> (In reply to :Gijs Kruitbosch from comment #8)
> > >  .customization-lwtheme-menu-theme[active="true"],
> > >  .customization-lwtheme-menu-theme:hover {
> > > +  background-color: var(--arrowpanel-dimmed);
> > > +  border-color: var(--panel-separator-color);
> > 
> > It makes me uncomfortable that we're using this variable on things which are
> > clearly not panel separators. Can we rename it to something more sensible?
> 
> Suggestions welcome

adaptive-separator-color ?

Maybe we should have a second/independent variable that just happens to have the same values.

> (in another bug though).

Why? This is, AIUI, where we're first using these outside of the main menupanel and/or its toolbarbuttons/items. I don't recall reviewing other patches where this happened. Did I miss a trick?

> > 'arrowpanel-dimmed' (further) also doesn't seem like it really covers what
> > the lwtheme button's hover (or active) state is...
> 
> Dimmed on hover, dimmed further on active makes sense to me and is
> consistent with how this is used elsewhere.

I thought this was the main button. The naming here is maybe not ideal. Anyway, looking at this again, it seems your patch also uses it for borders on sections that don't have hover/active feedback at all.

> > >  #customization-lwtheme-menu-footer {
> > > +  background: linear-gradient(var(--arrowpanel-dimmed) 60%, transparent) border-box;
> > 
> > This gradient looks like it's the wrong way around on dark themes.
> 
> The gradient isn't supposed go any particular way except that it connects
> the footer with the panel arrow (which uses -moz-field).

Maybe we should update the arrow, then?

Comment 12

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #11)
> Comment on attachment 8791272 [details] [diff] [review]
> patch v2
> 
> (In reply to Dão Gottwald [:dao] from comment #9)
> > (In reply to :Gijs Kruitbosch from comment #8)
> > > >  .customization-lwtheme-menu-theme[active="true"],
> > > >  .customization-lwtheme-menu-theme:hover {
> > > > +  background-color: var(--arrowpanel-dimmed);
> > > > +  border-color: var(--panel-separator-color);
> > > 
> > > It makes me uncomfortable that we're using this variable on things which are
> > > clearly not panel separators. Can we rename it to something more sensible?
> > 
> > Suggestions welcome
> 
> adaptive-separator-color ?
> 
> Maybe we should have a second/independent variable that just happens to have
> the same values.
> 
> > (in another bug though).
> 
> Why? This is, AIUI, where we're first using these outside of the main
> menupanel and/or its toolbarbuttons/items. I don't recall reviewing other
> patches where this happened. Did I miss a trick?

It's used in many panels.

https://dxr.mozilla.org/mozilla-central/search?q=%22--panel-separator-color%22&=mozilla-central

> > > 'arrowpanel-dimmed' (further) also doesn't seem like it really covers what
> > > the lwtheme button's hover (or active) state is...
> > 
> > Dimmed on hover, dimmed further on active makes sense to me and is
> > consistent with how this is used elsewhere.
> 
> I thought this was the main button. The naming here is maybe not ideal.
> Anyway, looking at this again, it seems your patch also uses it for borders
> on sections that don't have hover/active feedback at all.

I'd prefer using --panel-separator-color for those borders but it's darker. Using --arrowpanel-dimmed is more conservative in terms of staying true to the original design and still kind of works semantically (i.e. the border surface dims the background).

> > > >  #customization-lwtheme-menu-footer {
> > > > +  background: linear-gradient(var(--arrowpanel-dimmed) 60%, transparent) border-box;
> > > 
> > > This gradient looks like it's the wrong way around on dark themes.
> > 
> > The gradient isn't supposed go any particular way except that it connects
> > the footer with the panel arrow (which uses -moz-field).
> 
> Maybe we should update the arrow, then?

We can't update it, we could fork and customize it for this particular panel. It's a bit of an effort and implies a maintenance cost that I'm not sure is justified considering that this is secondary UI. This is also orthogonal to this bug.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fea76c9b016b
Make customization palette respect OS themes. r=gijs

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fea76c9b016b