1429298 - Implement path() function for offset-path

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Boris Chiou [:boris]]

In CSS Motion Path, we support different types of offset-path [1]:
1. none
2. path()
3. ray()
4. <basic-shape>
5. <geometry-box>
6. <url>

In this bug, we want to support "none" and "path()" [2] first because we could reuse the implementation of SVG path. However, I'd like to let offset-path non-animatable in this bug, and we could fix it after finishing path(), ray(), and <basic-shape> on offset-path.

Besides, we don't need to render offset-path. offset-path defines a path, so we could move an element along this path by using offset-distance. i.e. offset-path is used after we implement offset-distance.

[1] https://drafts.fxtf.org/motion-1/#offset-path-property
[2] https://drafts.fxtf.org/motion-1/#offsetpath-pathfunc

Comment 1

[Boris Chiou [:boris]]

Oh, in this bug, we have to implement SVGPathData [1] and its parser in Rust for stylo. :(

[1] https://searchfox.org/mozilla-central/source/dom/svg/SVGPathData.h

Comment 2

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #1)
> Oh, in this bug, we have to implement SVGPathData [1] and its parser in Rust
> for stylo. :(
> 
> [1] https://searchfox.org/mozilla-central/source/dom/svg/SVGPathData.h

SVG path spec: https://www.w3.org/TR/SVG11/paths.html

Comment 3

[Brian Birtles (:birtles)]

There may be corrections in SVG 2, I can't remember: https://svgwg.org/svg2-draft/paths.html#PathData

But you might need to be careful to ignore the bearing commands there -- I don't think anyone implements those.

Comment 4

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #3)
> There may be corrections in SVG 2, I can't remember:
> https://svgwg.org/svg2-draft/paths.html#PathData
> 
> But you might need to be careful to ignore the bearing commands there -- I
> don't think anyone implements those.

Thanks. I will follow this one. :)

Comment 5

[Robert Longson [:longsonr]]

The SVG 2 path spec is pretty buggy. It only allows whole number values for instance. Much better to implement SVG 1.1, that's what the Firefox parser does.

Comment 6

[Boris Chiou [:boris]]

OK. I see. I will check what the Firefox parser does and the detail of both specs. Thanks. :)

Comment 7

[Boris Chiou [:boris]]

Current wip [1], but many debug codes and need to clean it up. Feel free to write your own. Thanks.

[1] https://github.com/BorisChiou/gecko-dev/commits/motion/offset-path/wip

Comment 8

[Brian Birtles (:birtles)]

Thank you Boris!!

Comment 9

[Brian Birtles (:birtles)]

\o/

Comment 10

[Boris Chiou [:boris]]

Attachment: Bug 1429298 - Implement motion-path with path().

1. Define the preference for motion-path.
2. Define offset-path property.
3. Implement the parser for moveto and lineto

Comment 11

[Boris Chiou [:boris]]

Attachment: Bug 1429298 - Part 1: Define the preference for motion-path.

Define the preference and only enable it for debug usage.

Comment 12

[Boris Chiou [:boris]]

Attachment: Bug 1429298 - Part 2: Define offset-path property.

Define offset-path property in CSS.

Depends on D2168

Comment 13

[Boris Chiou [:boris]]

Attachment: Bug 1429298 - Part 3: Update path parser for move-to and line-to.

Implement SVG Path parser for move-to and line-to commands.

Depends on D2171

Comment 14

[Boris Chiou [:boris]]

Attachment: Bug 1429298 - Part 4: Implement other commands.

Implement the rest part of SVG Path parser.

Depends on D2172

Comment 15

[Boris Chiou [:boris]]

Attachment: Bug 1429298 - Part 5: Use macro for path parser.

There are a lot of duplicates in the parser, so we use macro to refine them.

Depends on D2173

Comment 16

[Boris Chiou [:boris]]

Still working on this for c++ side and bindings.

Comment 17

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #16)
> Still working on this for c++ side and bindings.

And it's better to merge SVGPath into ShapeSource, so we could reuse other types for offset-path in the future.

Comment 18

[Boris Chiou [:boris]]

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

Comment 19

[Boris Chiou [:boris]]

Attachment: Bug 1429298 - Part 1: Define the preference for motion-path.

Define the preference. I will enable it only for debug usage and test
coverage in a different patch.

Comment 20

[Boris Chiou [:boris]]

Attachment: Bug 1429298 - Part 2: Define offset-path and implement it in style system.

