Closed Bug 1464865 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: DOM: CSS Object Model, enhancement)

Product:
Core
Component:
DOM: CSS Object Model
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(4 files)

Testcase
350 bytes, text/html
Details
Bug 1464865: Don't let @namespace rules that aren't going to be inserted affect the namespace map.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1464865: Some trivial cleanup.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1464865: Fix some typos in WPT tests.
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
See the test-case coming.
Attached 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.
See Also:  → https://github.com/w3c/csswg-drafts/issues/2716

    
    

    
  
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: