Skip to content

Conversation

@nolanlawson
Copy link
Contributor

@nolanlawson nolanlawson commented Apr 5, 2024

It occurred to me that, instead of regexes, we can use the elm.style CSSStyleDeclaration object to serialize out the style attribute. This has some benefits:

  1. We delegate the hard work to JSDOM
  2. We can handle stuff like data: base64 URLs correctly

And some downsides:

  1. JSDOM's parsing does not seem perfect, e.g. it doesn't seem to understand ! important (due to the space) and it doesn't seem to like a leading ;
  2. For invalid CSS declarations (e.g. color: yolo) it will just strip them out
  3. JSDOM normalizes some values, e.g. background-color:#FFFFFF;" becomes background-color: rgb(255, 255, 255);

I think this is an acceptable tradeoff, especially since it makes the code a lot cleaner.

/>
<div
style="background-color: red;"
style="background-color: red !important;"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out we don't parse !important correctly in some cases. Opened a separate bug for that: salesforce/lwc#4125

@nolanlawson nolanlawson requested a review from jmsjtu April 5, 2024 21:55
Copy link
Contributor

@jye-sf jye-sf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the change in behavior for ! important worth calling out in a comment?

@nolanlawson
Copy link
Contributor Author

! important was always kind of broken; we just discovered it now due to actually writing tests for all whitespace combinations.

When looking at snapshots in core, I don't see anyone actually using ! important so it probably doesn't matter.

@nolanlawson nolanlawson merged commit adb8770 into jtu/normalize-style-attr Apr 8, 2024
@nolanlawson nolanlawson deleted the nolan/use-jsdom branch April 8, 2024 16:49
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.

3 participants