<select> dropdowns are unreadable on Ubuntu

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
2 years 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

Attachments

(2 attachments)

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

See attachment.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Reporter)

Comment 2

2 years ago
mozreview-review
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)
Duplicate of this bug: 1336127
(Reporter)

Comment 6

2 years ago
mozreview-review
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

2 years 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
(Reporter)

Comment 12

2 years ago
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. :/
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)
If it helps, I have only seen the failure on OSX and Windows.
Comment hidden (mozreview-request)

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d02ab1fb2cbc
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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

2 years 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

2 years ago
Depends on: 1395340
You need to log in before you can comment on or make changes to this bug.