1246764 - Add support for |clip-path: path()|

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jonathan Watt [:jwatt]]

Bug 1075457 adds support for basic shape clip paths. Currently the only means of specifying a path in the spec is to specify a polygon, but this isn't sufficient for our Firefox frontend needs where we need to be able to clip to a curved path for tabs. We could add support for a |path()| value that supports the SVG path syntax. We should likely make that accessible only to chrome until/unless it is added to the specification.

Comment 1

[Jonathan Watt [:jwatt]]

(What the Firefox frontend currently does is reference an SVG <clipPath> element, but that requires the instantiation of an SVG document which has significant overhead.)

Comment 2

[Jonathan Watt [:jwatt]]

Implementing this would involve the following:

 * Extend CSSParserImpl::ParseBasicShape

 * Add a call to nsCSSValue::AppendToString similar to the
   AppendPolygonToString call

 * Add handling in nsComputedDOMStyle::CreatePrimitiveValueForClipPath
   similar to the nsStyleBasicShape::Type::ePolygon handling

 * Add handling in nsRuleNode::SetStyleClipPathToCSSValue similar to
   the eCSSKeyword_polygon handling

 * Add a define similar to #define NS_STYLE_BASIC_SHAPE_POLYGON in
   nsStyleConsts.h

 * Add a CreateClipPathPath method to nsCSSClipPathInstance

As far as creating a Moz2D path goes, we currently have the ability to parse a string to a Path using code along the lines of:

  SVGPathData pathData;
  nsSVGPathDataParser pathParser(pathString, &pathData);
  pathParser.Parse();
  if (!pathData.Length()) {
    return;
  }
  RefPtr<Path> path = pathData.BuildPath(builder, NS_STYLE_STROKE_LINECAP_BUTT, 0);

It seems unlikely that we'd want to store the path data in style as a string, but this is a useful starting point.

Comment 3

[Jonathan Watt [:jwatt]]

In fact we have SVGContentUtils::GetPath to parse a string to a Path. (Without digging into the callers I'm not sure why that's passing a non-zero stroke width to pathData.BuildPath though.)

Comment 4

[yisibl]

Specification: https://drafts.csswg.org/css-shapes-2/#supported-basic-shapes

Comment 5

[Jonathan Watt [:jwatt]]

More specifically: https://drafts.csswg.org/css-shapes-2/#funcdef-path

Comment 6

[Astley Chen (inactive)]

Given that it's part of features in CSS Shape 2 spec, I think we need to initiate the spec(CSS Masking 1 or 2?) discussion before going further.

Comment 7

[Boris Chiou [:boris]]

Attachment: Bug 1246764 - Part 1: Move SVGPathData and its parser into svg_path.rs.

SVGPathData will be used by clip-path and offset-path (and/or more on the
properties which support <basic-shape>). Therefore, let's move
SVGPathData out of motion.rs.

Comment 8

[Boris Chiou [:boris]]

Attachment: Bug 1246764 - Part 2: Define path() for clip-path.

For now, |clip-path: path()| is chrome-only, and not for shape-outside,
so we only implement the parser for clip-path. Besides, I didn't put
path() in BasicShape because path() doesn't use the reference box to
resolve the percentage or keywords (i.e. SVG path only accept floating
point or integer number as the css pixel value). Therefore, I add it into
ShapeSource, instead of BasicShape.

Depends on D3631

Comment 9

[Boris Chiou [:boris]]

Attachment: Bug 1246764 - Part 3: Layout part for |clip-path: path()|.

Create clip-path for the path function and reuse some APIs in
nsCSSClipPathInstance, so we don't have to update the code flow.

Depends on D3633

Comment 10

[Boris Chiou [:boris]]

Attachment: Bug 1246764 - Part 4: Rename mask flag and function name of xxxBasicShape to xxxBasicShapeOrPath.

This flag is used for both basic shapes and path function, so rename it.
path() is in css-shape-2, but this spec is still a draft version,
so I intentionally don't make path() be one of the basic shapes.

Depends on D3635

Comment 11

[Boris Chiou [:boris]]

Attachment: Bug 1246764 - Part 5: Tests.

Add a reftest for this.

Depends on D3636

Comment 12

[Boris Chiou [:boris]]

I will rebase those patches and send review requests after we land Bug 1429298.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9002037 [details]
Bug 1246764 - Part 1: Move SVGPathData and its parser into svg_path.rs.

Emilio Cobos Álvarez (:emilio) has approved the revision.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9002039 [details]
Bug 1246764 - Part 2: Define path() for clip-path.

Emilio Cobos Álvarez (:emilio) has approved the revision.

Comment 15

[Jonathan Watt [:jwatt]]

Comment on attachment 9002041 [details]
Bug 1246764 - Part 3: Layout part for |clip-path: path()|.

