Closed Bug 1429298 Opened 2 years ago Closed Last year

Implement path() function for offset-path

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: boris, Assigned: boris)

References

(Blocks 3 open bugs, )

Details

(Keywords: dev-doc-complete)

Attachments

(7 files, 8 obsolete files)

46 bytes, text/x-phabricator-request
emilio
: review+
Details | Review
46 bytes, text/x-phabricator-request
emilio
: review+
Details | Review
46 bytes, text/x-phabricator-request
emilio
: review+
Details | Review
46 bytes, text/x-phabricator-request
jwatt
: review+
Details | Review
46 bytes, text/x-phabricator-request
nical
: review+
Details | Review
46 bytes, text/x-phabricator-request
emilio
: review+
Details | Review
46 bytes, text/x-phabricator-request
TYLin
: review+
Details | Review
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
Status: NEW → ASSIGNED
Blocks: 1429299
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
(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
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.
(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. :)
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.
OK. I see. I will check what the Firefox parser does and the detail of both specs. Thanks. :)
Assignee: boris.chiou → nobody
Status: ASSIGNED → NEW
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
Thank you Boris!!
Priority: -- → P3
Assignee: nobody → boris.chiou
Status: NEW → ASSIGNED
Attachment #8991751 - Attachment is obsolete: true
Attachment #8992446 - Attachment is obsolete: true
Attachment #8992454 - Attachment is obsolete: true
Attachment #8992454 - Attachment is obsolete: true
Attachment #8992456 - Attachment is obsolete: true
Attachment #8992457 - Attachment is obsolete: true
Attachment #8992459 - Attachment is obsolete: true
Blocks: 1480665
Define the preference. I will enable it only for debug usage and test
coverage in a different patch.
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
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
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
There are a lot of duplicates, so we use macro to refine them.

Depends on D2965
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
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
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
Attachment #8998640 - Attachment is obsolete: true
Attachment #8998639 - Attachment is obsolete: true
Attachment #8998641 - Attachment description: Bug 1429298 - Part 5: Use macro for path parser and encoder. → Bug 1429298 - Part 3: Use macro for path parser.
Attachment #8998642 - Attachment description: Bug 1429298 - Part 6: Factor out BuildPath code. → Bug 1429298 - Part 4: Factor out BuildPath code and implement one for offset-path.
Attachment #8998643 - Attachment description: Bug 1429298 - Part 7: Create motion path transform and combine it with other transforms. → Bug 1429298 - Part 5: Create motion path transform and combine it with other transforms.
Attachment #8998644 - Attachment description: Bug 1429298 - Part 8: Tests. → Bug 1429298 - Part 6: Tests.
Attachment #8998643 - Attachment description: Bug 1429298 - Part 5: Create motion path transform and combine it with other transforms. → Bug 1429298 - Part 5: Apply motion path transform matrix.
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.
Attachment #8998638 - Flags: review+
Comment on attachment 8998641 [details]
Bug 1429298 - Part 3: Use macro for path parser.

Emilio Cobos Álvarez (:emilio) has approved the revision.
Attachment #8998641 - Flags: review+
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).
(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. :)
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).
(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.
(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.
(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.
(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.
(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. :)
Blocks: 1483410
Comment on attachment 8998644 [details]
Bug 1429298 - Part 7: Tests.

Emilio Cobos Álvarez (:emilio) has approved the revision.
Attachment #8998644 - Flags: review+
Attachment #8998642 - Flags: review?(jwatt)
Comment on attachment 8998637 [details]
Bug 1429298 - Part 1: Define the preference for motion-path.

Emilio Cobos Álvarez (:emilio) has approved the revision.
Attachment #8998637 - Flags: review+
Blocks: 1246764
Comment on attachment 8998643 [details]
Bug 1429298 - Part 6: Apply motion path transform matrix.

Nicolas Silva [:nical] has approved the revision.
Attachment #8998643 - Flags: review+
Blocks: 1484780
Attachment #8998642 - Attachment description: Bug 1429298 - Part 4: Factor out BuildPath code and implement one for offset-path. → Bug 1429298 - Part 4: Implement BuildPath for offset-path.
Comment on attachment 8998642 [details]
Bug 1429298 - Part 5: Implement BuildPath for offset-path.

Jonathan Watt [:jwatt] has approved the revision.
Attachment #8998642 - Flags: review+
Follow the rule of naming for the function parameters.

Depends on D2966
Attachment #8998642 - Attachment description: Bug 1429298 - Part 4: Implement BuildPath for offset-path. → Bug 1429298 - Part 5: Implement BuildPath for offset-path.
Attachment #8998643 - Attachment description: Bug 1429298 - Part 5: Apply motion path transform matrix. → Bug 1429298 - Part 6: Apply motion path transform matrix.
Attachment #8998644 - Attachment description: Bug 1429298 - Part 6: Tests. → Bug 1429298 - Part 7: Tests.
Attachment #8998642 - Flags: review?(jwatt)
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.
Attachment #9002929 - Flags: review+
Pushed by boris.chiou@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/38ad1cc1b0c8
Part 1: Define the preference for motion-path. r=emilio
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
Pushed by boris.chiou@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/761e9bb54adb
Part 3: Use macro for path parser. r=emilio
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
Pushed by boris.chiou@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c217209a3b04
Part 5: Implement BuildPath for offset-path. r=jwatt
Pushed by boris.chiou@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cc2785ab879e
Part 6: Apply motion path transform matrix. r=nical
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12609 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR was closed without merging
(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.
Flags: needinfo?(boris.chiou)
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
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
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Looks good. Thanks.
Blocks: 1551742
Blocks: 1582554
You need to log in before you can comment on or make changes to this bug.