[e10s] select popups on Linux lack sufficient contrast

NEW
Unassigned

Status

()

Core
Layout: Form Controls
5 months ago
10 days ago

People

(Reporter: jaws, Unassigned)

Tracking

(Blocks: 1 bug, {regression})

unspecified
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54- disabled)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

Since bug 910022 and its follow-ups have landed, we are lacking sufficient contrast in the popups on Linux.

Bug 910022 implemented the ability for pages to style the <select> popup. The style is calculated by getting the computed styles of the <select> and its children. The styles are then forwarded to the parent process and used on the <menupopup> and its  children, respectively. We also make an attempt to calculate the default color values for these elements and only apply the colors if the computed value differs from the default colors.

This approach appears to work fine for Windows and OSX, however on some GTK themes (maybe all?), this approach doesn't work. For example, the <select> button on Ubuntu 16.10 has a light grey background with dark grey text. But the <select> popup by default has a dark grey background with a light grey text. Our computation of the default <select> background returns the light grey background, likely because it is using the button's colors instead of the popup's colors. This button's colors are forwarded to the parent process and applied to the <menupopup>. The <option> colors that are computed match the default user agent styling and are not applied to the menuitem's in the parent process, but the parent process has a default styling of using color:MenuText which is light by default in these same themes.

This ends up giving the result of a light grey background for the popup and a light grey text (just slightly darker than the background).

Dao, do you have any ideas what we can do here? You can see this code in /toolkit/modules/SelectContentHelper.jsm (look for _calculateUAColors) and /toolkit/modules/SelectParentHelper.jsm (look for multiple uses of sheet.insertRule).
Flags: needinfo?(dao+bmo)

Updated

5 months ago
Flags: needinfo?(dao+bmo)

Comment 1

5 months ago
I had been wondering what broke select popups. Interestingly they appear to be working again in the current nightly. Has this feature been disabled on Linux?

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #0)
> This approach appears to work fine for Windows and OSX,

Have you tested High Contrast and Win7 / Classic themes too?

> Our computation of the default <select> background returns the light
> grey background, likely because it is using the button's colors instead of
> the popup's colors.

I hope you're not actually using the button to get that color, because it seems obvious that this couldn't possibly do the right thing when the button and the popup don't use matching colors.

Assuming you're trying to get the background color from the menupopup rather than from the button, what I think is happening is that you don't get the actual color being rendered, because that comes from -moz-appearance. toolkit/themes/linux/global/popup.css does not even set a background-color.

Can you use only the text color? I.e. assume a dark background for light text and vice versa, and maybe fall back on background-color when the text color lightness is in some middle range.
Flags: needinfo?(jaws)
(In reply to Dão Gottwald [:dao] from comment #1)
> I had been wondering what broke select popups. Interestingly they appear to
> be working again in the current nightly. Has this feature been disabled on
> Linux?

No the feature wasn't disabled. Bug 1338219 just landed in time for today's Nightly so that's why it looks to be fixed.

> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #0)
> > This approach appears to work fine for Windows and OSX,
> 
> Have you tested High Contrast and Win7 / Classic themes too?

I just tested with all of the High Contrast options on Windows 10 and it works fine there.

> > Our computation of the default <select> background returns the light
> > grey background, likely because it is using the button's colors instead of
> > the popup's colors.
> 
> I hope you're not actually using the button to get that color, because it
> seems obvious that this couldn't possibly do the right thing when the button
> and the popup don't use matching colors.

We are creating a <select> element and calling getComputedStyle on it. GTK seems to be the only platform that uses different colors for the button vs the popup. 

> Assuming you're trying to get the background color from the menupopup rather
> than from the button, what I think is happening is that you don't get the
> actual color being rendered, because that comes from -moz-appearance.
> toolkit/themes/linux/global/popup.css does not even set a background-color.

Yeah, this is problematic. getComputedStyle gets us the various CSS colors used at different stages, but not the composited color that is actually displayed on screen. We would have to use something like -moz-element combined with context2d.drawWindow to get the colors of the individual pixels.

> Can you use only the text color? I.e. assume a dark background for light
> text and vice versa, and maybe fall back on background-color when the text
> color lightness is in some middle range.

I don't know how we could use text color to infer background-color, though this route does seem like it would be the most durable. getComputedStyle for color seems to always return a usable color, though if it returned 'transparent' then we would be SOL. Using just text color to infer background-color means that we might end up using background-colors that don't match the platform styling. Perhaps I'm not thinking of it the way you are though, does the code at http://searchfox.org/mozilla-central/source/toolkit/modules/SelectContentHelper.jsm#139-154 seem reasonable to you?
Flags: needinfo?(jaws) → needinfo?(dao+bmo)

Comment 3

4 months ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> (In reply to Dão Gottwald [:dao] from comment #1)
> > I had been wondering what broke select popups. Interestingly they appear to
> > be working again in the current nightly. Has this feature been disabled on
> > Linux?
> 
> No the feature wasn't disabled. Bug 1338219 just landed in time for today's
> Nightly so that's why it looks to be fixed.

I take that back, it's broken again. Requesting tracking because we can't ship like this.

> > Can you use only the text color? I.e. assume a dark background for light
> > text and vice versa, and maybe fall back on background-color when the text
> > color lightness is in some middle range.
> 
> I don't know how we could use text color to infer background-color, though
> this route does seem like it would be the most durable.

You can try to infer whether the background is light or dark. If you need the actual color, this doesn't work.

> getComputedStyle for
> color seems to always return a usable color, though if it returned
> 'transparent' then we would be SOL.

It should never be transparent.

> does the code at
> http://searchfox.org/mozilla-central/source/toolkit/modules/
> SelectContentHelper.jsm#139-154 seem reasonable to you?

It assumes that the popup uses certain CSS platform colors which is just not true. (Also, if you're just interested in what -moz-comboboxtext, -moz-combobox, -moz-fieldtext, -moz-field compute to, you might as well use <span> instead of <select> and <option>.)
status-firefox54: --- → affected
tracking-firefox54: --- → ?
Flags: needinfo?(dao+bmo)
(Reporter)

Updated

4 months ago
Duplicate of this bug: 1338931
Tracking 54+ for this issue.
tracking-firefox54: ? → +
Depends on: 1335483
Created attachment 8836934 [details]
screenshot in Ubuntu 16.10

Here's a screenshot showing what this looks like right now (in the Bugzilla attachment "details" page), on Ubuntu 16.10, in Nightly 54.0a1 (2017-02-13) (64-bit)
(Reporter)

Updated

4 months ago
Comment hidden (mozreview-request)
(Reporter)

Updated

4 months ago
Attachment #8837829 - Attachment is obsolete: true
In the interim, this will be solved by bug 1339966 which will disable this feature for Linux.
[Tracking Requested - why for this release]: I propose that we stop tracking this for 54 since it has been disabled by bug 1339966 on Linux.
tracking-firefox54: + → ?
Linux is Tier 1 though, and supporting only a sub set of web features there doesn't seem right. This is also completely our fault, not an inherent limitation of Linux. We should at least have a plan to get this fixed.
Untracking for 54, but it seems like there should be a bug tracking re-enabling this feature on linux?
status-firefox54: affected → disabled
tracking-firefox54: ? → -
uh, nevermind, I guess this is that bug.

Updated

3 months ago
Blocks: 1154677
Keywords: regression
Summary: select popups on Linux lack sufficient contrast → [e10s] select popups on Linux lack sufficient contrast
You need to log in before you can comment on or make changes to this bug.