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!: reduce specificity of generated stylesheets to 0 #1930

Closed
wants to merge 3 commits into from

Conversation

mstade
Copy link
Contributor

@mstade mstade commented Nov 17, 2023

What is this, and why is it necessary?

By reducing the specificity of selectors in the generated stylesheets to 0, consumers of Plot can much more easily override these styles using their own stylesheets while still enjoying the default styling of Plot.

Without this, consumers of Plot must resort to increasing the specificity of selectors or individual styles, which can be both awkward and hard to do.

A breaking change, technically

While this change is meant to be safe, it is technically a breaking change. Any consumer of Plot that is trying to override css, but failing due to lower specificity, should start seeing those styles applied. While that may actually fix a few bugs out there, it could also cause some others, where those consumers have simply missed that those styles aren't being applied on top of the defaults.

Why so many snapshot updates?

Because the change in selector specificity affects every chart, all snapshots will need regenerating. The rendered output should yield no differences however. If it does, that's a bug.

Alas – this many snapshot updates seem to make GitHubs UI practically worthless for reviewing the code however. My apologies, I don't have a good answer for how to fix that. I did however split the changes into two commits for this reason:

I hope this helps.

Related resources

@mstade mstade marked this pull request as ready for review November 17, 2023 21:08
Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

Thanks, I didn’t know about :where. I can see this being useful. My consideration now is compatibility, including backwards compatibility. It looks like :where is fairly widely supported, but it’s only since 2021 that it’s been implemented in browsers. Can you speak a little more to the use case that this solved for you? Is there an example where you tried to override Plot’s default styles, and were surprised or stymied by the specificity of those styles?

@mstade
Copy link
Contributor Author

mstade commented Nov 17, 2023

These are great questions @mbostock – thank you so much for reviewing!

The compatibility concern is of course very relevant, and I must admit I have not done due diligence here – my apologies! I have the luxury of not really having to target legacy environments very often, and have been using this selector for some time with great success. It would be possible of course to use something like @supports selector(:where), but I fear that feature may be about as well supported as :where to be honest, so might not be all the rage after all.

In any event, to your questions.

We've been working with Plot the past week to put together some dead simple time series charts, and when we were happy with the functionality and configuration I got to work trying to style these. In our application we make heavy use of CSS modules for locally scoped class selectors. The specificity of these are 0,1,0 – the same as any other pure class selector.

When applying this to Plot, I noticed that my styles for the legend didn't seem to take, at all. It turns out, while setting className on the plot will just set it outright, for the legend this isn't true. Instead it sets names like ${className}-swatches ${className}-wrap, and similar. This is a little awkward, but possible to do in CSS modules using the :global scope – e.g. something like .legend:global(-swatches) will generate css looking something like .Xyz123-swatches, where Xyz123 is a unique hash generated at build time.

So I did this, and some of the styles took, but not all. For example, I tried setting font-size but the style generated by Plot took precedence because the style is injected later in the document than my CSS module styles. And with the same specificity (0,1,0), Plot's generated style won the day. Changing Plot's selectors to :where(...) however resets the specificity to 0,0,0, meaning it doesn't matter where Plot injects these styles, a regular class selector will take precedence.

Of course, it's possible to override these styles by increasing the specificity. Using something like !important or even just adding another type of selector would do, like an id or attribute selector, just bumping that selector ever so slightly higher. But this is not without issues either, and can break in subtle and hard-to-catch ways – for example if Plot changes its selectors to be more specific.

That's why many reusable libraries that ship styles these days make heavy use of the :where selector, to reset the specificity all the way down to 0. Since any other selector will increase the specificity this basically guarantees the styles will be overridable. @layer is also gaining in popularity, and may be an even better solution, but that's even more bleeding edge... 😅

It may be best to actually implement a new test case for this, and my apologies for not doing this – time permitting I will look into it over the weekend. It really shouldn't be particularly difficult. Maybe that will be a better answer to your questions?

@mstade
Copy link
Contributor Author

mstade commented Nov 18, 2023

@mbostock I implemented a very basic test case in e325399. These changes are wider than just categorical legends of course, but before I spend more effort in developing further test cases I'd like to just check that this:

  1. Is in line with how you test things in this project and
  2. You believe these changes to be relevant for the project

Verifying that these changes actually do what they say on the tin is very simple, just revert 755954e. I've recorded a brief session of me doing just this, to show you how reverting the changes makes the second legend ignore the styles we're trying to set by specifying className. The recording first shows the result of the test case with the fix implemented, and then I revert 755954e to show how the styles revert to the default, due to the specificity conflict. Finally I reset head to remove the revert commit and it shows the fixes being applied again:

Screen Recording 2023-11-18 at 16 03 00

I hope this is a sufficiently small and good example of the use case I'm trying to solve with these changes – many thanks for your review!

(Edit: updated SHAs post rebase)

@mstade
Copy link
Contributor Author

mstade commented Nov 21, 2023

If anyone else comes across this PR and wonders about a workaround that doesn't require you to write more specific CSS selectors, this hack should do the trick:

let css = style.innerHTML
for (const { selectorText } of style.sheet.rules) {
  css = css.replace(
    selectorText,
    `:where(${selectorText})`,
  )
}
style.innerHTML = css

In the above code, style refers to the <style> element that Plot generates when calling plot(), legend() etc.

By no means a great solution, but it does seem to work in the short term.

By reducing the specificity of selectors in the generated stylesheets to 0,
consumers of Plot can much more easily override these styles using their
own stylesheets while still enjoying the default styling of Plot.
Because the change in selector specificity affects every chart, all
snapshots will need regenerating. The rendered output should yield no
differences however.
This test renders two categorical legends, one using default Plot styles and
the other using styles by setting the `className` option. The styles being
overridden have the same selector specificity as those set by Plot, would
it not be for the `:where()` selector resetting Plot's specificity to 0.

This is easily verified by reverting b2ff68e.
@mstade
Copy link
Contributor Author

mstade commented Dec 4, 2023

@mbostock / @Fil – appreciating you have other things to do and this PR may not be a priority – I'm wondering if you think the proposed changes are at all relevant to this project? If so if I should implement more tests or have any thoughts on other changes before this can be brought to main? Thank you for taking the time to review!

@mbostock
Copy link
Member

mbostock commented Dec 8, 2023

Sorry for the delay on this. We’ll get this merged. I should have included it in 0.6.12 but I’ll take it from here. Thank you!

@mstade
Copy link
Contributor Author

mstade commented Dec 9, 2023

Thank you @mbostock – much obliged! If you'd like me to look into expanding the test suite or such related to this, please do feel free to let me know. Happy to contribute!

chaichontat pushed a commit to chaichontat/plot that referenced this pull request Jan 14, 2024
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.

2 participants