Skip to content

FF97 CSS basic-shape path() function enabled by default #14498

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

Merged
merged 10 commits into from
Feb 28, 2022

Conversation

hamishwillee
Copy link
Contributor

FF97 ships the bug Ship d property and make SVG path d attribute as the presentation attribute which was implemented behind pref in FF91 https://bugzilla.mozilla.org/show_bug.cgi?id=1340422

My understanding is that essentially this means you can now call path() on the d property of an SVG path element in CSS. There is a cool little demo here: https://codepen.io/airen/pen/wgNEQo

So what I have done here is for path() added a NOTE about the introduction of the feature in FF91 and a new version for FF97 that removes the preference. For FF android the preference does not work from FF79 so I just put that as the end version.

NOTE: I am not 100% certain about the partial implementation, but I will query that separately. This could go in as "no worse".

NOTE: Chrome also supports this in CSS now too, so that would have to be tested.


As a separate PR to follow ...

In addition, I THINK it would be useful to update the d attribute to indicate that from FF97 it is now a "presentation attribute", which is what allows it to be modified in CSS and set to the path() or none. Not sure right way to do this - subfeature "supports_css_path()" or similar. Or would we do this as a "reversed" one - e.g. not_supports_css_path. Note that current chrome does do the right thing. @ddbeck Advice?

Further, any reason not to add these spec URLs that are currently manually in the page here https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/d#specifications ?

@github-actions github-actions bot added the data:css Compat data for CSS features. https://developer.mozilla.org/docs/Web/CSS label Jan 11, 2022
@hamishwillee hamishwillee mentioned this pull request Jan 11, 2022
7 tasks
Copy link
Contributor

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

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

In addition, I THINK it would be useful to update the d attribute to indicate that from FF97 it is now a "presentation attribute", which is what allows it to be modified in CSS and set to the path() or none. Not sure right way to do this - subfeature "supports_css_path()" or similar. Or would we do this as a "reversed" one - e.g. not_supports_css_path. Note that current chrome does do the right thing. @ddbeck Advice?

I'm not sure, honestly. I don't know SVG that well. What kind of attribute was it before? Was it specified as that? Might be easier to have this discussion on a draft PR, though, since you've already got an idea and we may as well start from there.

Further, any reason not to add these spec URLs that are currently manually in the page here https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/d#specifications ?

None that I can see. Proceed!

One extra thing: the MDN URL points to a now-non-existent fragment. I'd like to see that go away.

@hamishwillee
Copy link
Contributor Author

hamishwillee commented Jan 14, 2022

@ddbeck This was all messed up, and what I did was not helping. The story is:

  1. v63 added support for path() function for offset-path - https://bugzilla.mozilla.org/show_bug.cgi?id=1429298
  2. v64 added support for path() function for clip-path behind preference layout.css.clip-path-path.enabled - https://bugzilla.mozilla.org/show_bug.cgi?id=1487838
  3. v71 shipped clip-path (set pref layout.css.clip-path-path.enabled was set true.) https://bugzilla.mozilla.org/show_bug.cgi?id=1488530 - ie pref layout.css.clip-path-path.enabled was set true.
  4. v91 allows path() to be set on d SVG presentation attribute behind the preference layout.css.d-property.enabled - https://bugzilla.mozilla.org/show_bug.cgi?id=1340422
  5. v97 ships the d thingy - sets layout.css.d-property.enabled true all the time. https://bugzilla.mozilla.org/show_bug.cgi?id=1744599

So I think the problem here is that the specific support should be done as subfeatures of path in this structure:

basic-shape
   path
     offset-path CSS property
     clip-path CSS property
     d SVG attribute

If you agree, would appreciate your advice on the names and descriptions for each of those new properties.

Am setting back to draft to make it clear this is just not right!

@hamishwillee hamishwillee marked this pull request as draft January 14, 2022 06:08
@ddbeck
Copy link
Contributor

