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

'd' presentation attribute: use path() function #374

Open
wants to merge 2 commits into
base: master
from

Conversation

@ewilligers
Copy link
Contributor

ewilligers commented Dec 18, 2017

This brings the spec, the web platform tests and the only
shipping implementation (Chrome) into consistency:

none | path()

closes #320

@ewilligers ewilligers force-pushed the ewilligers:d-path-function branch from 25c0cd2 to 71900f5 Dec 19, 2017

@dirkschulze

This comment has been minimized.

Copy link
Contributor

dirkschulze commented Dec 29, 2017

@ewilligers This does not seem correct. For compatibility reasons, the d CSS property must support the string version as well (beside the basic shape version). Furthermore, I am unsure if we want to limit d to just path() or support all basic shapes (including circle, ellipse and rectangle).

@ewilligers

This comment has been minimized.

Copy link
Contributor Author

ewilligers commented Jan 7, 2018

For compatibility reasons

Which implementations support a string version of the presentation attribute?

Do you mean consistency with the attribute? Note we already have unitless distance attributes (width etc) that have units in their CSS presentation attributes. Attribute syntax and CSS syntax are different.

@AmeliaBR

This comment has been minimized.

Copy link
Contributor

AmeliaBR commented Jan 8, 2018

@dirkschulze

For reference, there is discussion in the following issues:

I still strongly disagree with using path() in any place other than as an interoperable <shape-function> where it can be substituted for any of the other shape functions.

But none of the other implementers have weighed in to argue for that approach, and Chrome has been shipping it following Eric's proposal.

I will point out that the current state (path() supported in some CSS properties, other shapes in other properties) has proven to be very confusing to authors: https://css-tricks.com/basic-shapes-path-never-twain-shall-meet/

The proposal should probably be discussed with CSS WG as well as SVG.

@dirkschulze

This comment has been minimized.

Copy link
Contributor

dirkschulze commented Jan 8, 2018

@ewilligers The change you made affects the d property as well as the d presentation attribute. So you pretty much disallow path strings everywhere. This is the compatibility issue I mean. Furthermore, it is confusing that one will be allowed to use path strings on the d presentation attribute but not on the d CSS property. You can't simply copy the content from the attribute to the style section and are done with it. You must add the path() function. IMO there is no benefit to not allow the path string even if it is for the d property only. We are not getting away with the path string on the d presentation attribute ever. So why forcing its removal on the CSS property?

Also, the spec text should say what happens when a <fill-rule> was specified. How does this interact with the fill-rule CSS property? CSS Masking has a similar problem with clip-rule and clip-path.

@AmeliaBR I totally agree with you. IMO it is fine that Chrome starts shipping with path() and adds support for other shapes later but spec wise we should not split the shape functions.

IMO we should not make this change for SVG2 at all and instead split the d property out in general into its own spec or another existing FXTF/CSSWG spec. The path data string logic can stay in SVG2 for now.

@fsoder

This comment has been minimized.

Copy link

fsoder commented Jan 8, 2018

I'd agree with @dirkschulze that there would need to be additional prose for the presentation attribute form of d - because regardless of <string> or <path()> you cannot just pass the attribute value to the CSS parser and get something path'y back (a jumble of tokens is what you'll be getting.) (An attribute value - string - isn't a <string-token>. You need quotes and potentially escaping to produce the latter from the former.) So since pre-processing is going to be required anyway, one might as well go with what seems like the lesser legacy form (<path()>), and avoid adding another (<string>).

@css-meeting-bot

This comment has been minimized.

Copy link
Member

css-meeting-bot commented Apr 2, 2018

The Working Group just discussed 'd' presentation attribute notation and PR.

The full IRC log of that discussion <BogdanBrinza> topic: 'd' presentation attribute notation and PR
<BogdanBrinza> GitHub: https://github.com//pull/374
<BogdanBrinza> We discussed this just now and the current change introduces the CSS notation for the d attribute. For compatibility SVG attribute would need to take the string as well, not just the path function - so we'd like to ask to update the PR to capture this difference in SVG vs CSS syntax more prominently.
@dirkschulze

This comment has been minimized.

Copy link
Contributor

dirkschulze commented Jul 23, 2018

