Namespace rules inserted via CSSOM are visible even if they're never inserted.

RESOLVED FIXED in Firefox 62

Status

()

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: emilio, Assigned: emilio)

Tracking

unspecified
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(4 attachments)

See the test-case coming.
Posted file Testcase
Component: CSS Parsing and Computation → DOM: CSS Object Model
Assignee: nobody → emilio
Of course we still don't remove from the map, nor do other browsers... I think we could consider removing access to @namespace from the OM altogether IMO, it'd be nice.
Comment on attachment 8981266 [details]
Bug 1464865: Don't let @namespace rules that aren't going to be inserted affect the namespace map.

https://reviewboard.mozilla.org/r/247360/#review253402

r=me with nits below addressed.

::: servo/components/style/stylesheets/rule_parser.rs:59
(Diff revision 1)
>      /// Whether we have tried to parse was invalid due to being in the wrong
>      /// place (e.g. an @import rule was found while in the `Body` state). Reset
>      /// to `false` when `take_had_hierarchy_error` is called.

This comment needs update.

::: servo/components/style/stylesheets/rule_parser.rs:92
(Diff revision 1)
> -    /// Returns whether we previously tried to parse a rule that was invalid
> -    /// due to being in the wrong place (e.g. an @import rule was found after
> -    /// a regular style rule).  The state of this flag is reset when this
> -    /// function is called.
> -    pub fn take_had_hierarchy_error(&mut self) -> bool {
> -        let had_hierarchy_error = self.had_hierarchy_error;
> +    /// Checks whether we can parse a rule that would transition us to
> +    /// `new_state`.
> +    ///
> +    /// This is usually a simple branch, but we may need more bookkeeping if
> +    /// doing `insertRule` from CSSOM.
> +    fn check_state(&mut self, new_state: State) -> bool {

Since you are altering `dom_error` in all branches which return `false`, it probably makes more sense to have this function take a non-mut `&self` and return an `Result<(), RulesMutateError>` then update the `dom_error` in the callsite.

That should make it clearer that it's either valid or contains a mutate error.

::: testing/web-platform/tests/css/cssom/at-namespace.html:4
(Diff revision 1)
> +<!doctype html>
> +<title>CSS Test: @namespace in CSSOM is not severely broken</title>
> +<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
> +<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1464865">

I don't think we should include Gecko bug in wpt...
Attachment #8981266 - Flags: review?(xidorn+moz) → review+
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #4)
> Comment on attachment 8981266 [details]
> Bug 1464865: Don't let @namespace rules that aren't going to be inserted
> affect the namespace map.
> 
> https://reviewboard.mozilla.org/r/247360/#review253402
> 
> r=me with nits below addressed.
> 
> ::: servo/components/style/stylesheets/rule_parser.rs:59
> (Diff revision 1)
> >      /// Whether we have tried to parse was invalid due to being in the wrong
> >      /// place (e.g. an @import rule was found while in the `Body` state). Reset
> >      /// to `false` when `take_had_hierarchy_error` is called.
> 
> This comment needs update.

Good catch, will do.

> ::: servo/components/style/stylesheets/rule_parser.rs:92
> (Diff revision 1)
> > -    /// Returns whether we previously tried to parse a rule that was invalid
> > -    /// due to being in the wrong place (e.g. an @import rule was found after
> > -    /// a regular style rule).  The state of this flag is reset when this
> > -    /// function is called.
> > -    pub fn take_had_hierarchy_error(&mut self) -> bool {
> > -        let had_hierarchy_error = self.had_hierarchy_error;
> > +    /// Checks whether we can parse a rule that would transition us to
> > +    /// `new_state`.
> > +    ///
> > +    /// This is usually a simple branch, but we may need more bookkeeping if
> > +    /// doing `insertRule` from CSSOM.
> > +    fn check_state(&mut self, new_state: State) -> bool {
> 
> Since you are altering `dom_error` in all branches which return `false`, it
> probably makes more sense to have this function take a non-mut `&self` and
> return an `Result<(), RulesMutateError>` then update the `dom_error` in the
> callsite.
> 
> That should make it clearer that it's either valid or contains a mutate
> error.

I'm not sure I love the result, seems more manual book-keeping at the caller's side:

  if let Err(err) = self.check_state(State::Imports) {
    self.dom_error = Some(err);
    return Err(...);
  }

> 
> ::: testing/web-platform/tests/css/cssom/at-namespace.html:4
> (Diff revision 1)
> > +<!doctype html>
> > +<title>CSS Test: @namespace in CSSOM is not severely broken</title>
> > +<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
> > +<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1464865">
> 
> I don't think we should include Gecko bug in wpt...

I actually think it's really useful for context.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> > ::: servo/components/style/stylesheets/rule_parser.rs:92
> > (Diff revision 1)
> > > -    /// Returns whether we previously tried to parse a rule that was invalid
> > > -    /// due to being in the wrong place (e.g. an @import rule was found after
> > > -    /// a regular style rule).  The state of this flag is reset when this
> > > -    /// function is called.
> > > -    pub fn take_had_hierarchy_error(&mut self) -> bool {
> > > -        let had_hierarchy_error = self.had_hierarchy_error;
> > > +    /// Checks whether we can parse a rule that would transition us to
> > > +    /// `new_state`.
> > > +    ///
> > > +    /// This is usually a simple branch, but we may need more bookkeeping if
> > > +    /// doing `insertRule` from CSSOM.
> > > +    fn check_state(&mut self, new_state: State) -> bool {
> > 
> > Since you are altering `dom_error` in all branches which return `false`, it
> > probably makes more sense to have this function take a non-mut `&self` and
> > return an `Result<(), RulesMutateError>` then update the `dom_error` in the
> > callsite.
> > 
> > That should make it clearer that it's either valid or contains a mutate
> > error.
> 
> I'm not sure I love the result, seems more manual book-keeping at the
> caller's side:
> 
>   if let Err(err) = self.check_state(State::Imports) {
>     self.dom_error = Some(err);
>     return Err(...);
>   }

The good side is that you would never forget to handle it.

Maybe something like

  self.check_state(State::Imports)
      .map_err(|err| { self.dom_error = Some(err); ... })?;

would make you feel better?

> > ::: testing/web-platform/tests/css/cssom/at-namespace.html:4
> > (Diff revision 1)
> > > +<!doctype html>
> > > +<title>CSS Test: @namespace in CSSOM is not severely broken</title>
> > > +<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
> > > +<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1464865">
> > 
> > I don't think we should include Gecko bug in wpt...
> 
> I actually think it's really useful for context.

Maybe your CSSWG issue is a better fit. Also I doubt whether it should be in <link rel="help">. As a context, it can probably just be a comment around the test... I suspect <link rel="help"> should always refer to the spec.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad877938d752
Don't let @namespace rules that aren't going to be inserted affect the namespace map. r=xidorn
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11221 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Backed out changeset ad877938d752 (bug 1464865) for wpt3 failures in /css/cssom/insertRule-import-no-index.htm on a CLOSED TREE
Problematic push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ad877938d7527cfff12168661d0a81a1c1077fd3&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=180671515
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=180671515&repo=mozilla-inbound&lineNumber=6245
Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d4c518223e7f9b0b0622d651d96a9491fd776fe2&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified

[task 2018-05-29T09:43:31.085Z] 09:43:31     INFO - TEST-UNEXPECTED-FAIL | /css/cssom/insertRule-import-no-index.html | inserRule with import and omitted index argument - assert_throws: function "function() { sheet.insertRule("p { color: green; }"); }" did not throw
[task 2018-05-29T09:43:31.087Z] 09:43:31     INFO - @http://web-platform.test:8000/css/cssom/insertRule-import-no-index.html:21:9
[task 2018-05-29T09:43:31.087Z] 09:43:31     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1538:20
[task 2018-05-29T09:43:31.087Z] 09:43:31     INFO - test@http://web-platform.test:8000/resources/testharness.js:548:9
[task 2018-05-29T09:43:31.087Z] 09:43:31     INFO - @http://web-platform.test:8000/css/cssom/insertRule-import-no-index.html:19:5
[task 2018-05-29T09:43:31.087Z] 09:43:31     INFO - 
[task 2018-05-29T09:43:31.087Z] 09:43:31     INFO - TEST-UNEXPECTED-FAIL | /css/cssom/insertRule-import-no-index.html | insertRule with import and undefined index argument - assert_equals: expected 1 but got 2
Flags: needinfo?(emilio)
Flags: needinfo?(emilio)
Comment on attachment 8981266 [details]
Bug 1464865: Don't let @namespace rules that aren't going to be inserted affect the namespace map.

This could get a once over again. I basically just added some missing checks that are now relevant in insertRule.

Let me know if you prefer to duplicate the "next rule" check in insertRule instead, which may be worth it to avoid doing that check for every rule in the body...
Attachment #8981266 - Flags: review+ → review?(xidorn+moz)
Comment on attachment 8981266 [details]
Bug 1464865: Don't let @namespace rules that aren't going to be inserted affect the namespace map.

https://reviewboard.mozilla.org/r/247360/#review253504

I think this is fine. I would prefer we keep it consistent that we reject an invalid rule before we actually start parsing it.
Attachment #8981266 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8981384 [details]
Bug 1464865: Some trivial cleanup.

https://reviewboard.mozilla.org/r/247510/#review253506
Attachment #8981384 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8981385 [details]
Bug 1464865: Fix some typos in WPT tests.

https://reviewboard.mozilla.org/r/247512/#review253510
Attachment #8981385 - Flags: review?(xidorn+moz) → review+
Upstream PR was closed without merging
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/09d9ee3fe4bb
Don't let @namespace rules that aren't going to be inserted affect the namespace map. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d23f192ffe3
Some trivial cleanup. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/85572fbd6673
Fix some typos in WPT tests. r=xidorn
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
You need to log in before you can comment on or make changes to this bug.