ddbeck commented Jan 18, 2022

Ugh, this is a bit messy; I think it's an artifact of how we've recorded CSS functions in BCD (which is to say, abstractly—like mixins, instead of concrete interfaces). I think you've got the right idea, but perhaps we need to go a step further:

  • Use support statements on path() feature to show partial support, with notes showing the introduction of each supported property (basically, continue down the path started in this PR)

  • Add/update/reconcile subfeatures of

    • css.properties.clip-path
    • css.properties.offset-path
    • svg.elements.path.d

    to show support for path() on those specific items. Some of them already have a path() subfeature, so it's just a matter of making sure the numbers are consistent.

Verified

This commit was signed with the committer’s verified signature.

Verified

This commit was signed with the committer’s verified signature.

Verified

This commit was signed with the committer’s verified signature.
esheppa Eric Sheppard
@github-actions github-actions bot added the data:svg Compat data for SVG features. https://developer.mozilla.org/docs/Web/SVG label Jan 31, 2022
@hamishwillee
Copy link
Contributor Author

hamishwillee commented Jan 31, 2022

@ddbeck Re #14498 (comment) - what I have done/questions:

  1. css.properties.offset-path - was already correct.
  2. css.properties.clip-path - updated to match bugzilla version and match FF desktop and android builds to closest versions
  3. svg.elements.path.d - added svg.element.path.d.path - Chrome and Safari versions tested on browserstack. Everything else mirrored from Chrome.
  4. Added all the information about version support/preferences as notes for basic-shapes BCD. Not entirely sure of this because by doing this we can't have the original flags information (as there are a bunch of flags that change this and they affect different versions).
  5. The chrome info for basic-shape.path says ""notes": "Only supported on the offset-path property."" but I know that this is also supported from v52 on d. Perhaps add a note for that too in the same way as for firefox?
  6. As I understand this comment, svg.element.path.d should, according to the spec, be able to set either path() or a string. There is talk of a change to make it only support path(), which is what both FF and Chrome do, and what the changes I have made here show. But we don't show the "lack" of support for a string. The right way to do that would be to add a similar "supportsString" feature. I propose we omit doing anything for that until someone implements it or the spec settles.

@hamishwillee hamishwillee marked this pull request as ready for review January 31, 2022 01:29
@hamishwillee hamishwillee requested a review from ddbeck January 31, 2022 23:21
Copy link
Contributor

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

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

More suggestions for you, @hamishwillee. This has been a tough one. Forgive me. 🙏

@hamishwillee
Copy link
Contributor Author

Thanks for your help @ddbeck . I integrated your suggestions. If you wander through the file view you will see the response to your question and a couple of extra "notes". Ready for re-review

@hamishwillee hamishwillee requested a review from ddbeck February 11, 2022 05:41
Copy link
Contributor

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

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

Getting pretty close here. Thanks again, Hamish!

Co-authored-by: Daniel D. Beck <daniel@ddbeck.com>
@hamishwillee
Copy link
Contributor Author

@ddbeck I think "this" is ready. Your question answered in https://github.com/mdn/browser-compat-data/pull/14498/files#r809627789

Co-authored-by: Daniel D. Beck <daniel@ddbeck.com>
@hamishwillee hamishwillee requested a review from ddbeck February 27, 2022 23:41
@hamishwillee
Copy link
Contributor Author

I merged your commit. Should be good to go now.

Copy link
Contributor

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you! 🎉

@ddbeck ddbeck merged commit f185e9c into mdn:main Feb 28, 2022
@hamishwillee hamishwillee deleted the ff97css_shape branch February 28, 2022 23:32
@hamishwillee
Copy link
Contributor Author

Thanks @ddbeck .- feels like a long time coming!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:css Compat data for CSS features. https://developer.mozilla.org/docs/Web/CSS data:svg Compat data for SVG features. https://developer.mozilla.org/docs/Web/SVG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants