Page MenuHomePhabricator

Bug 1598158 - Support offset-path:url() in style.
ClosedPublic

Authored by boris on Jul 24 2023, 11:01 PM.
Referenced Files
Unknown Object (File)
Thu, Apr 9, 6:01 AM
Unknown Object (File)
Wed, Mar 25, 5:21 PM
Unknown Object (File)
Feb 6 2026, 7:02 PM
Unknown Object (File)
Jan 29 2026, 11:45 PM
Unknown Object (File)
Jan 3 2026, 2:52 AM
Unknown Object (File)
Jan 3 2026, 2:51 AM
Unknown Object (File)
Nov 16 2025, 2:44 PM
Unknown Object (File)
Oct 12 2025, 9:06 PM
Subscribers

Details

Summary

In layout, we build a default path("m 0 0") for now. We will implement
it later.

Besides, we don't support compositor animations for url(), so we don't
have to serialize it for IPC.

Note:
<url> includes url() and src(). For now we only support url().
We should revisit src() in Bug 1845390.

Event Timeline

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.
boris edited the summary of this revision. (Show Details)

Rebased and added tests for non-svg element

boris added 1 blocking reviewer(s): Restricted Project.Jul 28 2023, 6:28 PM
emilio requested changes to this revision.Jul 31 2023, 5:41 PM

I'm fairly confused about the deserialization stuff... Unless I'm missing something we should definitely not crash.

servo/components/style/values/generics/motion.rs
113

I'm confused, seems like #[serde(skip_deserializing)] should work? But at the very least we should return a proper error.

119

This will crash the parent / GPU process when deserializing right? That doesn't seem acceptable?

servo/ports/geckolib/cbindgen.toml
1045

This only has one caller, so probably I'd just remove it?

This revision now requires changes to proceed.Jul 31 2023, 5:41 PM
servo/components/style/values/generics/motion.rs
113

It should work, but it seems it makes IPC::ReadParam<::mozilla::StyleOffsetPath>(aReader) returns error. Maybe our serde version is too old.

I just noticed it may be a bug but a newer version fixed it: https://github.com/serde-rs/serde/releases/tag/v1.0.165.

servo/components/style/values/generics/motion.rs
113

This happens even if the input type of OffsetPathFunction is not url(). So I suspect this is a serde bug.

Anyway, I will make this return an Err() instead to work around it.

boris marked 3 inline comments as done.

Addressed comments

emilio added a project: testing-approved.
emilio removed reviewers: layout-reviewers, Restricted Project.
emilio added inline comments.
servo/components/style/values/generics/motion.rs
113

Can you cargo update -p serde, then ./mach vendor rust instead?

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3aa65b84ebc70e40d1537582edf3df55 seems to work.

Otherwise, mind filing a serde issue and referencing it here?

This revision is now accepted and ready to land.Aug 7 2023, 2:00 PM
servo/components/style/values/generics/motion.rs
113

Updating serde doesn't work. I notices this doesn't happen when I use ray(). The assertion happen only when I use basic shape, path(), circle(), ...etc. I'm still trying to figure out what happen.

So I would like to file a Gecko bug first to figure out what happen, so I could file a serde issue with a reduced example.

Of course, I will reference the Gecko bug here.

Add follow-up bug numbers.

boris marked an inline comment as done.Aug 7 2023, 7:17 PM