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

Lowercase all CSS scope tokens #3313

Closed
Tracked by #2964 ...
nolanlawson opened this issue Jan 24, 2023 · 7 comments · Fixed by #3580
Closed
Tracked by #2964 ...

Lowercase all CSS scope tokens #3313

nolanlawson opened this issue Jan 24, 2023 · 7 comments · Fixed by #3580
Labels

Comments

@nolanlawson
Copy link
Collaborator

In #3312 we got rid of the static optimization for SVGs due to a bug in the case sensitivity of styling tokens (synthetic shadow, scoped styles).

The best long-term solution would be to lowercase all scope tokens in the style compiler. This is what Vue does.

Note that this would be an observable change. And it could break code that predicts the scope token value and uses it, e.g. for querySelector:

// This would fail if the attribute were '[componentname_componentname]'
this.template.querySelector('text[componentName_componentName]')
@git2gus
Copy link

git2gus bot commented Jan 24, 2023

This issue has been linked to a new work item: W-12425100

@nolanlawson
Copy link
Collaborator Author

nolanlawson commented Jan 24, 2023

I would maybe even go one step further and randomize the scope token, e.g. lwc-<hash>. It should not be predictable, so that developers cannot take a dependency on it.

@ekashida
Copy link
Member

This is a somewhat similar problem to our scoped id implementation which we "resolved" through documentation. We basically say that id should never be used as a query selector nor as part of a CSS selector, and that it should only be used for accessibility. It would also have been prudent to use a hash instead of appending sequential numbers so if we ever end up generating a hash for each component we should consider using the same one for mangling ids.

@nolanlawson
Copy link
Collaborator Author

nolanlawson commented Feb 20, 2023

Thoughts on this:

  1. The token can't be "truly" randomized, because then Jest snapshots wouldn't be consistent
  2. Ideally it should be opaque, though, to emphasize "don't depend on this"
  3. Right now the token is only based on the namespace and tag name, so in theory these could actually collide today (e.g. two c-buttons on the same page, both using synthetic shadow) – surprised this hasn't been reported already
  4. Tokens are actually associated with templates, not stylesheets. Generating the hash based on the template content seems error-prone – e.g. two templates with the same HTML would hash to the same thing
  5. Maybe the best option is to hash based on the namespace, tagName, and template content – these are unlikely to collide.

@ekashida
Copy link
Member

Although it will initially break tests, we can make snapshots consistent using the jest serializer.

Including the template content into the hash introduces some randomness but it would remain static for as long as the template doesn't change, which I'm guessing is for the majority of components. It could be worse than having a static scope token that never changes or a dynamic one that always changes because it might be confusing for the developer unless they understand the implementation. If we decide to do it, I think it should probably change on every page reload.

@nolanlawson
Copy link
Collaborator Author

Fixing it in the serializer makes sense to me. I do wonder though, how being truly random would interact with Hot Module Replacement. I.e. would we just keep adding new <style>s to the head? And is this maybe a good thing, so that CSS changes are reflected immediately? Might be worth testing.

@nolanlawson
Copy link
Collaborator Author

@ekashida I looked into it, and I'm not sure we can modify the Jest serializer to strip the scope tokens.

Stripping lwc-* is simple enough (just check if it starts with lwc-), but for the existing ones, we can't predict based on the Element instance what the scope token actually is. Even elm.$shadowToken$ is not sufficient, because that doesn't handle the scope tokens added by *.scoped.css.

We might just have to break people's snapshots. 😕

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.

2 participants