Closed Bug 1338283 Opened 7 years ago Closed 2 years ago

[e10s] select popups on Linux lack sufficient contrast (when dom.forms.select.customstyling is enabled)

Categories

(Core :: Layout: Form Controls, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox54 - disabled

People

(Reporter: jaws, Unassigned)

References

(Blocks 2 open bugs, )

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

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)
Flags: needinfo?(dao+bmo)
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)
(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>.)
Flags: needinfo?(dao+bmo)
Tracking 54+ for this issue.
Depends on: 1335483
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)
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.
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?
uh, nevermind, I guess this is that bug.
Blocks: e10s-select
Keywords: regression
Summary: select popups on Linux lack sufficient contrast → [e10s] select popups on Linux lack sufficient contrast
Blocks: 1406865
(In reply to Julien Cristau [:jcristau] from comment #11)
> Untracking for 54, but it seems like there should be a bug tracking
> re-enabling this feature on linux?
(In reply to Julien Cristau [:jcristau] from comment #12)
> uh, nevermind, I guess this is that bug.

No -- IIUC, this bug tracks the contrast issues *that happen when the feature is enabled*. (the issues that need to be fixed before the feature can be re-enabled).

IMO we should leave this bug focusing on the same contrast issues that it was initially filed for, and we can track the pref-flip re-enabling in a separate bug. (I'll co-opt bug 1406865 for that, which was filed recently on the fact that the feature is disabled.)
Summary: [e10s] select popups on Linux lack sufficient contrast → [e10s] select popups on Linux lack sufficient contrast (when dom.forms.select.customstyling is enabled)
Blocks: e10s-select-styling
No longer blocks: e10s-select
Is this on the roadmap at all?  Per comment 10, it seems like we should aim to get the issues here fixed, so we can reenable custom styling on Linux (bug 1406865).  Right now, it's effectively a linux-only regression from e10s.

(FWIW: I just retested this to be sure it's still an issue, and it is.  Specifically, if I enable the customstyling pref, then bugzilla's attachment dropdown-menus still render with barely-visible text, as shown in attachment 8836934 [details].)
What's the status with this bug? Having disabled dom.forms.select.customstyling is pretty annoying. On the other hand, having it enabled, Firefox lacks some styling. It's one of the bugs that are really annoying and version after version they are not fixed. I will attach screenshots.
Attached image Ff-dropdown.png
This one shows how Firefox renders the drop down with dom.forms.select.customstyling enabled.
Attached image Chr-dropdown.png
This one shows how a Chromium based browser (Vivaldi in this case) renders the drop down, which is correct.
This one shows how Firefox renders the drop down with dom.forms.select.customstyling disabled. Extremely ugly (no colors, ugly font, no font sizes or style).

I think maybe this issue has gone away -- I'm guessing as part of our non-native theme conversion, maybe (bug 1535761), since this was due in part to the particulars of certain gtk themes (per comment 0).

I just retested Ubuntu 16.10 using data:text/html,<select><option>1<option>2<option>3

  • an old Nightly from the day that I posted comment 6 (when we were just shipping Nighlty with this contrast issue by default, before the dom.forms.select.customstyling pref was introduced)
  • ...vs. current Nightly with the customstyling pref flipped to true

And I confirmed that I could reproduce the issue in old Nightly (the unselection options are grayed-out and invisible), vs. no such issue in current Nightly with the pref flipped to true.

I'm going to see if I can narrow down a fix range, but for now unless anyone else can repro, I think we can call this WORKSFORME and then perhaps we can flip the pref in bug 1406865.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WORKSFORME
See Also: → 1751545

Note, there's a version of this bug that still does exist, which I filed as bug 1751545.

(In reply to Daniel Holbert [:dholbert] from comment #21)

I'm going to see if I can narrow down a fix range

OK, I've got a fix range (obtained in an Ubuntu 16.04.7 VM where I can still reproduce this bug in old affected Nightlies)

Last bad:
http://archive.mozilla.org/pub/firefox/nightly/2018/12/2018-12-09-09-35-14-mozilla-central/firefox-65.0a1.en-US.linux-x86_64.tar.bz2
Built from https://hg.mozilla.org/mozilla-central/rev/88d304c633b6091a21fc64290256ef3fb51b7421

First good:
http://archive.mozilla.org/pub/firefox/nightly/2018/12/2018-12-09-21-43-27-mozilla-central/firefox-65.0a1.en-US.linux-x86_64.tar.bz2
Built from https://hg.mozilla.org/mozilla-central/rev/53fd96ca5aa4298054f581ca846ea2cccbe76085

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=88d304c633b6091a21fc64290256ef3fb51b7421&tochange=53fd96ca5aa4298054f581ca846ea2cccbe76085

In that range, it looks like bug 1511138 would've been the thing that fixed this. (It had a bunch of patches, some of which mention theming and SelectChild which I think is about the select element).

--> changing resolution to FIXED with a dependency on that bug.

Depends on: 1511138
Resolution: WORKSFORME → FIXED
You need to log in before you can comment on or make changes to this bug.