<select> dropdowns are unreadable on Ubuntu

RESOLVED FIXED in Firefox 54

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
10 months ago
3 months ago

People

(Reporter: mconley, Assigned: jaws)

Tracking

(Blocks: 1 bug)

50 Branch
mozilla54
Unspecified
Linux
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify ?

Firefox Tracking Flags

(firefox53 ?, firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

Created attachment 8832130 [details]
On mozilla.org in the language selector

See attachment.
(Assignee)

Updated

10 months ago
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment on attachment 8832217 [details]
Bug 1335483 - Compare the user agent value for the select option styling to determine if content has opted-in to custom styling.

https://reviewboard.mozilla.org/r/108544/#review110100

I think my only concern here is that we're going to be calling this maybe-recursively, and that means calling getComputedStyle (so flushing styles) perhaps multiple times. Can we avoid that?

::: toolkit/modules/SelectParentHelper.jsm:186
(Diff revision 1)
> +  // Determine user agent background-color and color and
> +  // skip applying the custom color if it matches the user agent values.
> +  let dummy = element.ownerDocument.createElement("label");
> +  dummy.style.color = "-moz-comboboxtext";
> +  dummy.style.backgroundColor = "-moz-combobox";
> +  let dummyCS = win.getComputedStyle(dummy);
> +  let uaBackgroundColor = dummyCS.backgroundColor;
> +  let uaColor = dummyCS.color;

populateChildren might be called recursively for optgroups, which means we'll run this again. We should probably only run it once, max.
Attachment #8832217 - Flags: review?(mconley) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

10 months ago
Duplicate of this bug: 1336127
Comment on attachment 8832217 [details]
Bug 1335483 - Compare the user agent value for the select option styling to determine if content has opted-in to custom styling.

https://reviewboard.mozilla.org/r/108544/#review110410

Looks sensible. Thanks!

::: toolkit/modules/SelectContentHelper.jsm:134
(Diff revision 3)
> +  get uaBackgroundColor() {
> +    if (!this._uaBackgroundColor) {
> +      this._calculateUAColors();
> +    }
> +    return this._uaBackgroundColor;
> +  },
> +
> +  get uaColor() {
> +    if (!this._uaColor) {
> +      this._calculateUAColors();
> +    }
> +    return this._uaColor;
> +  },

Ah, okay, caching. I wonder if we need to add an observer if the native theme stuff changes to clear these cached values?

Maybe fodder for a follow-up - I won't block here.
Attachment #8832217 - Flags: review?(mconley) → review+
Duplicate of this bug: 1336421
Comment hidden (mozreview-request)
(In reply to Mike Conley (:mconley) from comment #6)
> ::: toolkit/modules/SelectContentHelper.jsm:134
> (Diff revision 3)
> > +  get uaBackgroundColor() {
> > +    if (!this._uaBackgroundColor) {
> > +      this._calculateUAColors();
> > +    }
> > +    return this._uaBackgroundColor;
> > +  },
> > +
> > +  get uaColor() {
> > +    if (!this._uaColor) {
> > +      this._calculateUAColors();
> > +    }
> > +    return this._uaColor;
> > +  },
> 
> Ah, okay, caching. I wonder if we need to add an observer if the native
> theme stuff changes to clear these cached values?
> 
> Maybe fodder for a follow-up - I won't block here.

I tested this and the cache only exists while the popup is opened, so we don't need to worry about clearing the cache.

Comment 10

10 months ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/88872e0b68d2
Compare the user agent value for the select option styling to determine if content has opted-in to custom styling. r=mconley

Comment 11

10 months ago
I had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=74335804&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/eff98747c6060d924a88a75d2f99798422803168
Flags: needinfo?(jaws)
If we can't get this fixed today, we should maybe backout bug 910022 from central, because I suspect we're making life kinda hard for our Nightly Linux users. :/

Comment 13

10 months ago
I can push a backout, just needinfo me if that's what you end up choosing.
I've got a push to tryserver to figure out why that test is failing there. I can't reproduce the failure locally.
Flags: needinfo?(jaws)

Comment 15

10 months ago
If it helps, I have only seen the failure on OSX and Windows.
Comment hidden (mozreview-request)
https://hg.mozilla.org/integration/autoland/rev/d02ab1fb2cbc43b425c2858d3b70f9ade88cfd65

Comment 18

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d02ab1fb2cbc
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Updated

10 months ago
Duplicate of this bug: 1336800
Comment hidden (typo)
Comment hidden (typo)
Are you still planning uplift for this (from the discussion in https://bugzilla.mozilla.org/show_bug.cgi?id=910022#c66 )?
status-firefox53: --- → ?
Flags: needinfo?(jaws)
Not yet, bug 1338283 would need to get fixed first. Right now that is the last known issue that we are tracking, though there may be issues with high contrast and classic themes.
Flags: needinfo?(jaws)
(I'm marking bug 1338283 as depending on this one, since it kinda picks up where this bug left off in making these dropdowns readable.  In this bug they were dark-text-on-dark-background; now that this is fixed, they're light-text-on-light-background, which is covered in bug 1338283 IIUC.)
Blocks: 1338283

Updated

8 months ago
Depends on: 1354196
Jared, do we have automation coverage for this? Would it benefit from manual testing?
Flags: qe-verify?
Flags: needinfo?(jaws)
Hi Andrei, the feature is disabled on Linux due to bug 1338283. We can do manual testing to confirm that the dropdowns are readable on Linux, but the automated testing will cover all other platforms.
Flags: needinfo?(jaws)

Updated

3 months ago
Depends on: 1395340
(Assignee)

Updated

3 months ago
Depends on: 1391118
You need to log in before you can comment on or make changes to this bug.