Skip to content
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

test whitespace in pseudo-element selector throws syntax error #16228

Closed

Conversation

@jacksonweekes
Copy link

jacksonweekes commented Apr 2, 2017

Issue #15448 - Land the test cases that should've landed with the selectors update. Tests that syntax error is thrown when pseudo-element selector contains whitespace - related to issue #15335


  • [X ] ./mach build -d does not report any errors
  • [X ] ./mach test-tidy does not report any errors
  • These changes fix #15448 (github issue number if applicable).
  • There are tests for these changes OR
  • [X ] These changes do not require tests because implements a test not included when issue #15335 was fixed

This change is Reviewable

@highfive
Copy link

highfive commented Apr 2, 2017

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @jdm (or someone else) soon.

Copy link
Member

emilio left a comment

Hi, thanks for doing this!

Seems like you need to update the test manifest so CI is green (./mach update-manifest).

We should also add tests for empty attribute selectors. There are some tests already, but we need to check [attr$=""] and [attr=^=""].

Thanks again for looking into this!

@@ -37,6 +37,8 @@ var invalidSelectors = [
{name: "Unknown pseudo-element", selector: "div::example"},
{name: "Unknown pseudo-element", selector: "::example"},
{name: "Invalid pseudo-element", selector: ":::before"},
{name: "Invalid pseudo-element", selector: ": before"},

This comment has been minimized.

@emilio

emilio Apr 2, 2017

Member

nit: You have a tab in this line that should be converted to spaces.

@jacksonweekes
Copy link
Author

jacksonweekes commented Apr 2, 2017

Hi @emilio thanks for reviewing! I have fixed the whitespace issue but just wanted to clarify about the checks for empty attribute selectors. I have added
{name: "Invalid [att=value] selector", selector: "[attr$=\"\"]"}, {name: "Invalid [att=value] selector", selector: "[attr=^=\"\"]"},
to invalid selectors, however the first test fails. I'm not sure if I've misunderstood you or the parser is just not throwing an error for this case. Any advice?

@emilio
Copy link
Member

emilio commented Apr 2, 2017

Sorry, those are not invalid selectors.

[attr$=""] is a valid selector, we should make sure it matches nothing. [attr=^=""] is indeed an invalid selector, but that's just because I made a typo (it should be [attr^=""]).

@jdm jdm assigned emilio and unassigned jdm Apr 3, 2017
@jdm
Copy link
Member

jdm commented Apr 20, 2017

@jacksonweekes Are you planning to finish this?

@bors-servo
Copy link
Contributor

bors-servo commented Jul 21, 2017

The latest upstream changes (presumably #17822) made this pull request unmergeable. Please resolve the merge conflicts.

@emilio
Copy link
Member

emilio commented Sep 7, 2017

Closing due to lack of activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.