Closed Bug 1246764 Opened 4 years ago Closed Last year

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: jwatt, Assigned: boris)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete)

Attachments

(5 files)

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.
(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.)
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.
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.)
See Also: → basic-shape-ship
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.
Assignee: jwatt → nobody
See Also: basic-shape-ship
Assignee: nobody → boris.chiou
Status: NEW → ASSIGNED
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.
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
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
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
Add a reftest for this.

Depends on D3636
Attachment #9002037 - Attachment description: Bug 1246764 - Part 1: Move SVGPathData and its parser into svg_path.rs (only movement). → Bug 1246764 - Part 1: Move SVGPathData and its parser into svg_path.rs.
Depends on: 1429298
Depends on: 1484316
I will rebase those patches and send review requests after we land Bug 1429298.
Attachment #9002042 - Attachment description: Bug 1246764 - Part 4: Rename shouldApplyBasicShape to shouldApplyBasicShapeOrPath. → Bug 1246764 - Part 4: Rename mask flag and function name of xxxBasicShape to xxxBasicShapeOrPath.
Attachment #9002043 - Attachment description: Bug 1246764 - Part 5: Test. → Bug 1246764 - Part 5: Tests.
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.
Attachment #9002037 - Flags: review+
Comment on attachment 9002039 [details]
Bug 1246764 - Part 2: Define path() for clip-path.

Emilio Cobos Álvarez (:emilio) has approved the revision.
Attachment #9002039 - Flags: review+
Comment on attachment 9002041 [details]
Bug 1246764 - Part 3: Layout part for |clip-path: path()|.

Jonathan Watt [:jwatt] has approved the revision.
Attachment #9002041 - Flags: review+
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.
Attachment #9002042 - Flags: review+
Comment on attachment 9002043 [details]
Bug 1246764 - Part 5: Tests.

Jonathan Watt [:jwatt] has approved the revision.
Attachment #9002043 - Flags: review+
I'm very happy to see this being implemented. Thanks, 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. :)
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
Blocks: 1487838
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
(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. :)
You need to log in before you can comment on or make changes to this bug.