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

Do not add spaces when serializing operators in attribute selectors #17181

Closed
Manishearth opened this issue Jun 6, 2017 · 10 comments
Closed

Do not add spaces when serializing operators in attribute selectors #17181

Manishearth opened this issue Jun 6, 2017 · 10 comments
Labels
A-content/css A-stylo E-easy

Comments

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Jun 6, 2017 •
edited by jdm

Servo serializes [a="b"] as [a = "b"], other browsers omit the spaces.

Relevant code:

servo/components/selectors/attr.rs

Line 79 in 2da8eb0

impl ToCss for AttrSelectorOperator {

Simplified testcase:

<style type="text/css">
  div [a=b]{}
</style>
<script type="text/javascript">document.write(document.styleSheets[0].cssRules[0].selectorText)</script>

This causes stylo (not run on Servo CI) failures on:

  /selectors/attribute-selectors/attribute-case/cssom.html
  /selectors/attribute-selectors/attribute-case/semantics.htm
  /selectors/attribute-selectors/attribute-case/syntax.html
@highfive
Copy link

@highfive highfive commented Jun 6, 2017

cc @emilio
@highfive
Copy link

@highfive highfive commented Jun 6, 2017

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄
@Manishearth Manishearth changed the title Do not serialize Do not add spaces when serializing operators in attribute selectors Jun 6, 2017
@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Jun 6, 2017

Are spaces removed if the input has them?
@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Jun 6, 2017

Yep. We should not print spaces here at all.
@achiwhane
Copy link
Contributor

@achiwhane achiwhane commented Jun 6, 2017

Hey, I'd be happy to work on this!
@highfive: assign me
@highfive highfive added the C-assigned label Jun 6, 2017
@highfive
Copy link

@highfive highfive commented Jun 6, 2017

Hey @achiwhane! Thanks for your interest in working on this issue. It's now assigned to you!
@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Jun 6, 2017

Yeah, this applies to all operators.
@achiwhane achiwhane mentioned this issue Jun 7, 2017
Closed
4 of 5 tasks complete
@jyc jyc mentioned this issue Jun 9, 2017
Closed
4 of 4 tasks complete
bors-servo added a commit that referenced this issue Jun 9, 2017
@bors-servo
Auto merge of #17203 - achiwhane:master, r=<try>
Loading status checks…
60d2bd5 
Remove whitespace when serializing operators in attribute selectors

<!-- 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 fix #17181.

<!-- Either: -->
- [x] 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/17203)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jun 13, 2017
@bors-servo
Auto merge of #17203 - achiwhane:master, r=Manishearth
Loading status checks…
84b2aed 
Remove whitespace when serializing operators in attribute selectors

<!-- 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 fix #17181.

<!-- Either: -->
- [x] 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/17203)
<!-- Reviewable:end -->
@jdm
Copy link
Member

@jdm jdm commented Aug 10, 2017

The patch in #17203 looks good, but it needs to remove some expected test failures that now pass.
@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Aug 14, 2017

Fixed in #17538.
@SimonSapin SimonSapin closed this Aug 14, 2017
@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Aug 14, 2017

Thanks to @rlustin for catching this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-content/css A-stylo E-easy
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

Remove whitespace when serializing operators in attribute selectors
5 participants
@jdm @SimonSapin @Manishearth @achiwhane @highfive
You can’t perform that action at this time.