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

Re-fix custom properties css specificity mixin issue #3326

Closed
rictic opened this issue Jan 23, 2016 · 10 comments
Closed

Re-fix custom properties css specificity mixin issue #3326

rictic opened this issue Jan 23, 2016 · 10 comments
Assignees
Labels

Comments

@rictic
Copy link
Contributor

rictic commented Jan 23, 2016

#3323 reverted a desirable change from fd57784 that we'd like to have back. Need to track down the tests that broke, find the root cause, and re-integrate the change with a fix.

@sorvell
Copy link
Contributor

sorvell commented Jan 27, 2016

There is a flaw in the design of the custom property shimming. The PR simply
put more pressure on this by exposing more styling to this flaw. This bin
shows the basic problem:

http://jsbin.com/jisuxo/edit?html,output

@miztroh-zz
Copy link

It's interesting to note that making the document level style node a custom-style works around the issue.

@sorvell
Copy link
Contributor

sorvell commented Jan 28, 2016

Good point. We're trying to preserve as much of the spirit of the spec around :host as we can.

@dfreedm
Copy link
Member

dfreedm commented Jan 28, 2016

In my mind, the most correct way to make the shim'd styles have the lowest priority is to generate classnames for all shim'd nodes that are unique, and of the lowest specificity we can make them. As long as the generated shim styles are placed before other styles, the user styles should then override as written.

@miztroh the reason custom-style works is because it appends a :not() to the selectors inside of it, which increases specificity. This trick should work for any style, as long as the contents of the :not() are more specific than the selector it is trying to override.

@nazar-pc
Copy link
Contributor

Well, I have custom build of 1.2.3 when specificity fix was not reverted and I see green block in both Firefox and Chromium with both Shadow DOM and Shady DOM.

@nazar-pc
Copy link
Contributor

Sorry, probably someone modified it to use is="custom-style" which really helps. In many practical cases there is .something .foo or .something > .foo structure used, especially when using SASS/LESS and I think this is much smaller evil comparing to completely broken specificity system that doesn't have easy workaround.

I do not see that it is feasible to have smaller specificity than it is now, especially that even my-element should override :host styles (and it does with [is=custom-style], which means we can't fix the issue without scanning and processing ALL STYLES, which is definitely not desired behavior.

[is=custom-style] or nested selectors might be documented as official workaround. @sorvell, @azakus, @rictic, what you think?

@sorvell
Copy link
Contributor

sorvell commented Jan 28, 2016

Yes, we should document that using [is=custom-style] helps work around this issue. There are times when this is problematic and I'd still like to consider if we can improve the situation...

I think @azakus's idea may strike the best balance. Practically, this would mean that we change shim'ing for :host in custom properties sheets from x-foo.x-foo-0 to .x-foo-0. While it's true that a selector for x-foo would not match in this case, a single class selector would match and it would be nice to catch this basic use case.

@nazar-pc
Copy link
Contributor

Anyway this is only half of the solution even for this case, since it doesn't cover x-foo's ability to override :host styles, but at least feasible. Whatever is decided - it should be emphasized in release notes and reflected in docs.
Can I prepare PR for this to speed up process or will you do it yourself?

@sorvell
Copy link
Contributor

sorvell commented Jan 28, 2016

A PR would be welcome. Thanks for your help!

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

Successfully merging a pull request may close this issue.

6 participants
@rictic @dfreedm @sorvell @nazar-pc @miztroh-zz and others