Define OffsetPath & SVGPathData on the servo-side, and StyleMotion &
StyleSVGPath on the gecko-side. We parse the SVG Path string into a
vector of PathCommand, and then encode this vector into an array of
float, so we could reuse the code segment of mozilla::SVGPathData::BuildPath()
to build the gfx::Path, and then use the gfx::Path to calculate the
motion path transform.

The basic flow is (from top to bottom):
* SVG Path String (CSS input)
       |
    (parse)
       |
* SVGPathData (in Rust)
       |
    (encode/decode)
       |
* Vec<f32> (only for binding, in Rust)
       |
    (binding, Rust <-> C++)
       |
* nsTArray<float> (stored in mozilla::StyleSVGPath, in C++)
       |
       |
* gfx::Path (build the path in layout, C++)

Finally, we use the gfx::Path to create a motion path transform.
The layout implementation is in the later patch.

The encoder and decoder are a little bit complicated, so I will implement it
in a later patch. The serialization in getComputedStyle will be
implemented later as well.

Besides, I will use macro to reduce the duplicate codes of the parser.


Depends on D2962

Comment 21

[Boris Chiou [:boris]]

Attachment: Bug 1429298 - Part 3: Implement the conversion between SVGPathData (Servo) and StyleSVGPath (Gecko).

We encode the SVGPathData into Vec<f32>, and then store it
in mozilla::StyleSVGPath.
The opposite direction is we decode nsTArray<float> into SVGPathData
(in clone_offset_path()).

The encoder has some patterns to use macro, so I will use macro to reduce
it in the later patch.


Depends on D2963

Comment 22

[Boris Chiou [:boris]]

Attachment: Bug 1429298 - Part 4: Implement the serialization of getComputedStyle.

Add an FFI to serialize the SVG path, so we could reuse to_css, and
the serializations of computed value and specified value would be the same.

Depends on D2964

Comment 23

[Boris Chiou [:boris]]

Attachment: Bug 1429298 - Part 3: Use macro for path parser.

There are a lot of duplicates, so we use macro to refine them.

Depends on D2965

Comment 24

[Boris Chiou [:boris]]

Attachment: Bug 1429298 - Part 5: Implement BuildPath for offset-path.

We would like to reuse the BuildPath code for <offset-path>, so move it
out of mozilla::SVGPathData and put in nsSVGUtils.

Depends on D2966

Comment 25

[Boris Chiou [:boris]]

Attachment: Bug 1429298 - Part 6: Apply motion path transform matrix.

We implement the layout part of offset-path. Now we don't have
offset-distance, so use the default value, 0%, for it.

Note: Drop mCombinedTransform from nsStyleDisplay, and move the
combination code into nsLayoutUtils. However, a better location is in
ReadTransform because the combination is redundant for generating the
matrix.

Depends on D2967

Comment 26

[Boris Chiou [:boris]]

Attachment: Bug 1429298 - Part 7: Tests.

In wpt, now we support "offset-path: none | path()", so parsing none or
path function should be correct. Animations which animate "from none"
or "to none" will pass because we could serialize "none", even if we
don't support animations on offset-path.

Depends on D2968

Comment 27

[Boris Chiou [:boris]]

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

Comment 28

[Boris Chiou [:boris]]

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

Comment 29

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8998638 [details]
Bug 1429298 - Part 2: Define offset-path and implement it in style system.

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

Comment 30

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8998641 [details]
Bug 1429298 - Part 3: Use macro for path parser.

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

Comment 31

[Nicolas Silva [:nical]]

