stylo: route CSS parsing errors to the developer tools console

RESOLVED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
4 months ago
11 days ago

People

(Reporter: heycam, Assigned: jdm)

Tracking

(Depends on: 1 bug, Blocks: 5 bugs)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(2 attachments, 9 obsolete attachments)

(Reporter)

Description

4 months ago
Like nsCSSParser and mozilla::css::ErrorReporter, we need to send CSS parsing errors to a page's developer tools console or the browser console.
These mochitest failures are for this: https://dxr.mozilla.org/mozilla-central/rev/00a166a8640dffa2e0f48650f966d75ca3c1836e/layout/style/test/stylo-failures.md#82-84
Blocks: 1321197
Priority: -- → P2
Priority: P2 → P1
(Assignee)

Comment 2

3 months ago
I'm giving this a shot. I'll unassign myself if I'm not making progress.
Assignee: nobody → josh
Awesome, thanks jdm!
Blocks: 1322657
(Assignee)

Comment 4

3 months ago
Created attachment 8859633 [details] [diff] [review]
WIP

I made a bunch of progress during a 14 hour plane ride! CSS parsing errors are now appearing in the console for operations like "element.style = 'foopy'"; I'm going to look into the automated tests next, as well as sort out how to get the stylesheet URL from stylo into the error reporter.
:jdm, any news here?  Will you be able to continue working on this?
Flags: needinfo?(josh)
(Assignee)

Comment 6

2 months ago
Yes. I have been actively working on it; https://github.com/servo/rust-cssparser/pull/155 and https://github.com/servo/servo/pull/16752 are the prerequisite API changes to the CSS parser. After those merge it will be much easier to work on the Stylo-specific parts.
Flags: needinfo?(josh)
This also affects some other mochitests like dom/base/test/test_bug513194.html
(Assignee)

Comment 8

21 days ago
Created attachment 8883410 [details] [diff] [review]
Hook up Stylo CSS parser to Gecko error reporter

MozReview-Commit-ID: 3r5Z6KiPgRM
Attachment #8883410 - Flags: review?(manishearth)
(Assignee)

Updated

21 days ago
Attachment #8859633 - Attachment is obsolete: true
(Assignee)

Comment 9

21 days ago
Created attachment 8883411 [details] [diff] [review]
Address failing CSS parser error tests

MozReview-Commit-ID: KfcpLYLIIve
Attachment #8883411 - Flags: review?(manishearth)
(Assignee)

Comment 10

21 days ago
The first patch sets up the infrastructure, but it's unlikely that any tests pass. The second patch requires https://github.com/servo/rust-cssparser/pull/168 and passes test_bug413958.html, test_parser_diagnostics_unprintables.html, and test_bug513194.html.
Comment on attachment 8883410 [details] [diff] [review]
Hook up Stylo CSS parser to Gecko error reporter

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

(Just drive-by reviewing, overall looks good)

::: dom/base/FragmentOrElement.cpp
@@ +508,5 @@
> +  // We are not going to support xml:base for stylo, but we want to
> +  // ensure we unship that support before we enabling stylo.
> +  MOZ_ASSERT(nsLayoutUtils::StyleAttrWithXMLBaseDisabled());
> +  // This also ignores the case that SVG inside XBL binding.
> +  // But it is probably fine.

This can be easily solved calling bindingParent->GetCSSLoaderForStyleAttribute above, right?

Also, since this really can't return null, I believe according to the style guide this should be called CSSLoaderForStyleAttr.

::: servo/components/style/stylist.rs
@@ +602,5 @@
>                                           pseudo: &PseudoElement,
>                                           parent: Option<&Arc<ComputedValues>>,
>                                           cascade_flags: CascadeFlags,
> +                                         font_metrics: &FontMetricsProvider,
> +                                         reporter: &ParseErrorReporter)

Please document (here and in the other functions) that the error reporter is needed only for the custom properties code?

