Skip to content

/ servo

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Incorrect serialization of :not(*) #16017

Closed
bzbarsky opened this issue Mar 18, 2017 · 9 comments
Closed

Incorrect serialization of :not(*) #16017

bzbarsky opened this issue Mar 18, 2017 · 9 comments
Assignees
@SimonSapin
Labels
A-content/css A-stylo C-assigned C-has-manual-testcase

Comments

@bzbarsky
Copy link
Contributor

@bzbarsky bzbarsky commented Mar 18, 2017

Testcase (in stylo; I'm not sure how to get at this in servo proper):

<style>
  :not(*) {}
</style>
<script>
  alert(document.styleSheets[0].cssRules[0].selectorText);
</script>

This alerts :not() which is not a valid selector.
@bzbarsky
Copy link
Contributor Author

@bzbarsky bzbarsky commented Mar 18, 2017

I should note that :not(|*), and :not(*|*) also serialize to :not(), which is rather odd: those selectors mean different things, and hence presumably are represented differently internally...
@bzbarsky
Copy link
Contributor Author

@bzbarsky bzbarsky commented Mar 18, 2017 •
edited

And :not(foo|*) is serialized as the invalid selector :not(foo|). Testcase:

<style>
  @namespace foo url(foo);
  a:not(foo|*), div { color: green; }
</style>
<script>
  alert(document.styleSheets[0].cssRules[1].selectorText);
</script>
@bholley bholley added the A-stylo label Mar 26, 2017
@highfive
Copy link

@highfive highfive commented Mar 26, 2017

cc @emilio
@pyfisch
Copy link
Contributor

@pyfisch pyfisch commented Apr 8, 2017

I am working on this.

Servo testcase for the first one:

<!doctype html>
<style>
     :not(*) {}
</style>
<script>
    alert(document.styleSheets[0].cssRules[0].cssText);

</script>
  • Expected: :not(*) { }
  • Found: :not() { }
@pyfisch
Copy link
Contributor

@pyfisch pyfisch commented Apr 8, 2017

I am not working on this anymore. The problem is the serialization at https://github.com/servo/servo/blob/master/components/selectors/parser.rs#L440 but I do not understand the data structures used. args contains one element for :not(*) with an empty serialization. Somehow it should serialize to *.
@bzbarsky
Copy link
Contributor Author

@bzbarsky bzbarsky commented Apr 9, 2017

The problem is that the to_css function needs more context. It thinks it's serializing a selector at toplevel, but the rules are slightly different inside not().

And even at toplevel Servo is kinda buggy around selectors involving wildcards. See for example #16020
@pyfisch
Copy link
Contributor

@pyfisch pyfisch commented Apr 9, 2017

And this is really hard to debug because gdb does not work well with indirection and traits (but I am a beginner with it so I might have missed something) and all implementations for Debug just call to_css instead of showing the actual contents.
@pyfisch
Copy link
Contributor

@pyfisch pyfisch commented Apr 15, 2017

@highfive: assign me

Some stuff works: not(*), not(|*) and not(foo|*) are serialized correctly. Surprisingly even a normal wildcard * was serialized as the empty string before. *|foo from #16020 require more changes.
@highfive highfive added the C-assigned label Apr 15, 2017
@highfive
Copy link

@highfive highfive commented Apr 15, 2017

Hey @pyfisch! Thanks for your interest in working on this issue. It's now assigned to you!
@SimonSapin SimonSapin self-assigned this May 15, 2017
SimonSapin added a commit that referenced this issue May 16, 2017
@SimonSapin
Fix serialization of namespace and universal selectors
Loading status checks…
390e688 
Fix #16017
Fix #16020
@SimonSapin SimonSapin mentioned this issue May 16, 2017
Merged
bors-servo added a commit that referenced this issue May 16, 2017
@bors-servo
Auto merge of #16890 - servo:qname, r=<try>
Loading status checks…
77aa36f 
Fix serialization of namespace and universal selectors

Fix #16017
Fix #16020

<!-- 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/16890)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue May 16, 2017
@bors-servo
Auto merge of #16890 - servo:qname, r=emilio
Loading status checks…
825a623 
Fix serialization of namespace and universal selectors

Fix #16017
Fix #16020

<!-- 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/16890)
<!-- Reviewable:end -->
emilio added a commit to emilio/servo that referenced this issue May 16, 2017
@SimonSapin @emilio
Fix serialization of namespace and universal selectors
Unverified
This user has not uploaded their public key yet.
GPG key ID: 056B727BB9C1027C Learn about signing commits
0689652 
Fix servo#16017
Fix servo#16020
bors-servo added a commit that referenced this issue May 16, 2017
@bors-servo
Auto merge of #16890 - servo:qname, r=emilio
Loading status checks…
a038128 
Fix serialization of namespace and universal selectors

Fix #16017
Fix #16020

<!-- 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/16890)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue May 17, 2017
@bors-servo
Auto merge of #16890 - servo:qname, r=emilio
Loading status checks…
c962458 
Fix serialization of namespace and universal selectors

Fix #16017
Fix #16020

<!-- 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/16890)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@SimonSapin SimonSapin

Labels
A-content/css A-stylo C-assigned C-has-manual-testcase
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
@jdm @SimonSapin @bholley @bzbarsky @pyfisch @highfive
You can’t perform that action at this time.