Hey Boris, I'm at a conference and I'd rather wrap my head around this early next week, unless you need this reviewed soon (need-info me if you do and I'll do my best to squeeze that between sessions).

Comment 32

[Boris Chiou [:boris]]

(In reply to Nicolas Silva [:nical] from comment #31)
> Hey Boris, I'm at a conference and I'd rather wrap my head around this early
> next week, unless you need this reviewed soon (need-info me if you do and
> I'll do my best to squeeze that between sessions).

Sure. No hurry, so please take your time. It's ok to review this after your conference. Thanks. :)

Comment 33

[Boris Chiou [:boris]]

For now, we define `enum PathCommand` to store the commands of SVGPath. However, the tagged union may waste a lot of memory, and that's why gecko uses an encoded array of float. A possible way is to store the path as an array of f32 after we parse it in Rust, and decode it only for serialization, animations and building a gfx path. I would try this in a different bug (e.g. Bug 1246764 or a new filed bug).

Comment 34

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Chiou [:boris] from comment #33)
> For now, we define `enum PathCommand` to store the commands of SVGPath.
> However, the tagged union may waste a lot of memory, and that's why gecko
> uses an encoded array of float. A possible way is to store the path as an
> array of f32 after we parse it in Rust, and decode it only for
> serialization, animations and building a gfx path. I would try this in a
> different bug (e.g. Bug 1246764 or a new filed bug).

If we care a lot about memory usage of the path, which is reasonable I guess, it could make sense to store it as a refcounted encoded array (Arc<[f32]> or such), and just bump the refcount when passing it down to style structs.

Comment 35

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #34)
> (In reply to Boris Chiou [:boris] from comment #33)
> > For now, we define `enum PathCommand` to store the commands of SVGPath.
> > However, the tagged union may waste a lot of memory, and that's why gecko
> > uses an encoded array of float. A possible way is to store the path as an
> > array of f32 after we parse it in Rust, and decode it only for
> > serialization, animations and building a gfx path. I would try this in a
> > different bug (e.g. Bug 1246764 or a new filed bug).
> 
> If we care a lot about memory usage of the path, which is reasonable I
> guess, it could make sense to store it as a refcounted encoded array
> (Arc<[f32]> or such), and just bump the refcount when passing it down to
> style structs.

Or maybe Arc<[PathCommand]> is good enough, and avoids the encoding / decoding, which could be nicer.

Comment 36

[Boris Chiou [:boris]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #35)
> > If we care a lot about memory usage of the path, which is reasonable I
> > guess, it could make sense to store it as a refcounted encoded array
> > (Arc<[f32]> or such), and just bump the refcount when passing it down to
> > style structs.
> 
> Or maybe Arc<[PathCommand]> is good enough, and avoids the encoding /
> decoding, which could be nicer.

At least we could use Arc to reduce the memory copy. For serialization and building gfx::Path, probably we don't really need to decode (if using Arc[f32]). Maybe only animation interpolation needs more things to do if using Arc[f32]. I will keep tracking this.

Comment 37

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Chiou [:boris] from comment #36)
> (In reply to Emilio Cobos Álvarez (:emilio) from comment #35)
> > > If we care a lot about memory usage of the path, which is reasonable I
> > > guess, it could make sense to store it as a refcounted encoded array
> > > (Arc<[f32]> or such), and just bump the refcount when passing it down to
> > > style structs.
> > 
> > Or maybe Arc<[PathCommand]> is good enough, and avoids the encoding /
> > decoding, which could be nicer.
> 
> At least we could use Arc to reduce the memory copy. For serialization and
> building gfx::Path, probably we don't really need to decode (if using
> Arc[f32]). Maybe only animation interpolation needs more things to do if
> using Arc[f32]. I will keep tracking this.

Yeah, interpolation and serialization are the things that make us do more work. I think I'd prefer Arc because it means most of the memory win, no encoding during either parsing or computation, no decoding when serializing or interpolating, and nicer code.

Comment 38

[Boris Chiou [:boris]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #37)
> > At least we could use Arc to reduce the memory copy. For serialization and
> > building gfx::Path, probably we don't really need to decode (if using
> > Arc[f32]). Maybe only animation interpolation needs more things to do if
> > using Arc[f32]. I will keep tracking this.
> 
> Yeah, interpolation and serialization are the things that make us do more
> work. I think I'd prefer Arc because it means most of the memory win, no
> encoding during either parsing or computation, no decoding when serializing
> or interpolating, and nicer code.

Cool. Thanks for the feedback. Let me file a bug for using Arc. :)

Comment 39

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8998644 [details]
Bug 1429298 - Part 7: Tests.

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

Comment 40

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8998637 [details]
Bug 1429298 - Part 1: Define the preference for motion-path.

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

Comment 41

[Nicolas Silva [:nical]]

Comment on attachment 8998643 [details]
Bug 1429298 - Part 6: Apply motion path transform matrix.

Nicolas Silva [:nical] has approved the revision.

Comment 42

[Jonathan Watt [:jwatt]]

Comment on attachment 8998642 [details]
Bug 1429298 - Part 5: Implement BuildPath for offset-path.

Jonathan Watt [:jwatt] has approved the revision.

Comment 43

[Boris Chiou [:boris]]

Attachment: Bug 1429298 - Part 4: Rename builder as aBuilder in SVGPathData.cpp.

Follow the rule of naming for the function parameters.

Depends on D2966

Comment 44

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 9002929 [details]
Bug 1429298 - Part 4: Rename builder as aBuilder in SVGPathData.cpp.

Ting-Yu Lin [:TYLin] (UTC-7) has approved the revision.

Comment 45

[Boris Chiou [:boris]]

final try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=25828ab13b65d6f54f489d62cc1758c11836aed1

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/38ad1cc1b0c8
Part 1: Define the preference for motion-path. r=emilio

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b9ec0d707b5
Part 2: Define offset-path and implement it in style system. r=emilio

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/761e9bb54adb
Part 3: Use macro for path parser. r=emilio

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/196fc7b48b84
Part 4: Rename builder as aBuilder in SVGPathData.cpp. r=TYLin

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c217209a3b04
Part 5: Implement BuildPath for offset-path. r=jwatt

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc2785ab879e
Part 6: Apply motion path transform matrix. r=nical

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/48214a8e1b6b
Part 7: Tests. r=emilio

Comment 53

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12609 for changes under testing/web-platform/tests

Comment 54

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 55

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 56

[Andrei Ciure[:aciure]]

Backed out 7 changesets (bug 1429298) for xpcshell failures properties-db.js

failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=89117b6b5799666c38926cd566f230e19b050528&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable&selectedJob=195217238&filter-searchStr=Linux+x64+asan+Xpcshell+tests+test-linux64-asan%2Fopt-xpcshell-2+X%28X2%29

log: https://treeherder.mozilla.org/logviewer.html#?job_id=195217238&repo=autoland

backout: https://treeherder.mozilla.org/logviewer.html#?job_id=195217238&repo=autoland

Comment 57

[Boris Chiou [:boris]]

(In reply to Andrei Ciure[:andrei_ciure_] from comment #56)
> Backed out 7 changesets (bug 1429298) for xpcshell failures properties-db.js
> 
> failure:
> https://treeherder.mozilla.org/#/
> jobs?repo=autoland&fromchange=89117b6b5799666c38926cd566f230e19b050528&filter
> -resultStatus=testfailed&filter-resultStatus=busted&filter-
> resultStatus=exception&filter-resultStatus=retry&filter-
> resultStatus=usercancel&filter-resultStatus=running&filter-
> resultStatus=pending&filter-
> resultStatus=runnable&selectedJob=195217238&filter-
> searchStr=Linux+x64+asan+Xpcshell+tests+test-linux64-asan%2Fopt-xpcshell-
> 2+X%28X2%29
> 
> log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=195217238&repo=autoland
> 
> backout:
> https://treeherder.mozilla.org/logviewer.html#?job_id=195217238&repo=autoland

hmm,
I know the reason. I should move the update of devtools/shared/css/generated/properties-db.js into the last patch.
If I could push all patches together into m-i, these will not be backed out.

Comment 58

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e03480382807
Backed out 7 changesets for xpcshell failures properties-db.js CLOSED TREE

Comment 59

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/085b28450cc3
Part 1: Define the preference for motion-path. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/04a196dc2cc2
Part 2: Define offset-path and implement it in style system. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/d78aadc6a6bf
Part 3: Use macro for path parser. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7b80e0bc7bf
Part 4: Rename builder as aBuilder in SVGPathData.cpp. r=TYLin
https://hg.mozilla.org/integration/mozilla-inbound/rev/96671e2dcead
Part 5: Implement BuildPath for offset-path. r=jwatt
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad10cbe201c4
Part 6: Apply motion path transform matrix. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d61347cec03
Part 7: Tests. r=emilio

Comment 60

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 61

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/085b28450cc3
https://hg.mozilla.org/mozilla-central/rev/04a196dc2cc2
https://hg.mozilla.org/mozilla-central/rev/d78aadc6a6bf
https://hg.mozilla.org/mozilla-central/rev/f7b80e0bc7bf
https://hg.mozilla.org/mozilla-central/rev/96671e2dcead
https://hg.mozilla.org/mozilla-central/rev/ad10cbe201c4
https://hg.mozilla.org/mozilla-central/rev/6d61347cec03

Comment 62

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 63

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/085b28450cc3
https://hg.mozilla.org/mozilla-central/rev/04a196dc2cc2
https://hg.mozilla.org/mozilla-central/rev/d78aadc6a6bf
https://hg.mozilla.org/mozilla-central/rev/f7b80e0bc7bf
https://hg.mozilla.org/mozilla-central/rev/96671e2dcead
https://hg.mozilla.org/mozilla-central/rev/ad10cbe201c4
https://hg.mozilla.org/mozilla-central/rev/6d61347cec03

Comment 64

[Sebastian Zartner [:sebo]]

I've added a section for this at the experimental features:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Experimental_features#motion-path-properties.

Sebastian

Comment 65

[Boris Chiou [:boris]]

Looks good. Thanks.