New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #7: make option list readable in Firefox when parent has dark b… #14

Merged
merged 2 commits into from Jul 21, 2016

Conversation

Projects
None yet
3 participants
@brendanfalkowski
Copy link
Contributor

brendanfalkowski commented Mar 4, 2016

Resolves issue #7. In Firefox the option list inherits the background color of the closest ancestor that has a solid background color. If the .custom-select wrapper uses a dark solid background or only uses a gradient, then Firefox traces up through its ancestors until a solid color is found. If that color is dark then the option list is unreadable in Firefox (black text on dark background).

By setting a solid fallback of #fff on the .custom-select (or in the demo's case: .button) Firefox will render the option list readably (black text on white background).

I updated the demo to include a dark parent example. The issue can be reproduced by modifying the CSS for .button to remove the solid background fallback in the gradient declaration.

@toddparker

This comment has been minimized.

Copy link
Member

toddparker commented Mar 15, 2016

Nice. Thanks @brendanfalkowski. We'll try to pick this up in the next few days.

@brendanfalkowski

This comment has been minimized.

Copy link
Contributor Author

brendanfalkowski commented Jul 19, 2016

@toddparker Any chance of getting this PR accepted? Would like to see the additional dark-background case handled so it's more robust.

@zachleat

This comment has been minimized.

Copy link
Member

zachleat commented Jul 21, 2016

Looking!

@zachleat

This comment has been minimized.

Copy link
Member

zachleat commented Jul 21, 2016

@brendanfalkowski Just to confirm—this only modifies the demo styles. Is this what you intended? Is the bug reproducible when the demo styles are not used?

@zachleat zachleat self-assigned this Jul 21, 2016

@zachleat zachleat added the question label Jul 21, 2016

@brendanfalkowski

This comment has been minimized.

Copy link
Contributor Author

brendanfalkowski commented Jul 21, 2016

@zachleat Thanks for picking this up!

It totally depends how you style the <select> background. So, yes, it's present in the demo.css and not the core select-css.css. Way way back in 2014 I used both while testing, and ran into the Firefox issue.

As-is, the demo code doesn't produce a readable option list if used on a dark page in Firefox. You can test by modifying the .button CSS from:

background: linear-gradient(to bottom, #ffffff 0%,#e5e5e5 100%);
to
background: linear-gradient(to bottom, #ffffff 0%,#e5e5e5 100%) #FFF;

Firefox repurposes the background on the <select> to style the (normally inaccessible) option list background. The option list text color isn't changeable to my knowledge. Here's what happens:

  1. The demo code uses a gradient (my test did too) on .button and Firefox won't apply gradients to the option list.
  2. Firefox traces up the DOM until it finds an ancestor element with a solid background color.
  3. It sets that color as the option list background.
  4. If the ancestor's background is dark, the option list is dark text on dark background (not readable).

You can short-circuit this behavior by providing a solid fallback color on the <select>. I assigned #FFF in the demo, so the option list is black text on white background.

Note: if the gradient depends on rgba() for transparency, setting a background might spoil a design that has the page background bleeding through the UI (someone's glassy skeuomorphantasy) — but having a readable option list matters more.

So the pull request updates the demo with a solid fallback color to the background gradient on .button and a test case in the demo/index.html of a dark ancestor. If somebody downloads the project and starts hacking on it, hopefully they won't get stuck unable to use it if the demo case is more robust.

@zachleat

This comment has been minimized.

Copy link
Member

zachleat commented Jul 21, 2016

@brendanfalkowski Yes—I’m definitely on board with merging this. I wonder if it’s also worth adding one or two sentences describing the problem as a note to the readme as well? Since it’s a demo style, it’s worth documenting the problem/solution in case others run into it. Maybe even something as simple as short description and a link to the issue?

@brendanfalkowski

This comment has been minimized.

Copy link
Contributor Author

brendanfalkowski commented Jul 21, 2016

(on mobile) I added this in an earlier commit/PR to index.html but I
think it's not linked to this PR. I was looking at it this morning, and
thought to include the same again but I saw the demo had the browser quirk
notes moved to the readme. I agree it'd be nice to see the note in the
context of the demo. I can add it in about an hour.

On Thursday, July 21, 2016, Zach Leatherman notifications@github.com
wrote:

@brendanfalkowski https://github.com/brendanfalkowski Yes—I’m
definitely on board with merging this. I wonder if it’s also worth adding
one or two sentences describing the problem as a note to the readme as
well? Since it’s a demo style, it’s worth documenting the problem/solution
in case others run into it. Maybe even something as simple as short
description and a link to the issue?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#14 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AANHjDg6Hh6f9qJ589NkRY82Zw4h7kgPks5qX7KygaJpZM4HpGCB
.

@zachleat

This comment has been minimized.

Copy link
Member

zachleat commented Jul 21, 2016

Great, thanks @brendanfalkowski!

@brendanfalkowski

This comment has been minimized.

Copy link
Contributor Author

brendanfalkowski commented Jul 21, 2016

Oops, I added notes into the demo (not readme). Looks like this now:

screen shot 2016-07-21 at 12 28 23 pm

If that's ok, merge away. Otherwise I can copy or move to the readme. I'd probably be more aware from seeing it in context of that demo case.

@zachleat

This comment has been minimized.

Copy link
Member

zachleat commented Jul 21, 2016

It’s beautiful—thanks!

@zachleat zachleat merged commit 448d639 into filamentgroup:master Jul 21, 2016

@zachleat zachleat removed the question label Jul 21, 2016

@zachleat

This comment has been minimized.

Copy link
Member

zachleat commented Jul 21, 2016

@zachleat zachleat added this to the v1.0.4 milestone Jul 21, 2016

@zachleat

This comment has been minimized.

Copy link
Member

zachleat commented Jul 21, 2016

Released with 1.0.4 and updated on NPM

@brendanfalkowski brendanfalkowski deleted the brendanfalkowski:fix-firefox-option-background branch Jul 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment