Jonathan Watt [:jwatt] Reporter
	Description • 8 years ago

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.

	Jonathan Watt [:jwatt] Reporter
	Comment 1 • 8 years ago

(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.)

	Jonathan Watt [:jwatt] Reporter
	Comment 2 • 8 years ago

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.

	Jonathan Watt [:jwatt] Reporter
	Comment 3 • 8 years ago

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.)

	yisibl
	Comment 4 • 8 years ago

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

	Jonathan Watt [:jwatt] Reporter
	Comment 5 • 8 years ago

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

	Astley Chen (inactive)
	Comment 6 • 8 years ago

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.

	Boris Chiou [:boris] Assignee
	Comment 7 • 6 years ago

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.

	Boris Chiou [:boris] Assignee
	Comment 8 • 6 years ago

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

	Boris Chiou [:boris] Assignee
	Comment 9 • 6 years ago

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

	Boris Chiou [:boris] Assignee
	Comment 10 • 6 years ago

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

	Boris Chiou [:boris] Assignee
	Comment 11 • 6 years ago

Add a reftest for this.

Depends on D3636

	Boris Chiou [:boris] Assignee
	Comment 12 • 6 years ago

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

	Emilio Cobos Álvarez (:emilio)
	Comment 13 • 6 years ago

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.

	Emilio Cobos Álvarez (:emilio)
	Comment 14 • 6 years ago

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

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

	Jonathan Watt [:jwatt] Reporter
	Comment 15 • 6 years ago

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

Jonathan Watt [:jwatt] has approved the revision.

	Jonathan Watt [:jwatt] Reporter
	Comment 16 • 6 years ago

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.

	Jonathan Watt [:jwatt] Reporter
	Comment 17 • 6 years ago

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

Jonathan Watt [:jwatt] has approved the revision.

	Jonathan Watt [:jwatt] Reporter
	Comment 18 • 6 years ago

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

	Boris Chiou [:boris] Assignee
	Comment 19 • 6 years ago

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

Thanks for the review. :)

	Boris Chiou [:boris] Assignee
	Comment 20 • 6 years ago

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

	Pulsebot
	Comment 21 • 6 years ago

Pushed by boris.chiou@gmail.com:
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

	Arthur Iakab [arthur_iakab]
	Comment 22 • 6 years ago
bugherder

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

	Estelle Weyl
	Comment 23 • 6 years ago

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

	Boris Chiou [:boris] Assignee
	Comment 24 • 6 years ago • Edited

(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. :)