Skip to content
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

Fix race condition related to theme CSS (fixes #154) #156

Merged
merged 1 commit into from Oct 28, 2016

Conversation

davidcalhoun
Copy link
Contributor

It looks like theme handling was added to index.js for themes to function properly in dev mode (index.html), but unfortunately this code is also redundantly run in prod, where theme CSS is already handled serverside (index.ejs). In prod, this resulted in the theme CSS being injected twice: once in synchronous CSS in the page head, and again asynchronously in clientside code.

This caused cross-browser styling inconsistencies in components that ultimately rely on window.getComputedStyle (particularly Grommet Box, which depends on the core library's hasDarkBackground in utils/DOM), which assumes that all CSS has already loaded and has been applied to the page. Which in this case isn't guaranteed, since CSS is being loaded async in the second instance.

For instance, Chrome reported elements with a background color of rgb(255,255,255) whereas IE reported elements with a background color of transparent. This code originally changed the href of the theme CSS link tag, and different browsers handled this in different ways. IE immediately resets the first CSS that gets applied (and thus getComputedStyle reported all background colors as transparent), whereas other browsers still retained the styles until the new stylesheet loaded. This ultimately lead to inconsistent styling as reported in #154.

This fix moves the clientside theme injection into the dev-only code (index.html) and injects it synchronously in the document head (yeah, ew document.write, but that's what we want it to do in this case).

@davidcalhoun davidcalhoun changed the title Fix race condition related to theme CSS Fix race condition related to theme CSS (fixes #154) Oct 19, 2016
@jwijay
Copy link
Contributor

jwijay commented Oct 28, 2016

Tested in IE11, Chrome, Safari, Firefox, and LGTM 👍 .

@jwijay jwijay merged commit 93192d7 into grommet:master Oct 28, 2016
@jwijay jwijay removed ready labels Oct 28, 2016
@davidcalhoun
Copy link
Contributor Author

Thanks a ton for checking it out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(Windows 10/ IE11) Section Marquee 1 & 2 text is wrong color
2 participants