Jonathan Watt [:jwatt] has approved the revision.

Comment 16

[Jonathan Watt [:jwatt]]

Comment on attachment 9002042 [details]
Bug 1246764 - Part 4: Rename mask flag and function name of xxxBasicShape to xxxBasicShapeOrPath.

Jonathan Watt [:jwatt] has approved the revision.

Comment 17

[Jonathan Watt [:jwatt]]

Comment on attachment 9002043 [details]
Bug 1246764 - Part 5: Tests.

Jonathan Watt [:jwatt] has approved the revision.

Comment 18

[Jonathan Watt [:jwatt]]

I'm very happy to see this being implemented. Thanks, Boris!

Comment 19

[Boris Chiou [:boris]]

(In reply to Jonathan Watt [:jwatt] from comment #18)
> I'm very happy to see this being implemented. Thanks, Boris!

Thanks for the review. :)

Comment 20

[Boris Chiou [:boris]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cc89a253f7d6854d219b8e361da6f17bbc45a9d

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/53cc417083ff
Part 1: Move SVGPathData and its parser into svg_path.rs. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdd09f26a2b0
Part 2: Define path() for clip-path. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcdbd9f8b52d
Part 3: Layout part for |clip-path: path()|. r=jwatt
https://hg.mozilla.org/integration/mozilla-inbound/rev/fff761953caf
Part 4: Rename mask flag and function name of xxxBasicShape to xxxBasicShapeOrPath. r=jwatt
https://hg.mozilla.org/integration/mozilla-inbound/rev/21b885b5fc62
Part 5: Tests. r=jwatt

Comment 22

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/53cc417083ff
https://hg.mozilla.org/mozilla-central/rev/cdd09f26a2b0
https://hg.mozilla.org/mozilla-central/rev/bcdbd9f8b52d
https://hg.mozilla.org/mozilla-central/rev/fff761953caf
https://hg.mozilla.org/mozilla-central/rev/21b885b5fc62

Comment 23

[Estelle Weyl]

In terms of docs, 

https://developer.mozilla.org/en-US/docs/Web/CSS/clip-path was updated, including updates to definition of <basic-shape>, compat table, interactive example, list of examples, and live example.

The basic shape definition page at https://developer.mozilla.org/en-US/docs/Web/CSS/basic-shape was edited, with the addition of 
an Interactive example and updated compat table.

The shape-outside property was not updated: https://developer.mozilla.org/en-US/docs/Web/CSS/shape-outside
Note: path() is not yet supported as a value for shape-outside.

The offset-path property https://developer.mozilla.org/en-US/docs/Web/CSS/offset-path was updated, with a move of path() from function to basic shape and the syntax box has been changed to reflect that. I do not see a bug for this. https://bugzilla.mozilla.org/show_bug.cgi?id=shape-outside is the closest thing I could find.

CSS 'd' property for SVG
https://codepen.io/estelle/pen/YOvMpN
works in Chrome but not FF. The d property doesn't seem to be documented anywhere on MDN (or anywhere at all?). Another example: https://codepen.io/chriscoyier/pen/NRwANp. Is this under consideration?

There are still outstanding Pull requests. Links of those maintained at https://github.com/mdn/sprints/issues/441

Comment 24

[Boris Chiou [:boris]]

(In reply to Estelle Weyl from comment #23)
> The shape-outside property was not updated:
> https://developer.mozilla.org/en-US/docs/Web/CSS/shape-outside
> Note: path() is not yet supported as a value for shape-outside.

We don't support this for now. Thanks.

> 
> The offset-path property
> https://developer.mozilla.org/en-US/docs/Web/CSS/offset-path was updated,
> with a move of path() from function to basic shape and the syntax box has
> been changed to reflect that. I do not see a bug for this.
> https://bugzilla.mozilla.org/show_bug.cgi?id=shape-outside is the closest
> thing I could find.

Actually, the syntax of path() in offset-path is a little bit different from the syntax of path() in basic-shapes-2. In other words, |offset-path:path()| doesn't have the <fill-rule>. Maybe the spec will be updated in the future. For now, probably we should not move the path() into basic-shape for offset-path property. (However, it's fine to keep the update if you want because |offset-path:path()| is just a simplified variant of path() in basic-shapes2. Either way is ok to me.)
BTW, after implementing offset-distance, we will see the correct Live Result. :) 

> 
> CSS 'd' property for SVG
> https://codepen.io/estelle/pen/YOvMpN
> works in Chrome but not FF. The d property doesn't seem to be documented
> anywhere on MDN (or anywhere at all?). Another example:
> https://codepen.io/chriscoyier/pen/NRwANp. Is this under consideration?

It seems this is a bug in Firefox because d property is defined in SVG2, and looks like we haven't implemented it, I think.
I didn't update this part, and maybe we should fix this later.

Thanks for updating this. :)