(Actually, I guess some of the code calling this is going to be relatively hot and we'll need to avoid the heap allocation of the error reporter using placement new or something like that, but I guess that can be done as a followup).
Comment on attachment 8883411 [details] [diff] [review]
Address failing CSS parser error tests

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

::: servo/components/style/gecko/error_reporter.rs
@@ +54,5 @@
> +        }
> +    }
> +}
> +
> +fn escape_css_ident(ident: &str) -> String {

We have a serialize_identifier function. Is this doing the same, or something different? If so, it'd be nice to comment on the differences.

@@ +118,5 @@
> +    escaped
> +}
> +
> +// https://drafts.csswg.org/cssom/#serialize-a-string
> +fn escape_css_string(s: &str) -> String {

We definitely have code for string serialization in tree, look at String::to_css, and such.

@@ +137,5 @@
> +    }
> +    escaped
> +}
> +
> +fn token_to_str<'a>(t: Token<'a>) -> String {

And I think this really is reinventing token.to_css_string?
(Assignee)

Comment 13

20 days ago
(In reply to Emilio Cobos Álvarez [:emilio] from comment #12)
> Comment on attachment 8883411 [details] [diff] [review]
> Address failing CSS parser error tests
> 
> Review of attachment 8883411 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: servo/components/style/gecko/error_reporter.rs
> @@ +54,5 @@
> > +        }
> > +    }
> > +}
> > +
> > +fn escape_css_ident(ident: &str) -> String {
> 
> We have a serialize_identifier function. Is this doing the same, or
> something different? If so, it'd be nice to comment on the differences.

They are almost identical, but escape_css_string uses numeric escapes for characters between 0x7f and 0xa0. Comment added.

> @@ +118,5 @@
> > +    escaped
> > +}
> > +
> > +// https://drafts.csswg.org/cssom/#serialize-a-string
> > +fn escape_css_string(s: &str) -> String {
> 
> We definitely have code for string serialization in tree, look at
> String::to_css, and such.

That 0x7f and 0xa0 character range behaves differently, unfortunately. Comment added.

> @@ +137,5 @@
> > +    }
> > +    escaped
> > +}
> > +
> > +fn token_to_str<'a>(t: Token<'a>) -> String {
> 
> And I think this really is reinventing token.to_css_string?

This would be, except for the earlier behaviour differences and some very specific Gecko choices around bad URLs and strings.
(Assignee)

Comment 14

20 days ago
Created attachment 8883739 [details] [diff] [review]
Hook up Stylo CSS parser to Gecko error reporter

MozReview-Commit-ID: 3r5Z6KiPgRM
Attachment #8883739 - Flags: review?(emilio+bugs)
(Assignee)

Updated

20 days ago
Attachment #8883410 - Attachment is obsolete: true
Attachment #8883410 - Flags: review?(manishearth)
(Assignee)

Comment 15

20 days ago
Created attachment 8883740 [details] [diff] [review]
Address failing CSS parser error tests

MozReview-Commit-ID: KfcpLYLIIve
Attachment #8883740 - Flags: review?(emilio+bugs)
(Assignee)

Updated

20 days ago
Attachment #8883411 - Attachment is obsolete: true
Attachment #8883411 - Flags: review?(manishearth)
(Assignee)

Comment 16

20 days ago
Interdiff works on both patches.
Comment on attachment 8883739 [details] [diff] [review]
Hook up Stylo CSS parser to Gecko error reporter

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

Lots of nits and similar, but looks good.

r=me with all the followups filed, or addressing them here directly :)

