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

Jump to bottom

{background,mask,object}-position incorrectly accepts several invalid three-value forms #15488

Closed
upsuper opened this issue Feb 10, 2017 · 9 comments · Fixed by #15702
Closed

{background,mask,object}-position incorrectly accepts several invalid three-value forms #15488

upsuper opened this issue Feb 10, 2017 · 9 comments · Fixed by #15702
Labels
A-content/css Interacting with CSS from web content (parsing, serializing, introspection) C-assigned There is someone working on resolving the issue E-less-complex Straightforward. Recommended for a new contributor.
Milestone
Stylo: Style syst...

Comments

@upsuper
Copy link
Contributor

upsuper commented Feb 10, 2017 •
edited

The formal syntax of background-position's <position> value is:

<position> = [
  [ left | center | right | top | bottom | <length-percentage> ]
|
  [ left | center | right | <length-percentage> ]
  [ top | center | bottom | <length-percentage> ]
|
  [ center | [ left | right ] <length-percentage>? ] &&
  [ center | [ top | bottom ] <length-percentage>? ]
]

In which the following values are invalid but accepted (and can be serialized to) in Servo:

  • 50% bottom 10%
  • right 10% 50%
  • 20% 30% 40%

Spec: https://drafts.csswg.org/css-backgrounds-3/#the-background-position

The relevant code is https://github.com/servo/servo/blob/master/components/style/values/specified/position.rs#L177-L223 for parsing and https://github.com/servo/servo/blob/master/components/style/values/specified/position.rs#L34-L57 for serialization.

Probably and easy one.
@upsuper upsuper added A-content/css Interacting with CSS from web content (parsing, serializing, introspection) E-less-complex Straightforward. Recommended for a new contributor. labels Feb 10, 2017
@highfive
Copy link

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. 😄

@upsuper
Copy link
Contributor Author

upsuper commented Feb 10, 2017

The same code would also affect mask-position which has the same issue.

@upsuper upsuper changed the title background-position incorrectly accepts several invalid three-value forms {background,mask}-position incorrectly accepts several invalid three-value forms Feb 10, 2017
@upsuper
Copy link
Contributor Author

upsuper commented Feb 10, 2017

The same code affects circle() and ellipse() functions as well.

@karan1276
Copy link
Contributor

can i do this ?

@KiChjang
Copy link
Contributor

@karan1276 Do you mind leaving this open to other contributors? You've already claimed a few issues, and you still have to address review comments in #15468.

@karan1276
Copy link
Contributor

karan1276 commented Feb 11, 2017 •
edited

@KiChjang Okay, ill land all other commits first :)

@shubDhond
Copy link
Contributor

Can I take a go at this?

@jdm
Copy link
Member

jdm commented Feb 11, 2017

Please do!

@KiChjang KiChjang added the C-assigned There is someone working on resolving the issue label Feb 11, 2017
@upsuper upsuper changed the title {background,mask}-position incorrectly accepts several invalid three-value forms {background,mask,object}-position incorrectly accepts several invalid three-value forms Feb 14, 2017
@shubDhond shubDhond mentioned this issue Feb 23, 2017
Merged
4 tasks
bors-servo pushed a commit that referenced this issue Mar 3, 2017
Auto merge of #15702 - shubDhond:master, r=<try>
421fd40 
Invalid three value positions are no longer accepted

<!-- Please describe your changes on the following line: -->
- Position parse no longer accepts invalid three value positions

---
<!-- 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 #15488 .

<!-- Either: -->
- [X] There are tests for these changes OR

<!-- 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/15702)
<!-- Reviewable:end -->
@nox
Copy link
Contributor

nox commented Mar 3, 2017 •
edited

This does not change the serialization code, but I guess that doesn't matter? Edit: intended to comment this on the PR...

bors-servo pushed a commit that referenced this issue Mar 4, 2017
Auto merge of #15702 - shubDhond:master, r=upsuper
8c4931f 
Invalid three value positions are no longer accepted

<!-- Please describe your changes on the following line: -->
- Position parse no longer accepts invalid three value positions

---
<!-- 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 #15488 .

<!-- Either: -->
- [X] There are tests for these changes OR

<!-- 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/15702)
<!-- Reviewable:end -->
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 Interacting with CSS from web content (parsing, serializing, introspection) C-assigned There is someone working on resolving the issue E-less-complex Straightforward. Recommended for a new contributor.
Projects
None yet
Milestone
Stylo: Style system mochitests
Development

Successfully merging a pull request may close this issue.

Invalid three value positions are no longer accepted shubDhond/servo
7 participants
@jdm @nox @upsuper @KiChjang @highfive @karan1276 @shubDhond