feat(color): Refactor existing color parsing to better align with spec #308

GPHemsley · 2022-10-08T03:00:52Z

Add public structs Hue and AlphaValue.

Add methods parse_hue() and parse_alpha_value() to public trait ColorComponentParser.

Add or update documentation to include links to relevant sections of css-color-4 draft.

Following on from my comment on #293, I've gone ahead and refactored the color parsing code to better align with the text of the draft css-color-4 spec, which also allows for easier extension to support the multitude of new functionality that is being specified. (I already have a working implementation of the none keyword nearing completion based on these changes, for example.)

I don't know the policy on removing public features, so I've left in the NumberOrPercentage and AngleOrNumber enums and the parse_angle_or_number() and parse_number_or_percentage() methods of ColorComponentParser, even though I think they are now redundant. I'd be happy to remove them in a later PR, if that's deemed appropriate.

For reference, the latest draft of css-color-4 is available here:
https://w3c.github.io/csswg-drafts/css-color-4/

Add public structs Hue and AlphaValue. Add methods parse_hue() and parse_alpha_value() to public trait ColorComponentParser. Add or update documentation to include links to relevant sections of css-color-4 draft.

emilio · 2022-10-20T06:50:58Z

Ah, sorry, I had missed this one, feel free to explicitly request review so I don't miss it in the future. I'll take a look asap.

emilio