::: dom/base/nsIContent.h
@@ +25,5 @@
>  namespace mozilla {
>  class EventChainPreVisitor;
>  struct URLExtraData;
> +namespace css {
> +class Loader;

Does this really need to be here? no other change references css::Loader in this file.

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +2852,4 @@
>  {
>    MOZ_ASSERT(aPresShell->StyleSet()->IsServo());
>  
> +  nsIDocument* document = aPresShell->GetDocument();

(Perhaps these functions should also assert aPresShell->GetDocument())

::: dom/svg/nsSVGElement.cpp
@@ +1212,4 @@
>                                     already_AddRefed<nsIURI> aBaseURI,
>                                     nsSVGElement* aElement,
>                                     StyleBackendType aBackend)
> +: mParser(aLoader), mLoader(aLoader), mDocURI(aDocURI), mBaseURI(aBaseURI),

nit: I think Gecko style would be something like:

  : mParser(aLoader)
  , mLoader(aLoader)
  , mDocURI(aDocURI)
  , //...

::: layout/style/ErrorReporter.cpp
@@ +149,5 @@
>  }
>  
> +ErrorReporter::ErrorReporter(const StyleSheet* aSheet,
> +                             const Loader* aLoader)
> +: mScanner(nullptr), mSheet(aSheet), mLoader(aLoader), mURI(nullptr),

Similarly, I think the colon should be indented two spaces (you can run ./mach clang-format and it'll auto-format your changes).

@@ +244,4 @@
>  }
>  
>  void
> +ErrorReporter::OutputError(uint32_t aLineNumber, uint32_t aLineOffset, nsIURI* aURI, const nsACString& aSource)

nit: I think this is more than 80 chars, so please wrap.

@@ +244,5 @@
>  }
>  
>  void
> +ErrorReporter::OutputError(uint32_t aLineNumber, uint32_t aLineOffset, nsIURI* aURI, const nsACString& aSource)
> +{

Should this assert that we either didn't have an URI, or that if we did, it's the same than what we were given?

Also re. aSource? It'd be nice to have docs about what this function expects.

@@ +246,5 @@
>  void
> +ErrorReporter::OutputError(uint32_t aLineNumber, uint32_t aLineOffset, nsIURI* aURI, const nsACString& aSource)
> +{
> +  mURI = aURI;
> +  mErrorLine = NS_ConvertUTF8toUTF16(aSource);

The equivalent of this assignment is fallible in Gecko. It seems it's due to bug 1171970... It'd be nice to avoid that pitfall. Can we do it, or get a followup filed for it?

@@ +315,5 @@
>  void
> +ErrorReporter::ReportUnexpectedUnescaped(const char *aMessage,
> +                                         const nsAutoString& aParam)
> +{
> +  if (!ShouldReportErrors()) return;

nit: braces for the if, and move the return into the next line.

::: layout/style/ServoBindings.cpp
@@ +2615,5 @@
> +ErrorReporter*
> +Gecko_CreateCSSErrorReporter(ServoStyleSheet* sheet,
> +                             Loader* loader)
> +{
> +  return new ErrorReporter(sheet, loader);

Can you file a bug to avoid the heap allocation here using placement new in the same fashion we do for the StyleChildrenIterator, and reference it from here?

::: servo/components/style/gecko/data.rs
@@ +126,5 @@
>      /// Map for effective counter style rules.
>      pub counter_styles: FnvHashMap<Atom, Arc<Locked<CounterStyleRule>>>,
> +
> +    /// Loader for this document.
> +    pub loader: *mut Loader,

We can get this pretty much every time through the stylist's device. Any reason why not doing that instead? Keeping the raw pointer around isn't particularly great, though I guess the StyleSet can't outlive the doc... Would be nice to at least mention it, but I'd rather do an accessor that calls:

Gecko_GetDocumentLoader(self.stylist.device().pres_context())

(We could also get it without FFI I guess).
Attachment #8883739 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8883740 [details] [diff] [review]
Address failing CSS parser error tests

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

::: layout/style/test/test_parser_diagnostics_unprintables.html
@@ +45,5 @@
>  ];
>  
> +const isStylo = SpecialPowers.getBoolPref('layout.css.servo.enabled', false);
> +
> +// Stylo's CSS parser only reports the 'url(' token, not the actual bad URL.

Is there a bug on file for this? Maybe it's fine to just drop it, but perhaps worth opening a bug blocking bug 1365771 (same for the different columns reported in error messages).

::: servo/components/selectors/parser.rs
@@ +1105,4 @@
>              let position = input.position();
>              match input.next_including_whitespace() {
>                  Ok(Token::Delim('|')) => {
> +                    let prefix = from_cow_str(value.clone().into());

It'd be nice to avoid the unconditional clone here... I guess we could parameterice SelectorParseError with Parser, and make ExpectedNamespace(P::NamespacePrefix) instead, but I can see why that's somewhat annoying... Maybe file a followup in cssparser? I think it'd be nice, but I think selectors with explicit namespaces are somewhat uncommon, so maybe it's fine.

::: servo/components/style/gecko/error_reporter.rs
@@ +46,5 @@
> +    UnexpectedToken(Token<'a>),
> +}
> +
> +impl<'a> ErrorString<'a> {
> +    fn as_str(self) -> String {

I think as_str doesn't follow the conventions of this, into_string instead?

@@ +57,5 @@
> +}
> +
> +// This is identical to the behaviour of cssparser::serialize_identifier, except that
> +// it uses numerical escapes for a larger set of characters.
> +fn escape_css_ident(ident: &str) -> String {

I guess it's ok... Though I don't _love_ having all this mostly-duplicated code. File a bug to consider using the rust-cssparser escaping when we have stylo-only and reference it from here?

::: third_party/rust/cssparser/src/serializer.rs
@@ +129,4 @@
>              Token::SquareBracketBlock => dest.write_str("[")?,
>              Token::CurlyBracketBlock => dest.write_str("{")?,
>  
> +            Token::BadUrl(_) => dest.write_str("url(<bad url>)")?,

Well, we could potentially do better here I guess... Though this has already been reviewed upstream I suppose.

::: third_party/rust/cssparser/src/tokenizer.rs
@@ +287,4 @@
>          self.source_location(position)
>      }
>  
> +    pub fn current_source_line(&self) -> &'a str {

Add docs?
Attachment #8883740 - Flags: review?(emilio+bugs) → review+
(Assignee)

Comment 19

19 days ago
I'm going to ignore some of the style comments, since they're either about preexisting code or disagree with many other instances in the same file.
(Assignee)

Updated

19 days ago
Depends on: 1378845
(Assignee)

Updated

19 days ago
Depends on: 1378867
(Assignee)

Comment 20

19 days ago
Created attachment 8884017 [details] [diff] [review]
Part 1- Hook up Stylo CSS parser to Gecko error reporter

Final patch stripped of servo changes.
Attachment #8883739 - Attachment is obsolete: true
(Assignee)

Comment 21

19 days ago
Created attachment 8884019 [details] [diff] [review]
Part 2 - Address failing CSS parser error tests
Attachment #8883740 - Attachment is obsolete: true
(Assignee)

Updated

19 days ago
Attachment #8884017 - Attachment description: Hookk up Stylo CSS parser to Gecko error reporter → Part 1- Hook up Stylo CSS parser to Gecko error reporter
(Assignee)

Updated

19 days ago
Attachment #8884019 - Attachment description: Address failing CSS parser error tests → Part 2 - Address failing CSS parser error tests
(Assignee)

Comment 22

19 days ago
Created attachment 8884055 [details] [diff] [review]
Part 2 - Address failing CSS parser error tests
Attachment #8884019 - Attachment is obsolete: true

Comment 23

19 days ago
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f220d8060c14
Hook up Stylo CSS parser to Gecko error reporter.
https://hg.mozilla.org/integration/autoland/rev/40ac529f9ea0
Address failing CSS parser error tests.

Comment 24

19 days ago
Backout by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/da8c70e7f313
Backed out changeset 40ac529f9ea0 for build bustage CLOSED TREE
https://hg.mozilla.org/integration/autoland/rev/a13e199c5e09
Backed out changeset f220d8060c14 for build bustage CLOSED TREE a=bustage
Blocks: 1378845
No longer depends on: 1378845
(Assignee)

Comment 25

15 days ago
Created attachment 8885032 [details] [diff] [review]
Part 1 - Hook up Stylo CSS parser to Gecko error reporter
Attachment #8884017 - Attachment is obsolete: true
(Assignee)

Comment 26

15 days ago
Created attachment 8885033 [details] [diff] [review]
Part 2 - Address failing CSS parser error tests
Attachment #8884055 - Attachment is obsolete: true
(Assignee)

Comment 27

15 days ago
Created attachment 8885074 [details] [diff] [review]
Part 1 - Hook up Stylo CSS parser to Gecko error reporter
(Assignee)

Updated

15 days ago
Attachment #8885032 - Attachment is obsolete: true
(Assignee)

Comment 28

15 days ago
https://github.com/servo/servo/pull/17655

Comment 29

15 days ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/28e303771cc0
Hook up Stylo CSS parser to Gecko error reporter. r=emilio
https://hg.mozilla.org/integration/autoland/rev/81adbdebbfe3
Address failing CSS parser error tests. r=emilio

Comment 30

15 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/28e303771cc0
https://hg.mozilla.org/mozilla-central/rev/81adbdebbfe3
Status: NEW → RESOLVED
Last Resolved: 15 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Updated

12 days ago
Depends on: 1380488
(Assignee)

Updated

11 days ago
Blocks: 1381137
You need to log in before you can comment on or make changes to this bug.