@ewilligers Do we want to move this change to SVG 2.1 or rather level 1 of a path spec?

@ewilligers ewilligers force-pushed the ewilligers:d-path-function branch from 71900f5 to 3d8486a Aug 14, 2018

@ewilligers

This comment has been minimized.

Copy link
Contributor Author

ewilligers commented Aug 14, 2018

I have updated the pull request to describe the difference between the property and attribute syntax.

@@ -144,7 +144,11 @@ <h2 id="syntax">Attribute syntax</h2>
unitless length and angles to be used in presentation attribute while
disallowing them in corresponding property values.</p>

<p class="note">Note that all <a>presentation attributes</a>, since they are
<p>The <a>'d'</a> presentation attribute is an exception, it is parsed

This comment has been minimized.

@dirkschulze

dirkschulze Aug 20, 2018

Contributor

For CSS Transforms we are a little bit more verbose and explain why there are differences as well. See https://drafts.csswg.org/css-transforms/#svg-syntax I think we should be a little bit more verbose here too.

Did we agree on freezing the d presentation attribute to the definition of SVG 1.1? No support of the additional CSS property values?

IMO we should allow all values the property has + the string value itself for backwards compatibility reasons. But I am not sure if:

  1. We have discussed this in the WG already and resolved to not extend the grammar
  2. Chrome would support the additional CSS syntax on the attribute as well.

This comment has been minimized.

@dirkschulze

dirkschulze Nov 7, 2018

Contributor

Guess I commented here already. Still applies IMO.

This comment has been minimized.

@AmeliaBR

AmeliaBR Jan 7, 2019

Contributor

I think it would confuse parsing too much to to allow the path() notation inside the attribute.

That said, I don't see any reason to freeze the attribute at SVG 1.1 when it comes to path data commands. Instead, treat the attribute value as a string injected into a path() wrapper. If we ever do add other commands, they would be valid here, too.

@dirkschulze

This comment has been minimized.

Copy link
Contributor

dirkschulze commented Oct 27, 2018

@ewilligers I’d love to see this in SVG. At this point I wonder if we should defer it and look at adding this to the path spec as an extension to SVG2.0.

@dirkschulze dirkschulze added the Agenda+ label Oct 27, 2018

@AmeliaBR

This comment has been minimized.

Copy link
Contributor

AmeliaBR commented Oct 28, 2018

