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)
Core
DOM: CSS Object Model
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
References
Details
Attachments
(4 files)
See the test-case coming.
Assignee | ||
Comment 1•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Component: CSS Parsing and Computation → DOM: CSS Object Model
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → emilio
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•6 years ago
|
||
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 4•6 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 5•6 years ago
|
||
(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.
Comment 6•6 years ago
|
||
(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.
Comment 10•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(emilio)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•6 years ago
|
||
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 15•6 years ago
|
||
mozreview-review |
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 16•6 years ago
|
||
mozreview-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 17•6 years ago
|
||
mozreview-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
Comment 19•6 years ago
|
||
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.
Comment 21•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/09d9ee3fe4bb
https://hg.mozilla.org/mozilla-central/rev/4d23f192ffe3
https://hg.mozilla.org/mozilla-central/rev/85572fbd6673
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in
before you can comment on or make changes to this bug.
Description
•