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

Add tests for custom properties that use CSS-wide keywords #46814

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kizu
Copy link
Contributor

@kizu kizu commented Jun 18, 2024

After encountering several inconsistencies in how custom properties use CSS-wide keywords, I did an extensive testing for most of the cases I could think of (CodePen), and decided to add these as tests.

When I am testing it, Chrome succeeds in all cases, Firefox fails in 5 of them, and Safari TP in 8 of them.

Relevant specs:

There were a couple of disjointed tests for two of the assertions that I found (one, two), but I decided that it won't hurt to have those duplicate in one test file for all the CSS-wide keywords.

I also tested many various permutations, including some obvious ones, and this was good, as some of the failures I found were not obvious, like Firefox failing with two cases for registered properties with initial and inherit values.

I did find only one bug related to the failures I found: https://bugs.webkit.org/show_bug.cgi?id=271136 — there might be others, but I am planning to fill separate bugs for Firefox and Safari anyway. Here they are:

Copy link
Contributor

@svgeesus svgeesus left a comment

Choose a reason for hiding this comment

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

Looks nice overall

<!DOCTYPE html>
<head>
<title>CSS Custom Properties: Using CSS-wide keywords</title>
<link rel="help" href="https://drafts.csswg.org/css-variables/#ref-for-css-wide-keywords%E2%91%A0">
Copy link
Contributor

Choose a reason for hiding this comment

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

I worry a little about the ① which is how Bikeshed de-dupes ids.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, please don't use those #ref-for IDs, they're auto-generated and not remotely stable, and created just so the index can link to those elements. It would be more reasonable to use a text fragment; then at least it wouldn't break when someone added or removed a reference above that point in the spec.

But really, if you need an anchor to that paragraph, we can just add an id to the paragraph. Please do that instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tabatkins I'll be happy to change this. I did not quickly find a way to add an id to a paragraph in bikeshed, what could be the best way to do this in this case?

Here is this place in the raw spec: https://github.com/w3c/csswg-drafts/blob/main/css-variables-1/Overview.bs#L219

I wonder if there is a way to make the ref link in this case to be stable instead (provide a hard-coded id to it?)

Or maybe a more simple fix in this case would be for me to just link to the section containing this paragraph, in this case, https://drafts.csswg.org/css-variables/#defining-variables ?

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

Successfully merging this pull request may close these issues.

None yet

4 participants