Thanks. Not opposed to introducing Hue or AlphaValue (though I wonder if it'd make sense to do that separately, reusing parse_number_or_percentage).

I wonder what's the reasoning for changing the parsing code? At a glance seems it should be slower and without any obvious benefit, but it might have more obvious of benefit with following changes?

In any case we'd need to allow constructing Hue / AlphaValue externally.

emilio · 2022-10-20T08:18:36Z

src/color.rs

+    ) -> Result<Hue, ParseError<'i, Self::Error>> {
+        let location = input.current_source_location();
+        Ok(match *input.next()? {
+            Token::Number { value: degrees, .. } => Hue { degrees },


So the reason this (parse_angle_or_number, etc) are in ColorComponentParser is so that we can add calc() support externally e.g., in Firefox. However Hue is not constructible outside of this crate so that's not feasible. That needs to at least be fixed

emilio · 2022-10-20T08:18:48Z

src/color.rs

+    fn parse_alpha_value<'t>(
+        &self,
+        input: &mut Parser<'i, 't>,
+    ) -> Result<AlphaValue, ParseError<'i, Self::Error>> {


emilio · 2022-10-20T08:20:47Z

src/color.rs

-    // https://drafts.csswg.org/css-values/#angles
-    let hue_degrees = component_parser.parse_angle_or_number(arguments)?.degrees();
+    // Try to parse legacy syntax first.
+    let legacy_syntax_result = arguments.try_parse(|input| {


This refactoring seems somewhat gratuituous? What advantages does it give us? At a glance it seems it would make parsing a bit slower / rewind more.

bors-servo · 2022-10-20T08:40:42Z

☔ The latest upstream changes (presumably #309) made this pull request unmergeable. Please resolve the merge conflicts.

GPHemsley · 2022-10-29T21:41:13Z

feel free to explicitly request review so I don't miss it in the future.

I didn't/don't seem to have the permissions to be able to do that.

Thanks. Not opposed to introducing Hue or AlphaValue (though I wonder if it'd make sense to do that separately, reusing parse_number_or_percentage).

As I mentioned above, I would have already removed parse_number_or_percentage() as redundant if it weren't a public function. The goal of this refactor is to better align concepts and implementation with the css-color-4 spec, and "number or percentage" is not a term defined there. (It's possible that it's a term defined in another spec, but that doesn't seem to have any bearing on <color>.)

I wonder what's the reasoning for changing the parsing code? At a glance seems it should be slower and without any obvious benefit, but it might have more obvious of benefit with following changes?

I laid out my reasoning in #293 (comment).

The motivation for this refactoring is to make maintenance easier, based on how the spec is structured. I did not optimize for speed, nor is it clear to me that the difference is significant.

My implementation of none will rely on this refactoring more obviously, as none is not allowed in legacy syntax and the existing code relies on being able to use the red channel value to determine which branch of the syntax to use.

In any case we'd need to allow constructing Hue / AlphaValue externally.

I'm not sure I'm following what you mean here. parse_hue() and parse_alpha_value() allow you to turn a token stream into their corresponding structs, or you can just specify the values directly.

As far as I can tell, the existing color and calc implementations in servo would be just as happy to use Hue or AlphaValue instead, though it's difficult for me to say because servo defines its own NumberOrPercentage in multiple places. Can you point me to where you think the implementation needs to be able to differentiate between how the values were derived?

emilio · 2022-10-29T23:16:32Z

My point is, how do we re-implement ColorComponentParser::parse_hue to support e.g. calc() if the hue degrees member is not public and there's no function to construct a Hue? Sorry if I wasn't clear on that point, or if I've missed something.

And yeah, let's remove parse_number_or_percentage. Having a function that gets never called on an interface is silly, and it doesn't particularly help backwards compat (this should be published as a breaking change regardless).

GPHemsley · 2022-10-31T01:06:16Z

My point is, how do we re-implement ColorComponentParser::parse_hue to support e.g. calc() if the hue degrees member is not public and there's no function to construct a Hue? Sorry if I wasn't clear on that point, or if I've missed something.

I think that's a problem with NumberOrPercentage and AngleOrNumber because they're enums that doesn't exist for Hue and AlphaValue because they're structs.

For example, this is what I've been using to get the values out for tests, which is annoying:

            if let AngleOrNumber::Angle { degrees } = ColorComponentParser::parse_angle_or_number(
                &DefaultComponentParser,
                &mut Parser::new(&mut ParserInput::new("123deg")),
            )
            .unwrap()
            {
                degrees
            } else {
                unreachable!()
            };

But I think for the new stuff we can just do something like this:

        let x = Hue { degrees: 0. };
        let y = x.degrees;

I'll dig around to confirm after I finish with this other thing I'm working on.

And yeah, let's remove parse_number_or_percentage. Having a function that gets never called on an interface is silly, and it doesn't particularly help backwards compat (this should be published as a breaking change regardless).

Oh, cool. parse_angle_or_number(), too, then?

emilio · 2022-10-31T08:47:45Z

My point is, how do we re-implement ColorComponentParser::parse_hue to support e.g. calc() if the hue degrees member is not public and there's no function to construct a Hue? Sorry if I wasn't clear on that point, or if I've missed something.

I think that's a problem with NumberOrPercentage and AngleOrNumber because they're enums that doesn't exist for Hue and AlphaValue because they're structs.

For example, this is what I've been using to get the values out for tests, which is annoying:
            if let AngleOrNumber::Angle { degrees } = ColorComponentParser::parse_angle_or_number(
                &DefaultComponentParser,
                &mut Parser::new(&mut ParserInput::new("123deg")),
            )
            .unwrap()
            {
                degrees
            } else {
                unreachable!()
            };

Unclear why you wouldn't be able to just use 123 there? That's besides my point anyways. My point is that we want to override parse_hue etc, and while you can construct a NumberOrPercentage from a dependent crate right now (like here), you wouldn't be able to construct a Hue (because visibility of struct fields and enum fields is different). That is, the degrees field of Hue should be public.

Oh, cool. parse_angle_or_number(), too, then?

Yes, all of them.

GPHemsley · 2022-11-01T22:18:29Z

My point is, how do we re-implement ColorComponentParser::parse_hue to support e.g. calc() if the hue degrees member is not public and there's no function to construct a Hue? Sorry if I wasn't clear on that point, or if I've missed something.

I think that's a problem with NumberOrPercentage and AngleOrNumber because they're enums that doesn't exist for Hue and AlphaValue because they're structs.
For example, this is what I've been using to get the values out for tests, which is annoying:
            if let AngleOrNumber::Angle { degrees } = ColorComponentParser::parse_angle_or_number(
                &DefaultComponentParser,
                &mut Parser::new(&mut ParserInput::new("123deg")),
            )
            .unwrap()
            {
                degrees
            } else {
                unreachable!()
            };
Unclear why you wouldn't be able to just use 123 there? That's besides my point anyways.

You mean for the value being parsed? I'm testing angle parsing in this example.

My point is that we want to override parse_hue etc, and while you can construct a NumberOrPercentage from a dependent crate right now (like here), you wouldn't be able to construct a Hue (because visibility of struct fields and enum fields is different). That is, the degrees field of Hue should be public.

Oooooh. That was unintentional; I'll fix it. (I think I was actually confusing struct with impl wrt to visibility.)

Oh, cool. parse_angle_or_number(), too, then?

Yes, all of them.

👍🏻

tiaanl · 2022-11-01T22:58:05Z

src/color.rs

-        }
-    }
+/// https://w3c.github.io/csswg-drafts/css-color-4/#hue-syntax
+pub struct Hue {


Perhaps a newtype as there is only one value:

pub struct Hue(f32);

tiaanl · 2022-11-01T22:58:37Z

src/color.rs

+}
+
+/// https://w3c.github.io/csswg-drafts/css-color-4/#alpha-syntax
+pub struct AlphaValue {


Same as above, also we know it's a value so just Alpha would suffice.

tiaanl · 2022-11-01T22:59:54Z

src/color.rs

+
+    /// Parse a `<hue>` value.
+    /// https://w3c.github.io/csswg-drafts/css-color-4/#typedef-hue
+    fn parse_hue<'t>(


This function doesn't really parse hue values, but angles. Should there be a specific abstraction for hue values only?

tiaanl · 2022-11-01T23:11:04Z

src/color.rs

-        NumberOrPercentage::Percentage { unit_value } => (clamp_unit_f32(unit_value), false),
-    };
+    // Try to parse legacy syntax first.
+    let legacy_syntax_result = arguments.try_parse(|input| {


This function ends up being very long and duplicates quite a bit of code. That and the fact that the hsl and hwb versions are also on their own now, duplicating code, I'm not sure this is an improvement over the original.

The abstractions for Hue and Alpha doesn't add much value on their own. If you are the consumer of the parse_hue function you don't even know what unit you're getting back. Adding some unit function might help, e.g. Hue::as_degrees(..), Hue::as_percentage(..) or Hue::as_unit(..). This of course is that NumberOrPercentage does with enums.

GPHemsley · 2022-11-22T13:50:35Z

Just to checkpoint this, since it's taking a while...

My initial approach in this PR was to do this refactoring iteratively so as to preserve backwards compatibility.

However, given the feedback above, I have now opted to perform a complete rewrite of color support all at once, based directly on the css-color-4 spec (as opposed to the hybrid between css-color-3 and css-color-4 that currently exists in the codebase). This will allow the addition of a handful of new features straight away and will make it easier to add the remaining support in the future. I am also attempting to draw the line more firmly between parsing and processing, so that more parsing things can be done in this library, but some other processing things are deferred to code that makes use of it.

My work has been complicated by the refactoring that was done as part of #309, but I think I've gotten through that and am now tweaking and tidying up edgecases. That being said, it will likely be a couple more weeks before I have a new PR ready to present. (Until then, I'll leave this one open.)

GPHemsley · 2022-12-08T14:51:30Z

Superseded by #314.

feat(color): Refactor existing color parsing to better align with spec

66d30e4

Add public structs Hue and AlphaValue. Add methods parse_hue() and parse_alpha_value() to public trait ColorComponentParser. Add or update documentation to include links to relevant sections of css-color-4 draft.

GPHemsley mentioned this pull request Oct 20, 2022

Implement [ok]lab, [ok]lch from color-4 spec. #309

Merged

emilio requested changes Oct 20, 2022

View reviewed changes

tiaanl reviewed Nov 1, 2022

View reviewed changes

GPHemsley marked this pull request as draft November 2, 2022 02:48

GPHemsley mentioned this pull request Dec 2, 2022

Change RGBA components to f32 and change tests to check css. #311

Closed

GPHemsley closed this Dec 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(color): Refactor existing color parsing to better align with spec #308

feat(color): Refactor existing color parsing to better align with spec #308

GPHemsley commented Oct 8, 2022

emilio commented Oct 20, 2022

emilio left a comment

emilio Oct 20, 2022

emilio Oct 20, 2022

emilio Oct 20, 2022

bors-servo commented Oct 20, 2022

GPHemsley commented Oct 29, 2022 •

edited

Loading

emilio commented Oct 29, 2022

GPHemsley commented Oct 31, 2022

emilio commented Oct 31, 2022

GPHemsley commented Nov 1, 2022

tiaanl Nov 1, 2022

tiaanl Nov 1, 2022

tiaanl Nov 1, 2022

tiaanl Nov 1, 2022

GPHemsley commented Nov 22, 2022

GPHemsley commented Dec 8, 2022

feat(color): Refactor existing color parsing to better align with spec #308

feat(color): Refactor existing color parsing to better align with spec #308

Conversation

GPHemsley commented Oct 8, 2022

emilio commented Oct 20, 2022

emilio left a comment

Choose a reason for hiding this comment

emilio Oct 20, 2022

Choose a reason for hiding this comment

emilio Oct 20, 2022

Choose a reason for hiding this comment

emilio Oct 20, 2022

Choose a reason for hiding this comment

bors-servo commented Oct 20, 2022

GPHemsley commented Oct 29, 2022 • edited Loading

emilio commented Oct 29, 2022

GPHemsley commented Oct 31, 2022

emilio commented Oct 31, 2022

GPHemsley commented Nov 1, 2022

tiaanl Nov 1, 2022

Choose a reason for hiding this comment

tiaanl Nov 1, 2022

Choose a reason for hiding this comment

tiaanl Nov 1, 2022

Choose a reason for hiding this comment

tiaanl Nov 1, 2022

Choose a reason for hiding this comment

GPHemsley commented Nov 22, 2022

GPHemsley commented Dec 8, 2022

GPHemsley commented Oct 29, 2022 •

edited

Loading