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

Potentially invalid value for dasharray #642

Closed
pyoor opened this issue Feb 1, 2019 · 8 comments
Closed

Potentially invalid value for dasharray #642

pyoor opened this issue Feb 1, 2019 · 8 comments

Comments

@pyoor
Copy link

pyoor commented Feb 1, 2019

Dasharray is currently defined as <dasharray> = [ <length> | <percentage> | <number> ]#*
According to https://drafts.csswg.org/css-values-3/#component-multipliers

A hash mark (#) indicates that the preceding type, word, or group occurs one or more times, separated by comma tokens (which may optionally be surrounded by white space and/or comments). It may optionally be followed by the curly brace forms, above, to indicate precisely how many times the repetition occurs, like <length>#{1,4}.

Based on this definition it doesn't appear that combining the hash and asterisk multipliers is valid.

It should be:
<dasharray> = [ <length> | <percentage> | <number> ]# | [ <length> | <percentage> | <number> ]*

[ <a>&lt;length&gt;</a> | <a>&lt;percentage&gt;</a> | <a>&lt;number&gt;</a> ]#*</p>

@ewilligers
Copy link
Contributor

It should be:
<dasharray> = [ <length> | <percentage> | <number> ]# | [ <length> | <percentage> | <number> ]*

Not quite, as we can have
document.body.style.strokeDasharray = '1 2, 3 4,5,6, 7 8';
document.body.style.strokeDasharray === "1, 2, 3, 4, 5, 6, 7, 8"
getComputedStyle(document.body).strokeDasharray === "1px, 2px, 3px, 4px, 5px, 6px, 7px, 8px"

#* is custom notation not defined by CSS Values 3. If it was necessary to stick with CSS Values 3 notation, we would need
<dasharray> = [ [ <length> | <percentage> | <number> ]* ]#
but that might incorrectly suggest that the distinction between whitespace and commas is meaningful.

@dirkschulze
Copy link
Contributor

dirkschulze commented Mar 17, 2019

Apparently, browsers are far more flexible than the above. Here an example: https://codepen.io/krit/pen/Rdydgw?editors=1000

The example suggests that <length> | <percentage> | <number> separation with wsp or comma is optional. No matter which combination.

Also: the relaxed syntax applies to CSS style and presentation attributes.

@AmeliaBR
Copy link
Contributor

Yes, most SVG 1 properties were defined to allow comma and/or whitespace separation of tokens, interchangeably. A single value can use a mix of commas and spaces. For example, you could put a comma after every dash length / gap length pair.

Combination multipliers were adopted when converting SVG to use the CSS grammar. The syntax <token>#* was intended to represent [<token>#]*, meaning any number of space-separated repeats of at least one comma-separated repeat of that token.

However, that syntax was previously discussed and decided against in #191
That resulted in the changes in cd8915c (which confusingly refers to a much later issue number in the commit text!). The chosen replacement uses square brackets for explicit grouping, as in the syntax definition for the points attribute.

The stroke-dasharray definition was clearly overlooked in that clean-up, probably because it uses the * multiplier (any number, including none) instead of the + multiplier (one or more).

Since there always needs to be at least one token in the dasharray for it to be valid, it should also use the + multiplier. And the square brackets would clarify:

<dasharray> = [ [ <length-percentage> | <number> ]+ ]#

Perhaps we could also add a note explaining this syntax in the section on SVG attribute syntaxes. We should also somewhere normatively state how these mixed-separator values are serialized, if that doesn't already exist.

@AmeliaBR
Copy link
Contributor

AmeliaBR commented Mar 17, 2019

Apparently, browsers are far more flexible than the above.

So browsers are using the SVG parsing rules, allowing things like .9.9.9.9 to be parsed as four distinct numbers.

I'm not sure whether that is consistent with CSS parsing. "Space-separated" is actually a misnomer for the CSS + multiplier. It just specifies repeats of tokens, which means the spaces could be optional if the token parsing is unambiguous. But I can't think of a CSS property that accepts a list of numbers, to know if browsers apply the same space-optional parsing rules.

Either way, I don't know whether we need to specify support for the condensed version. It would depend on whether SVG software (e.g., minimizing tools) currently takes advantage of the support.

@svgeesus
Copy link
Contributor

Unpopular opinion: if the CSS syntax is confusing implementors and users (because a given SVG atr uses syntax not compatible with CSS parsing rules) we could say "see text" and textally describe the required SVG parsing rules.

Being compatible with CSS spec notation is great, if it helps; but only, if it helps.

@AmeliaBR
Copy link
Contributor

@svgeesus Using custom parsing instructions would be fine if it was a regular attribute, or if the unusual syntax was only supported in the attribute format. But if browsers are supporting the unusual parsing in actual CSS property declarations (like the style attribute in Dirk's demo), then we need to make sure that CSS itself can describe what is happening.

@css-meeting-bot
Copy link
Member

The SVG Working Group just discussed Potentially invalid value for dasharray, and agreed to the following:

  • RESOLVED: Change the syntax as I wrote it out in AmeliaBR's comment yesterday
The full IRC log of that discussion <myles> Topic: Potentially invalid value for dasharray
<myles> GitHub: https://github.com//issues/642
<myles> AmeliaBR: About the syntax for stroke-array. As I commented yesterday, this is something we discussed before, and we changed a lot of other examples and this one somehow got missed in all those edits.
<myles> AmeliaBR: So, I would recommend changing the grammar to be consistent with the other edits we made, which is to explicitly separate out the space-separated repeats and the comma separated repeats using square brackets
<myles> AmeliaBR: The other issue that krit discovered is there are some weird things where you don't need space separating at all, and I'm not sure if that's consistent with CSS or not. I pinged TabAtkins to see his thoughts, but he didn't get back to me. All browsers are consistent, though, so CSS parsing should change.
<myles> krit: I know TabAtkins's opinion on that, so I would not count on a spec change. At least for the SVG attribute we can specify it how it's implemented. For CSS, it's up to the CSSWG.
<myles> krit: On the other hand, this part is not that important.
<myles> AmeliaBR: We can make an edit to the grammar ourselves, and follow-up on whether spaces are necessary between subsequent numbers if the numbers are obviously distinguishable otherwise. It's a separate issue.
<TabAtkins> There's a bunch of stuff in CSS where you don't *technically* need spaces, if you're careful in how you're butting things against each other.
<myles> AmeliaBR: The other comment I made at the end. We have a statement about how these things are serialized if we allow spaces or comments and I haven't looked to see whether we have that
<myles> krit: For computed style?
<myles> AmeliaBR: Yes, and for serialization too
<myles> AmeliaBR: <reads TabAtkins's IRC comments>
<myles> krit: They don't technically need spaces.
<myles> chris: I remember TabAtkins trying to explain that to me. There's a grammar that defines it but <missed>
<myles> chris: It sounded like handwaving magic to me but okay.
<myles> AmeliaBR: With that clarification from TabAtkins it covers that topic
<myles> AmeliaBR: I'll see if the other part is already covered, if it isn't, I'll make another issue
<TabAtkins> ? No grammar weirdness, you just follow the parsing algorithm. `.9.9` is NUMBER(.9) NUMBER(.9), because the number-token parsing stops at the second `.`, emitting the NUMBER, then starts a new NUMBER.
<AmeliaBR> s/other part/serialization issue/
<myles> krit: We should have another issue to see if implementations actually align.
<myles> krit: Let's open another issue
<chris> s/missed/it says spaces are required then says they aren't in some cases/
<chris> we agree, TabAtkins
<myles> krit: If you read the syntax, the prose doesn't require spaces.
<myles> AmeliaBR: It just says "repeated tokens" without commas that usually means spaces separation but it doesn't necessarily mean that
<TabAtkins> Note that serialization *should* always emit spaces, regardless of what the author input.
<myles> RESOLVED: Change the syntax as I wrote it out in AmeliaBR's comment yesterday
<myles> s/as I wrote/as AmeliaBR wrote/
<myles> AmeliaBR: I can make the edit. I'll also look into the other issue I brought up.

@tabatkins
Copy link
Member

But I can't think of a CSS property that accepts a list of numbers

Here's an example of repeated dimensions without spaces, which every browser correctly accepts as valid:

<!DOCTYPE html>
<div>foo</div>
<style>
div { border-width: .2em.2em.2em.2em; border-style: solid; }
</style>

(scale accepts repeated numbers, but it's not widely supported yet.)

Except where explicitly noted (such as in the grammar for calc()), CSS doesn't care about spaces at all. If you can produce the right token stream without them, it's fine; this can be reliably achieved by using comments to separate things instead, and in some cases (such as the example above) by just using characters that aren't valid at that spot in the current token, so it ends and a new token begins.

It just so happens that the vast majority of the time, spaces are the easiest, shortest way to separate your tokens.


The correct way to indicate that something can be repeated, with either WS or comma between them (but not multiple commas in a row) is with the +# stacked combinator. That handles all variants correctly. So the grammar for dasharray is already correct.

@AmeliaBR's suggested fix is unnecessary, but not incorrect; it's identical in meaning.

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

No branches or pull requests

7 participants