@dirkschulze Based on the resolutions we made in August (#320 (comment)), the revised spec would match one implementation (Chrome). Considering how eagerly anticipated it is by authors, I don't think it's appropriate to remove just yet.

(But @ewilligers, any chance you could update that PR so we can land it?)

@dirkschulze

This comment has been minimized.

Copy link
Contributor

dirkschulze commented Oct 28, 2018

@AmeliaBR I'd love to keep it but we need 2 implementations for transitioning the spec. Other implementations might be willing to implement it but this needs to happen before the transition. Lets discuss how we want to deal with features that have implementers feedback but no 2 implementations yet in our next WG call.

ericwilligers added some commits Dec 18, 2017

'd' presentation attribute: use path() function
This brings the spec, the web platform tests and the only
shipping implementation (Chrome) into consistency:

  none | path(<string>)

closes #320

@ewilligers ewilligers force-pushed the ewilligers:d-path-function branch from 3d8486a to 7d96fba Nov 5, 2018

@ewilligers

This comment has been minimized.

Copy link
Contributor Author

ewilligers commented Nov 5, 2018

@AmeliaBR I have updated the PR with the attribute specification.

@css-meeting-bot

This comment has been minimized.

Copy link
Member

css-meeting-bot commented Nov 5, 2018

The SVG Working Group just discussed `d` as a property (at-risk status).

The full IRC log of that discussion <AmeliaBR> Topic: `d` as a property (at-risk status)
<AmeliaBR> github: https://github.com//pull/374
<AmeliaBR> Eric: I've updated the PR to reflect different syntax for the attribute.
<AmeliaBR> Amelia: I'll try to take a look at that.
<AmeliaBR> Eric: Issue for today is, can we get a second implementation in time for PR?
<AmeliaBR> Tav: I'd be willing to do it for Inkscape.
<AmeliaBR> Eric: Are there any polyfills?
<AmeliaBR> Dirk, Tav: Not that we know of.
<AmeliaBR> Amelia: The main stumbling block for implementations in browsers, I think, was discrepancy between spec and shipped implementation in Chrome. Now that is sorted, we need to poke browser tracking issues again.
<AmeliaBR> Dirk: So, first step is to merge that PR to get the up-to-date spec.
<AmeliaBR> ... And then we can review status again before going to proposed Rec
<AmeliaBR> Resolved: Keep `d` as geometry property in SVG 2, merging the amendments previously agreed upon after review
<AmeliaBR> (discussion about difficulty implementing in Inkscape)
@Tavmjong

This comment has been minimized.

Copy link
Contributor

Tavmjong commented Nov 6, 2018

Inkscape master now supports the path() notation (for rendering).

@@ -231,6 +231,10 @@ <h3 id="TheDProperty">Specifying path data: the <span class='property'>'d'</span
If the path data string contains no valid commands, then the behavior
is the same as the <span class='prop-value'>none</span> value.</p>

<p>When <a>'d'</a> is parsed as an attribute, it is parsed according to the

This comment has been minimized.

@dirkschulze

dirkschulze Nov 7, 2018

Contributor

Could you be more specific what this means for the path() function please? I thing what you want to say is that the grammar would not allow path() and falls back to EBNF as specified in this spec.

This comment has been minimized.

@AmeliaBR

AmeliaBR Jan 7, 2019

Contributor

I agree that it could be more clear. Maybe:

Suggested change Beta
<p>When <a>'d'</a> is parsed as an attribute, it is parsed according to the
<p>When <a>'d'</a> is parsed as a <a>presentation attribute</a>,
the entire attribute value is parsed as if it was a `<string>` wrapped with `path()` function notation.
The string is then parsed according to the
<a href="#PathDataBNF">svg-path</a> <a href="types.html#syntax">EBNF grammar</a>,
and not as a sequence of CSS tokens.
</p>

This comment has been minimized.

@AmeliaBR

AmeliaBR Jan 7, 2019

Contributor

We also need to define the term "path data string" somewhere, since it's used a lot in the prose.

@dirkschulze dirkschulze removed the Agenda+ label Nov 13, 2018

@@ -177,7 +177,7 @@ <h3 id="TheDProperty">Specifying path data: the <span class='property'>'d'</span
</tr>
<tr>
<th>Value:</th>
<td>none | <a>&lt;string&gt;</a></td>
<td>none | path(<a>&lt;string&gt;</a>)</td>

This comment has been minimized.

@dirkschulze

dirkschulze Dec 2, 2018

Contributor

I wonder if we want to link path() (and maybe string as well).

This comment has been minimized.

@AmeliaBR

AmeliaBR Jan 7, 2019

Contributor

There might be a chicken-vs-egg issue with linking to another definition. The CSS reference definition for the path function is in the motion path module, which is still WD status, and which still references SVG 1.1 as the reference definition for the path syntax.

</p>

<p class="annotation">

This comment has been minimized.

@dirkschulze

dirkschulze Dec 2, 2018

Contributor

We let the annotation in the spec for later reference.

This comment has been minimized.

@AmeliaBR

AmeliaBR Jan 7, 2019

Contributor

Yes. It would also be helpful to extend the annotation to add a link to the resolution to use the path() syntax.

@dirkschulze

This comment has been minimized.

Copy link
Contributor

dirkschulze commented Dec 2, 2018

d is listed in the presentation attribute list already. It should also be added to https://svgwg.org/svg2-draft/propidx.html

Please check if the changes.html section needs updates.

@AmeliaBR
Copy link
Contributor

AmeliaBR left a comment

A few comments & suggested clarifications.

Sorry for how long it took to review... back to work for January!

@@ -177,7 +177,7 @@ <h3 id="TheDProperty">Specifying path data: the <span class='property'>'d'</span
</tr>
<tr>
<th>Value:</th>
<td>none | <a>&lt;string&gt;</a></td>
<td>none | path(<a>&lt;string&gt;</a>)</td>

This comment has been minimized.

@AmeliaBR

AmeliaBR Jan 7, 2019

Contributor

There might be a chicken-vs-egg issue with linking to another definition. The CSS reference definition for the path function is in the motion path module, which is still WD status, and which still references SVG 1.1 as the reference definition for the path syntax.

@@ -231,6 +231,10 @@ <h3 id="TheDProperty">Specifying path data: the <span class='property'>'d'</span
If the path data string contains no valid commands, then the behavior
is the same as the <span class='prop-value'>none</span> value.</p>

<p>When <a>'d'</a> is parsed as an attribute, it is parsed according to the

This comment has been minimized.

@AmeliaBR

AmeliaBR Jan 7, 2019

Contributor

I agree that it could be more clear. Maybe:

Suggested change Beta
<p>When <a>'d'</a> is parsed as an attribute, it is parsed according to the
<p>When <a>'d'</a> is parsed as a <a>presentation attribute</a>,
the entire attribute value is parsed as if it was a `<string>` wrapped with `path()` function notation.
The string is then parsed according to the
<a href="#PathDataBNF">svg-path</a> <a href="types.html#syntax">EBNF grammar</a>,
and not as a sequence of CSS tokens.
</p>
@@ -231,6 +231,10 @@ <h3 id="TheDProperty">Specifying path data: the <span class='property'>'d'</span
If the path data string contains no valid commands, then the behavior
is the same as the <span class='prop-value'>none</span> value.</p>

This comment has been minimized.

@AmeliaBR

AmeliaBR Jan 7, 2019

Contributor

We probably need to clarify how this fallback works in CSS terms.
Would this affect the "used value" or only the "actual value"?
Or should we match the terminology used in other shapes, e.g., "A computed value of zero for either dimension disables rendering of the element."?
(So it would be "A path data string that contains no valid commands disables rendering of the element.")

@@ -231,6 +231,10 @@ <h3 id="TheDProperty">Specifying path data: the <span class='property'>'d'</span
If the path data string contains no valid commands, then the behavior
is the same as the <span class='prop-value'>none</span> value.</p>

<p>When <a>'d'</a> is parsed as an attribute, it is parsed according to the

This comment has been minimized.

@AmeliaBR

AmeliaBR Jan 7, 2019

Contributor

We also need to define the term "path data string" somewhere, since it's used a lot in the prose.

</p>

<p class="annotation">

This comment has been minimized.

@AmeliaBR

AmeliaBR Jan 7, 2019

Contributor

Yes. It would also be helpful to extend the annotation to add a link to the resolution to use the path() syntax.


<div class='ready-for-wider-review'>

<h3 id="TheDAttribute">The <span class="attr-name">'d'</span> attribute</h3>

This comment has been minimized.

@AmeliaBR

AmeliaBR Jan 7, 2019

Contributor

Why was this section added?

I'd prefer to stick with defining both the property and attribute syntax in one place.
They are the same value, just with special parsing rules when the value is provided as a presentation attribute.

<td>no</td>
<td>N/A</td>
<td><a href="https://www.w3.org/TR/2008/REC-CSS2-20080411/media.html#visual-media-group">visual</a></td>
<td>yes</td>

This comment has been minimized.

@AmeliaBR

AmeliaBR Jan 7, 2019

Contributor

We've updated the Property Index to specify the type of animation, not just yes/no. There's also a "computed value" column. Which makes me realize that we never resolved #321 on what that should be (do you normalize before computed/used value?). For now:

Suggested change Beta
<td>yes</td>
<td>as path data string</td>
<td>as specified</td>

Ideally, "as path data string" would be linked to the definition of how animation works.

@@ -144,7 +144,11 @@ <h2 id="syntax">Attribute syntax</h2>
unitless length and angles to be used in presentation attribute while
disallowing them in corresponding property values.</p>

<p class="note">Note that all <a>presentation attributes</a>, since they are
<p>The <a>'d'</a> presentation attribute is an exception, it is parsed

This comment has been minimized.

@AmeliaBR

AmeliaBR Jan 7, 2019

Contributor

I think it would confuse parsing too much to to allow the path() notation inside the attribute.

That said, I don't see any reason to freeze the attribute at SVG 1.1 when it comes to path data commands. Instead, treat the attribute value as a string injected into a path() wrapper. If we ever do add other commands, they would be valid here, too.

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