Closed Bug 1335483 Opened 8 years ago Closed 8 years ago

<select> dropdowns are unreadable on Ubuntu

Categories

(Core :: Layout: Form Controls, defect)

Product:
Core
Component:
Layout: Form Controls
Version:
50 Branch
Platform:
Unspecified
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox53 --- ?
firefox54 --- fixed

People

(Reporter: mconley, Assigned: jaws)

References

Details

Attachments

(2 files)

On mozilla.org in the language selector
246.73 KB, image/png
Details
Bug 1335483 - Compare the user agent value for the select option styling to determine if content has opted-in to custom styling.
59 bytes, text/x-review-board-request
mconley
: review+
Details 
See attachment.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
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 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+
(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.
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
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.
https://hg.mozilla.org/mozilla-central/rev/d02ab1fb2cbc
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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
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)
Depends on: 1395340
Depends on: 1391118
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: