[WIP] Parse Style from and serialize to CSS syntax #460
Conversation
|
"unused variable" warnings are deliberate, they represent |
Encapsulate unsafe in `_cssparser_internal_to_lowercase` macro # Objective - Allow the `_cssparser_internal_to_lowercase` macro to be used in crates that set `#[forbid(unsafe)]`. - Document the safety invariant for this unsafe block ## Changes made - Wrap the `unsafe` block in the `_cssparser_internal_to_lowercase` macro in an `#[inline(always)]` function. - Add comments ## Context - DioxusLabs/taffy#460 (comment)
| // TODO: deal with shorthand v.s. longhand properties per | ||
| // https://drafts.csswg.org/cssom/#serialize-a-css-declaration-block | ||
| if *display != Style::DEFAULT.display { | ||
| decl!("display", display); | ||
| } |
There was a problem hiding this comment.
Given that Taffy's default (flex) is different to the web default (block), I'm thinking it might be better to unconditionally serialize display here for compatibility.
| }; | ||
| let css_inches = value / units_per_inch; | ||
| let css_pxs = css_inches * 96.; | ||
| let taffy_units = css_pxs * taffy.config.pixel_ratio; |
There was a problem hiding this comment.
I think it might make more sense to have Dimension::Length store logical pixels. That would simplify this code (it wouldn't have to care about pixel_ratio: it can just naively pass px values through to the Length variant), and would also match how I would expect users of Taffy's underlying Rust API to specify styles.
I think the pixel_ratio config option on the Taffy struct is really good idea, and actually enables this parsing code to ignore pixel ratio, becuase it means that Taffy can operate on a "logical pixels in, physical pixels out basis" (or perhaps even "unrounded logical pixels AND rounded physical pixels out").
There was a problem hiding this comment.
So Dimension::Length would be in CSS px and values in Layout would not be in the same unit anymore but differ by pixel_ratio? That could work, but I feel the latter half should be a separate PR. But yeah it’d be nice not to need to pass &Taffy around in all parsing and serialization functions.
There was a problem hiding this comment.
#478 attempts to implement your proposal, which would replace conversion during parsing and serialization.
| trait CssValue: Sized { | ||
| fn parse<'i, 't>(input: &mut Parser<'i, 't>, taffy: &Taffy) -> Result<Self, ParseError<'i>>; | ||
|
|
||
| fn serialize(&self, dest: &mut String, taffy: &Taffy); | ||
| } |
There was a problem hiding this comment.
I'm thinking it might make sense to split this up into two traits (one for parsing, one for serializing). That way we can put parsing and serializing code behind separate feature flags if we want to.
There was a problem hiding this comment.
I'm also wondering if it would make sense to just make these the Display and FromStr traits. Do you have any insight into the pros and cons of using the standard traits vs. custom ones?
There was a problem hiding this comment.
I’m not super convinced of the value of enabling one of parsing or serializing without the other but if you prefer yes it could be two traits.
Parsing needs a signature different than FromStr. For parsing a value of various Rust types while parsing a declaration, we want to keep using the same tokenizer and cssparser::Parser. Finding the right start and end to extract a &str slice and create another tokenizer for that would be extra work, and would lose the ability for errors to carry accurate source location (line numbers).
If a &Taffy parameter is no longer needed (see above) then serialization could have the same signature as Display but I think it still represents different intent. Values that can contain arbitrary text may need backslash-escapes for serialization to CSS syntax while Display (for presentation to users of the program) probably doesn’t want that.
| macro_rules! keywords_only { | ||
| ( $( | ||
| $( #[$meta: meta] )* | ||
| $keyword: literal => $enum_variant: ident, | ||
| )+ ) => { |
There was a problem hiding this comment.
Documentation on this macro (what it does and how it works) would be good
|
FWIW, the fact that (although I'm also wondering whether |
|
cssparser’s proc macros used to do more but they’ve shrunk over time. And I think |
…al pixels Per DioxusLabs#460 (comment) See details in doc-comment changes.
…al pixels Per DioxusLabs#460 (comment) See details in doc-comment changes.
…al pixels Per DioxusLabs#460 (comment) See details in doc-comment changes.
|
I tried using those |
This is an incomplete work in progress, submitted in case someone wants to have a look at the approach.
This requires and is based on top of #459, so consider looking at per-commit diff.Objective
Fixes #440
TODO: update
RELEASES.md.Context
See discussion in #440
Feedback wanted
No proc macro so far, but I’ve made liberal use of private traits and
macro_rules!to reduce redundant code and risk of copy-paste errors. Hopefully the code is still approachable.