Paths

Table of Contentst

Differential D202882

Bug 1823463 - Support shape() for clip-path property in style.
ClosedPublic
Actions

Authored by boris on Feb 27 2024, 11:01 PM.

Details

Reviewers

emilio

Commits

rMOZILLACENTRAL7f73d7224fdc: Bug 1823463 - Support shape() for clip-path property in style. r=emilio

Bugzilla Bug ID

1823463

Summary

Implement the style part for shape(). Besides, update some issues in the
test file, e.g. avoid using viewport height so we get the fixed result
on different devices.

I will refactor PathCommand to let it be a specialization of
GenericShapeCommand in the following path.

Diff Detail

Repository

rMOZILLACENTRAL mozilla-central

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Build Status

Buildable 640534
Build 739204: arc lint + arc unit

Event Timeline

wip

Herald added a project: secure-revision. · View Herald TranscriptFeb 27 2024, 11:01 PM

Harbormaster completed remote builds in B631968: Diff 828070.Feb 27 2024, 11:01 PM

boris added a child revision: D202883: Bug 1823463 - Make PathCommand as the specialization of GenericShapeCommand..Feb 27 2024, 11:01 PM

phab-bot published this revision for review.Feb 27 2024, 11:01 PM

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".

phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".

phab-bot removed a project: secure-revision.

Herald added subscribers: zrhoffman, emilio. · View Herald TranscriptFeb 27 2024, 11:01 PM

Code analysis found 1 defect in diff 828070:

1 defect found by eslint (Mozlint)

IMPORTANT: Found 1 defect (error level) that must be fixed before landing.

You can run this analysis locally with:

./mach lint --warnings --outgoing

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 828070.

wip

boris retitled this revision from Bug 1823463 - Support shape() for clip-path property in style (wip). to Bug 1823463 - Support shape() for clip-path property in style..Mar 7 2024, 10:38 PM

boris edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B636207: Diff 833111.Mar 7 2024, 10:38 PM

Code analysis found 1 defect in diff 833111:

1 defect found by eslint (Mozlint)

1 defect closed compared to the previous diff 828070.

IMPORTANT: Found 1 defect (error level) that must be fixed before landing.

You can run this analysis locally with:

./mach lint --warnings --outgoing

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 833111.

boris updated this revision to Diff 833573.Mar 8 2024, 8:49 PM

boris edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B636619: Diff 833573.Mar 8 2024, 8:49 PM

1 defect closed compared to the previous diff 833111.

If you see a problem in this automated review, please report it here.

Fix test failures

Harbormaster completed remote builds in B636706: Diff 833679.Mar 8 2024, 11:22 PM

boris added reviewers: layout-reviewers, Restricted Project.Mar 11 2024, 7:34 PM

It's a lot of code, but I can't think of a way of making it less... That said, it looks great, I have only nits. Thanks!

servo/components/style/values/generics/basic_shape.rs
598	Maybe it'd be more convenient (from the style system perspective) to store this as: pub fill: FillRule, pub from: CoordPair<..>, pub commands: ..., But I'm assuming having all the commands in a list would be convenient?
servo/components/style/values/specified/basic_shape.rs
510	maybe `expect_comma`?
767	This is rather slow. Maybe `let mut commands` instead of `let mut following_commands`, and then `commands.insert(0, ShapeCommand::Move { .. })`? Or maybe even remove `let starting_point` and do: let mut first = true; let commands = input.parse_comma_separated(\|i\| { if first { first = false; Ok(ShapeCommand::Move { by_to..., point: CoordinatePair::parse(context, input)?, }) } else { ShapeCommand::parse(context, i) } })?; Though then you need to manually check that `commands.len() != 1`. Your call, but let's at least do the insert().
785	`Ok(try_match_ident_... {})` then remove the `Ok()` from the branches.
829	Maybe just `if input.try_parse(..).is_ok() {` and deindent.
875	Just remove this and move the continue above? if rotate.is_none() && input.try_parse(..) { rotate = Some(Angle::parse(..)?); continue; }
testing/web-platform/tests/css/css-masking/animations/clip-path-interpolation-shape.html
159	Why this change?

This revision is now accepted and ready to land.Mar 12 2024, 8:37 PM

boris added inline comments.Mar 13 2024, 7:32 PM

servo/components/style/values/generics/basic_shape.rs
598	Yes. This makes us easier to build the `gfx::Path` with a list in Gecko, either to render this clip-path or to compute the motion path. We share the same code to build `gfx::Path` for both `path()` and `shape()`, so using a list can avoid doing redundant things there.
testing/web-platform/tests/css/css-masking/animations/clip-path-interpolation-shape.html
159	Using `vh` causes a different result on my local machine (i.e. mac), so I changed it to px to avoid getting the different viewport height on different platforms, just in case.