Boris Chiou [:boris] Assignee
	Description • 6 years ago

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

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

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

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

(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

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 3 • 6 years ago

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.

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

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

	Robert Longson [:longsonr]
	Comment 5 • 6 years ago

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.

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

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

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

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

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 8 • 6 years ago

Thank you Boris!!

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 9 • 6 years ago

\o/

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

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

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

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

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

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

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

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

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

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

Depends on D2965

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

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

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

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

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

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

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

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

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

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.

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

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

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

	Nicolas Silva [:nical]
	Comment 31 • 6 years ago

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

	Nicolas Silva [:nical]
	Comment 41 • 6 years ago

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

Nicolas Silva [:nical] has approved the revision.

	Jonathan Watt [:jwatt]
	Comment 42 • 6 years ago

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

Jonathan Watt [:jwatt] has approved the revision.

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

Follow the rule of naming for the function parameters.

Depends on D2966

	Ting-Yu Lin [:TYLin] (PDT, UTC-7)
	Comment 44 • 6 years ago

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.

	Pulsebot
	Comment 46 • 6 years ago

Pushed by boris.chiou@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/38ad1cc1b0c8
Part 1: Define the preference for motion-path. r=emilio

	Pulsebot
	Comment 47 • 6 years ago

Pushed by boris.chiou@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0b9ec0d707b5
Part 2: Define offset-path and implement it in style system. r=emilio

	Pulsebot
	Comment 48 • 6 years ago

Pushed by boris.chiou@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/761e9bb54adb
Part 3: Use macro for path parser. r=emilio

	Pulsebot
	Comment 49 • 6 years ago

Pushed by boris.chiou@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/196fc7b48b84
Part 4: Rename builder as aBuilder in SVGPathData.cpp. r=TYLin

	Pulsebot
	Comment 50 • 6 years ago

Pushed by boris.chiou@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c217209a3b04
Part 5: Implement BuildPath for offset-path. r=jwatt

	Pulsebot
	Comment 51 • 6 years ago

Pushed by boris.chiou@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cc2785ab879e
Part 6: Apply motion path transform matrix. r=nical

	Pulsebot
	Comment 52 • 6 years ago

Pushed by boris.chiou@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/48214a8e1b6b
Part 7: Tests. r=emilio

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 53 • 6 years ago

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

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 54 • 6 years ago

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

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 55 • 6 years ago

Upstream PR was closed without merging

	Andrei Ciure[:aciure]
	Comment 56 • 6 years ago

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

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

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

	Pulsebot
	Comment 58 • 6 years ago

Backout by aciure@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e03480382807
Backed out 7 changesets for xpcshell failures properties-db.js CLOSED TREE

	Pulsebot
	Comment 59 • 6 years ago

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

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 60 • 6 years ago

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

	Noemi Erli[:noemi_erli]
	Comment 61 • 6 years ago
bugherder

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

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 62 • 6 years ago

Upstream PR merged

	Noemi Erli[:noemi_erli]
	Comment 63 • 6 years ago
bugherder

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

	Sebastian Zartner [:sebo]
	Comment 64 • 6 years ago

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

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

Looks good. Thanks.