Some fixes to selector serialization re: namespaces and universal selector #17537
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
highfive
added
the
S-awaiting-review
|
@bors-servo try @jyc this looks good to me, is this waiting for anything in particular? Does it have tests? |
|
⌛ Trying commit eaa1d40 with merge 9e0d601178f6b39d92c9a71bf94b26c592f6c31b... |
|
💔 Test failed - mac-rel-wpt1 |
|
Hi @emilio! Thanks, sorry for not being clear; was planning to run it to see what tests changed. It looks like it failed due to something in the infrastructure-- @bors-servo retry |
bors-servo
pushed a commit
that referenced
this pull request
Jun 29, 2017
[WIP] Replace Namespace components which map to the default namespace with DefaultNamespace It seems we don't need to preserve the original prefix name, and this conveniently leads to_css to now implement CSSOM's requirement to elide the namespace prefix during serialization when it maps to the default namespace. https://drafts.csswg.org/cssom/#serialize-a-simple-selector <!-- Please describe your changes on the following line: --> --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes is part of a series to fix #17182 <!-- Either: --> I am running this to identify what tests will fail; as the other PRs in the series are merged, I believe the tests that pass will change as well. - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17537) <!-- Reviewable:end -->
|
☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev |
5 tasks
|
@bors-servo: retry EDIT: oops, misread. it's not merged! |
jyc
changed the title
jdm
added
the
S-blocked-on-external
5 tasks
|
jyc
force-pushed
the
default-namespace-serialization
branch
from
914f7c8
to
3677587
Compare
…DefaultNamespace. It seems we don't need to preserve the original prefix name, and this conveniently leads to_css to now implement CSSOM's requirement to elide the namespace prefix during serialization when it maps to the default namespace. https://drafts.csswg.org/cssom/#serialize-a-simple-selector
|
The tests used a parser with no default namespace but asserted that *| should be preserved in the output, which is contrary to the tests in |
jyc
force-pushed
the
default-namespace-serialization
branch
from
3677587
to
4269ce3
Compare
|
@bors-servo: retry |
|
@bors-servo: try |
bors-servo
pushed a commit
that referenced
this pull request
Jul 14, 2017
Replace Namespace components which map to the default namespace with DefaultNamespace - Fix eliding default namespace when serializing - Fix shortest serialization property when namespace prefix is `*|` and there is no default namespace <!-- Please describe your changes on the following line: --> --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes is part of a series to fix #17182 <!-- Either: --> I'd like to land #17501 first, because it allows some tests for this to work. - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17537) <!-- Reviewable:end -->
|
💔 Test failed - mac-rel-wpt1 |
bors-servo
pushed a commit
that referenced
this pull request
Jul 17, 2017
Some fixes to selector serialization re: namespaces and universal selector - Fix eliding default namespace when serializing - Fix shortest serialization property when namespace prefix is `*|` and there is no default namespace - Omit universal selector when serializing to match `cssom/serialize-namespaced-type-selectors` (again so we get the shortest serialization) <!-- Please describe your changes on the following line: --> --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes is part of a series to fix #17182 <!-- Either: --> I'd like to land #17501 first, because it allows some tests for this to work. - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17537) <!-- Reviewable:end -->
|
☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev |
|
Problem was the fake combinators we have for pseudo element selectors. Should be good now! @bors-servo r?emilio |
|
r? @emilio |
emilio
reviewed
Jul 17, 2017
There was a problem hiding this comment.
Looks good! Mostly nits.
Also maybe @SimonSapin wants to take a look.
components/selectors/parser.rs
Outdated
| for item in compound.iter() { | ||
| item.to_css(dest)?; | ||
|
|
||
| // https://www.w3.org/TR/cssom-1/#serializing-selectors |
There was a problem hiding this comment.
nit: We usually link to drafts.csswg.org.
components/selectors/parser.rs
Outdated
| // | ||
| // If we are in this case, we continue to the next iteration of the | ||
| // `for compound in compound_selectors` loop. | ||
| let namespace = if compound.len() > 0 { |
components/selectors/parser.rs
Outdated
| // If we are in this case, we continue to the next iteration of the | ||
| // `for compound in compound_selectors` loop. | ||
| let namespace = if compound.len() > 0 { | ||
| let (namespace, first_non_namespace) = match &compound[0] { |
There was a problem hiding this comment.
nit: Why the reference? I think it'd be clearer to do:
let has_namespace = matches!(compound[0], Component::ExplicitAnyNamespace | ...);And derive the rest from that.
There was a problem hiding this comment.
Hm, I think it is clearer to keep the logic as close to where it is used as possible. Also, we don't quite want has_namespace, I think we instead want something like can_elide_namespace. Going to include proposed change, though.
components/selectors/parser.rs
Outdated
| // in cssom/serialize-namespaced-type-selectors.html, which the | ||
| // following code tries to match. | ||
| for simple in compound.iter() { | ||
| if let &Component::ExplicitUniversalType = simple { |
There was a problem hiding this comment.
nit: we usually do:
if let Component::ExplicitUniversalType = *simple {or,
if matches!(*simple, Component::ExplicitUniversalType) {There was a problem hiding this comment.
Didn't know about the matches! macro, thanks!
components/selectors/parser.rs
Outdated
| // compound selector, so we don't have to worry about the | ||
| // real namespace being in a different `compound`. | ||
| match namespace { | ||
| Some(&Component::DefaultNamespace(_)) | None => continue, |
There was a problem hiding this comment.
nit: You can also use matches! here if you prefer, and avoid the _ => () thing.
components/selectors/parser.rs
Outdated
| input: &'i str, | ||
| parser: &DummyParser, | ||
| expected: Option<&'a str> | ||
| ) -> Result<SelectorList<DummySelectorImpl>, ParseError<'i, SelectorParseError<'i, ()>>> |
There was a problem hiding this comment.
nit: I think the brace should be in the previous line per https://github.com/rust-lang-nursery/fmt-rfcs
components/selectors/parser.rs
Outdated
| @@ -2060,7 +2211,10 @@ pub mod tests { | |||
|
|
|||
| #[test] | |||
| fn test_universal() { | |||
| let selector = &parse("*|*::before").unwrap().0[0]; | |||
| let selector = &parse_ns("*|*::before", &DummyParser { | |||
| default_ns: Some(DummyAtom::from("https://mozilla.org")), | |||
There was a problem hiding this comment.
nit: Maybe having a DummyParser::default() or something like that would be nice, to avoid some duplication here.
SimonSapin
reviewed
Jul 17, 2017
components/selectors/parser.rs
Outdated
| fn parse<'i>(input: &'i str) -> Result<SelectorList<DummySelectorImpl>, | ||
| ParseError<'i, SelectorParseError<'i, ()>>> { | ||
| fn parse<'i>(input: &'i str) | ||
| -> Result<SelectorList<DummySelectorImpl>, ParseError<'i, SelectorParseError<'i, ()>>> |
There was a problem hiding this comment.
No need to change this now, but next time you’d like to make whitespace-only changes please make them in a separate commit.
Once again it seems we don't need to preserve the original prefix name,
and this lets to_css serialize to the shortest form when there is no
default namespace and the *| prefix is used.
Selectors § 6.1.1 says:
Element type selectors that have no namespace component (no namespace
separator) represent elements without regard to the element's namespace
(equivalent to "*|") unless a default namespace has been declared for
namespaced selectors (e.g. in CSS, in the style sheet). If a default
namespace has been declared, such selectors will represent only elements
in the default namespace.
Then if there is no default namespace, *| (which we write as
QNamePrefix::ExplicitAnyNamespace) is equivalent to what we write as
QNamePrefix::ImplicitAnyNamespace; the latter has a shorter
serialization, so we should use that.
If omitting the universal selector in the serialization is possible, we should do it so we obtain a shorter serialization (to match the behavior asserted in cssom/serialize-namespaced-type-selectors.html). For example, if someone writes *|*::before and there is no default namespace, we should serialize to ::before; however, if there is a default namespace, we should serialize to *|*::before. (This is the test case "Universal selector in any namespace followed by pseudo element). This matches the behavior implemented by WebKit; that one case in particular isn't implemented by Gecko, but other cases where the universal selector should be elided are implemented by Gecko but were not previously by Servo.
jyc
force-pushed
the
default-namespace-serialization
branch
from
b19434b
to
1301bcd
Compare
|
@bors-servo: try |
bors-servo
pushed a commit
that referenced
this pull request
Jul 17, 2017
Some fixes to selector serialization re: namespaces and universal selector - Fix eliding default namespace when serializing - Fix shortest serialization property when namespace prefix is `*|` and there is no default namespace - Omit universal selector when serializing to match `cssom/serialize-namespaced-type-selectors` (again so we get the shortest serialization) <!-- Please describe your changes on the following line: --> --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes is part of a series to fix #17182 <!-- Either: --> I'd like to land #17501 first, because it allows some tests for this to work. - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17537) <!-- Reviewable:end -->
|
💔 Test failed - linux-rel-wpt |
|
@bors-servo retry |
bors-servo
pushed a commit
that referenced
this pull request
Jul 17, 2017
Some fixes to selector serialization re: namespaces and universal selector - Fix eliding default namespace when serializing - Fix shortest serialization property when namespace prefix is `*|` and there is no default namespace - Omit universal selector when serializing to match `cssom/serialize-namespaced-type-selectors` (again so we get the shortest serialization) <!-- Please describe your changes on the following line: --> --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes is part of a series to fix #17182 <!-- Either: --> I'd like to land #17501 first, because it allows some tests for this to work. - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17537) <!-- Reviewable:end -->
|
☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev |
|
@bors-servo r+ Awesome, thanks! |
|
📌 Commit 1301bcd has been approved by |
highfive
added
S-awaiting-merge
bors-servo
pushed a commit
that referenced
this pull request
Jul 17, 2017
Some fixes to selector serialization re: namespaces and universal selector - Fix eliding default namespace when serializing - Fix shortest serialization property when namespace prefix is `*|` and there is no default namespace - Omit universal selector when serializing to match `cssom/serialize-namespaced-type-selectors` (again so we get the shortest serialization) <!-- Please describe your changes on the following line: --> --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes is part of a series to fix #17182 <!-- Either: --> I'd like to land #17501 first, because it allows some tests for this to work. - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17537) <!-- Reviewable:end -->
|
☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev |
highfive
removed
the
S-awaiting-merge
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
*|and there is no default namespacecssom/serialize-namespaced-type-selectors(again so we get the shortest serialization)./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsan+bwhen serializing #17182I'd like to land #17501 first, because it allows some tests for this to work.
This change is