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

Rule::listDelimiterForRule needs to return different order for many properties #791

Open
JakeQZ opened this issue Jan 17, 2025 · 6 comments
Labels

Comments

@JakeQZ
Copy link
Contributor

JakeQZ commented Jan 17, 2025

Continuing from #789.

The above-mentioned method is only currently special-casing font properties and the fix for the above.

Many properties' values are comma-separated before being space-separated. It might be better to make that the default, then special-case the others. But doing so risks a breaking change. I don't know which properties use / as a separator. I can't think of any, but there must be some, otherwise it wouldn't be there.

Things like box-shadow will not currently be parsed correctly into the expected object structure, though will still render back out OK.

@JakeQZ
Copy link
Contributor Author

JakeQZ commented Jan 17, 2025

The method was introduced in 9ed24ad.

@JakeQZ
Copy link
Contributor Author

JakeQZ commented Jan 17, 2025

Related: w3c/csswg-drafts#7218

@JakeQZ
Copy link
Contributor Author

JakeQZ commented Jan 17, 2025

From the references, / as delimiter might only be needed for border-image and not any other property.

@oliverklee oliverklee changed the title Rule::listDelimiterForRule needs to return different order for many properties Rule::listDelimiterForRule needs to return different order for many properties Jan 17, 2025
@oliverklee oliverklee added the bug label Jan 17, 2025
@sabberworm
Copy link
Contributor

From the references, / as delimiter might only be needed for border-image and not any other property.

I think we introduced it because we wanted to parse font: Helvetica 12px/2 (which is interpreted as font-family: Helvetica; font-size: 12px; line-height: 2).

@JakeQZ
Copy link
Contributor Author

JakeQZ commented Jan 18, 2025

From the references, / as delimiter might only be needed for border-image and not any other property.

I think we introduced it because we wanted to parse font: Helvetica 12px/2 (which is interpreted as font-family: Helvetica; font-size: 12px; line-height: 2).

Yes, that occurred to me after I wrote the above.

Thinking about various properties, and reviewing the docs on MDN, it seems to me that the best general case would be

['/', ' ', ',']

I.e. most values are first separated by commas, then by spaces, then somtimes by slashes.

I can't think of any exceptions to this, though that doesn't mean there aren't any.

In the case of font-*, I can't think of a value component that might contain a comma apart from a typeface name, in which case it would surely need to be in quotes.

For the next major release, I would suggest simply using the above separator precedence for all cases. We are allowed to make potentially breaking changes in a major release, but I think doing so would fix more issues than it creates. We can then special-case certain properties that may need a differerent ordering. There are many that need separation by commas first.

@sabberworm, @oliverklee WDYT?

@JakeQZ
Copy link
Contributor Author

JakeQZ commented Jan 18, 2025

In the case of font-*, I can't think of a value component that might contain a comma apart from a typeface name, in which case it would surely need to be in quotes.

Actually, I can: a list of typefaces can be provided, and to complicate matters futher, the names may contain spaces and don't need to be enclosed in quotes even if they do. E.g. I think font: Times, Times New Roman, serif 16px/1.5; is valid. However, judging by the DocBlock for the RuleValueList class which gives a similar example, I think this is parsed correctly, and is why a different separator ordering for font* is needed, with space as the 'outermost' delimeter.

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

No branches or pull requests

3 participants