1572208 - SVG SMIL syncbase attributes that reference IDs of animations break if they contain a dash.

Status: RESOLVED INVALID

(Core :: SVG, defect, P3)

Description

[oscar.spen]

Attachment: syncbase-ff-broken.svg

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/75.0.3770.142 Safari/537.36

Steps to reproduce:

Create an SVG with at least two animations.
Give the first animation a begin property of begin="0s".
Give the first animation an id property of first-animation.
Give the second animation a begin property of begin="first-animation.begin+1s".

Actual results:

The second animation does not begin, and a console warning states that a parse error occurred trying to parse "first-animation.begin+1s".

If the dash is removed from the id, say, the id property of the first animation is changed to "first" (and references are updated) the animation works as expected.

Attached is an SVG that should animate properly, but does not. If dashes are removed from the id properties (or the file is opened in Chrome or Safari), it animates as expected.

Expected results:

The second animation should begin one second after the first animation.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[Daniel Holbert [:dholbert]]

Thanks for the report! This seems like a valid bug.

The relevant spec text would be https://www.w3.org/TR/SVG11/animate.html#SyncbaseValueSyntax which says

syncbase-value ::= ( Id-value "." ( "begin" | "end" ) ) ( S? ("+"|"-") S? Clock-value )?

but doesn't define the parsing rules for the Id-value token. It's reasonable to assume that any valid element ID should be allowable there, and apparently we incorrectly reject hyphens.

It looks like the relevant parsing code here is in this function:
https://searchfox.org/mozilla-central/rev/9ae20497229225cb3fa729a787791f424ff8087b/dom/smil/SMILParserUtils.cpp#309

Comment 3

[Daniel Holbert [:dholbert]]

RE what characters are valid for IDs: I found a few resources:

SVG 1.1 has this chunk, which defers to the XML spec:
https://www.w3.org/TR/SVG11/struct.html#IDAttribute

XML has some BNF expressions to define the grammar, which do seem to allow hyphens:
https://www.w3.org/TR/REC-xml/#id

HTML5 apparently allows any non-whitespace character:
https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/id

Comment 4

[Robert Longson [:longsonr]]

You have to escape the dash i.e. begin="first\-animation.begin+1s". Because othewise the dash is interpreted as a minus sign. See https://www.w3.org/TR/2001/REC-smil-animation-20010904/#TimingAttrValGrammars

The backslash character '' can be used to escape the dot separator within identifier and event-name references.

This is a longstanding Chrome bug that it doesn't do this properly.

Comment 5

[oscar.spen]

Arguably in "Parsing timing specifiers",

5. Else: Build a token substring up to but not including any sign indicator (i.e. strip off any offset)

specifically, i.e. strip off any offset means to match an offset-value that appears at the end of the string.

And from:

ID reference values are references to the value of an "id" attribute of another element in the document.

Id-value ::= IDREF
The IDREF is a legal XML identifier.

I would expect it to parse any legal XML identifier with the exception of dots as mentioned in the spec, especially as hyphens are commonly used in identifiers. It seems a bit much that the spec would require the user to remember to escape those.

If you look at the grammar listed, with the exception of dots (because of clock-values), the grammar is unambiguous given any identifier and there's only one valid parse tree for first-animation.begin+1s.

Comment 6

[Robert Longson [:longsonr]]

5. Else: Build a token substring up to but not including any sign indicator...

Is a pretty unambiguous statement of what to do. Feel free to ask the w3c to change it by raising an issue it if you have some proposal. If that happens we can implement the new specification.

Comment 7

[oscar.spen]

No worries at all. I made an issue: https://github.com/w3c/svgwg/issues/722