Closed Bug 1381143 Opened 2 years ago Closed 2 years ago

Stylo: No CSS error reported for invalid color values

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: jryans, Assigned: jdm)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Gecko reports an CSS console message for invalid colors (PEColorNotColor) when you have things like `color: blah`, but Stylo doesn't log anything.

This happens to be tested in a DevTools console test (devtools/client/webconsole/test/browser_cached_messages.js), so we'll want to either add the message or update this test.
I've been working on addressing this in between doing error reporting performance investigations.
Assignee: nobody → josh
This implementation contains some trickery. The ideal way to solve
this would have the value parsing functions return a separate error
type that encapsulates the errors that can be encountered while
parsing them. However, the changes required to support this are
intrusive and pervasive, and I have an incomplete multi-patch stack
that is taking way too long to actually reach a useful point.

Rather than hold up all potential error reporting improvements like
this one, I chose to add a new variant to StyleParseError that is
only used to smuggle a more restricted enum that encapsulates the
value parsing errors that Gecko can report meaningful, specific
errors for.

This patch also adds support for reporting an extra message before
the main error message. I'll hold off adding support for a second
one until I reach the point where it's required.
Attachment #8890620 - Flags: review?(cam)
bzexport ruins my diff context settings, so here's a better one.
Attachment #8890620 - Attachment is obsolete: true
Attachment #8890620 - Flags: review?(cam)
I had to attach the result of qdiff like an animal.
Attachment #8890627 - Attachment is obsolete: true
Attachment #8890628 - Flags: review?(cam)
Comment on attachment 8890628 [details] [diff] [review]
Report specific error for invalid CSS colors

Review of attachment 8890628 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/ServoBindings.h
@@ +655,5 @@
>                                      nsIURI* aURI,
> +                                    const char* followup,
> +                                    const char* prefix,
> +                                    const char* prefixParam,
> +                                    uint32_t prefixParamLen);

Maybe re-order the arguments so that all of the message-related ones are together, e.g.

  message, param, paramLen, prefix, prefixParam, prefixParamLen, followup [or maybe call it suffix now]

?

::: servo/components/style_traits/lib.rs
@@ +126,5 @@
> +}
> +
> +impl<'i> ValueParseError<'i> {
> +    /// Attempt to extract a ValueParseError value from a ParseError.
> +    pub fn from_parse_error(this: ParseError<'i>) -> Option<ValueParseError<'i>> {

I wonder if it make the call sites of this a bit more succinct if it's instead a `impl From<ParseError> for Option<ValueParseError>`?  (I'm not sure if implementing From for Option<X> is normal or weird.)

::: servo/ports/geckolib/error_reporter.rs
@@ +281,5 @@
>                  _, CssParseError::Custom(SelectorParseError::Custom(
>                      StyleParseError::PropertyDeclaration(
> +                        PropertyDeclarationParseError::InvalidValue(_, ref err))))) => {
> +                let prefix = match *err {
> +                    Some(ValueParseError::InvalidColor(_)) => Some(&b"PEColorNotColor\0"[..]),

(Why is the "[..]" needed?)
Attachment #8890628 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #5)
> Comment on attachment 8890628 [details] [diff] [review]
> Report specific error for invalid CSS colors
> 
> Review of attachment 8890628 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: servo/components/style_traits/lib.rs
> @@ +126,5 @@
> > +}
> > +
> > +impl<'i> ValueParseError<'i> {
> > +    /// Attempt to extract a ValueParseError value from a ParseError.
> > +    pub fn from_parse_error(this: ParseError<'i>) -> Option<ValueParseError<'i>> {
> 
> I wonder if it make the call sites of this a bit more succinct if it's
> instead a `impl From<ParseError> for Option<ValueParseError>`?  (I'm not
> sure if implementing From for Option<X> is normal or weird.)

I don't believe this is legal - ParseError is a typedef for cssparser::ParseError, so it's not a type defined in the local crate.
 
> ::: servo/ports/geckolib/error_reporter.rs
> @@ +281,5 @@
> >                  _, CssParseError::Custom(SelectorParseError::Custom(
> >                      StyleParseError::PropertyDeclaration(
> > +                        PropertyDeclarationParseError::InvalidValue(_, ref err))))) => {
> > +                let prefix = match *err {
> > +                    Some(ValueParseError::InvalidColor(_)) => Some(&b"PEColorNotColor\0"[..]),
> 
> (Why is the "[..]" needed?)

The coercion from &[u8; N] to &[u8] does not occur when the literal appears inside of Some.
Attachment #8890628 - Attachment is obsolete: true
This needs to be pushed to autoland after https://github.com/servo/servo/pull/17926 merges.
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2c6f632e1326
Report specific error for invalid CSS colors. r=heycam a=bustage
https://hg.mozilla.org/mozilla-central/rev/2c6f632e1326
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.