Make String Handlers Use useImportant #256
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The string handlers were always inserting rules with no `!important`, rather than respecting the settings specified by the library. This updates the string handlers to receive an additional `useImportant` argument, which is then passed down to the various places that need it (rather than just passing `false`). I updated the existing test, which was confirming the wrong input, and also added some more tests to `no-important_test.js` to confirm that it was still being disabled. In practice this helps to fix the issue where Firefox makes it impossible to override CSS that has `!important` with an animation, unless the animation is, also, using `!important`: http://tosbourn.com/firefox-honours-important-in-css-animations-no-one-else-seems-to/ Test Plan: I ran `npm test` and it passed.
xymostech
approved these changes
Jul 14, 2017
There was a problem hiding this comment.
This code looks good to me! Pretty simple to see the flow here. :) I guess we just never did this before because we weren't sure it was important?
In practice this helps to fix the issue where Firefox makes it impossible to override CSS that has !important with an animation, unless the animation is, also, using !important:
Huh, what an interesting effect. Looking now, I would have expected that would happen everywhere, but CSS is strange, so oh well.
|
|
||
| assertStylesInclude('font-family:"CoolFont",sans-serif'); | ||
| assertStylesInclude('font-family:CoolFont;'); | ||
| assertStylesInclude("src:url('coolfont.ttf');"); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The string handlers were always inserting rules with no
!important, rather than respecting the settings specified by the library. This updates the string handlers to receive an additionaluseImportantargument, which is then passed down to the various places that need it (rather than just passingfalse). I updated the existing test, which was confirming the wrong input, and also added some more tests tono-important_test.jsto confirm that it was still being disabled.In practice this helps to fix the issue where Firefox makes it impossible to override CSS that has
!importantwith an animation, unless the animation is, also, using!important:http://tosbourn.com/firefox-honours-important-in-css-animations-no-one-else-seems-to/
Test Plan:
I ran
npm testand it passed.