Customization palette doesn't obey High Contrast settings

RESOLVED FIXED in Firefox 51

Status

()

defect
P3
normal
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: bjl, Assigned: dao)

Tracking

(Blocks 2 bugs, {access})

Trunk
Firefox 51
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
Uploaded screen shot
Status: UNCONFIRMED → NEW
Component: Untriaged → Theme
Ever confirmed: true
Flags: firefox-backlog+
OS: Windows 7 → All
Hardware: x86_64 → All
Summary: Text not visible on new toobar customisation when using white text on a black background colour settings → Customization toolbox should hardcode text color where the background is hardcoded
Posted image Screen shot with test colours set (obsolete) —
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.
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.
Summary: Customization toolbox should hardcode text color where the background is hardcoded → Customization palette is unusable when using a High Contrast theme
Whiteboard: p=3
Points: --- → 3
Whiteboard: p=3
Duplicate of this bug: 1243389
Priority: -- → P3
Summary: Customization palette is unusable when using a High Contrast theme → Button labels are invisible in the customization palette when using a High Contrast theme
This appears to have been fixed. The labels are visible now, but the palette just doesn't obey high contrast mode at all.
Priority: P3 → P4
Summary: Button labels are invisible in the customization palette when using a High Contrast theme → Customization palette doesn't obey High Contrast settings
Attachment #8420669 - Attachment is obsolete: true
Attachment #8420870 - Attachment is obsolete: true
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.
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Priority: P4 → P3
Version: 29 Branch → Trunk
Posted patch patch (obsolete) — Splinter Review
Attachment #8791222 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8791222 - Flags: review?(gijskruitbosch+bugs) → review-
(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).
Posted patch patch v2Splinter Review
Attachment #8791222 - Attachment is obsolete: true
Attachment #8791272 - Flags: review?(gijskruitbosch+bugs)
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?
Attachment #8791272 - Flags: review?(gijskruitbosch+bugs)
(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.
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fea76c9b016b
Make customization palette respect OS themes. r=gijs
https://hg.mozilla.org/mozilla-central/rev/fea76c9b016b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Depends on: 1303330
You need to log in before you can comment on or make changes to this bug.