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

transition-duration should not accept negative time value #15343

Closed
upsuper opened this issue Feb 2, 2017 · 10 comments
Closed

transition-duration should not accept negative time value #15343

upsuper opened this issue Feb 2, 2017 · 10 comments
Labels
A-content/css C-assigned
Milestone
Stylo: Style syst...

Comments

@upsuper
Copy link
Member

@upsuper upsuper commented Feb 2, 2017 •
edited

Spec: https://drafts.csswg.org/css-transitions/#transition-duration-property

Relevant code: https://github.com/servo/servo/blob/master/components/style/properties/longhand/box.mako.rs#L465

We probably need Time::parse_non_negative as well.
@upsuper upsuper mentioned this issue Feb 6, 2017
Closed
@jdm
Copy link
Member

@jdm jdm commented Feb 14, 2017

@simon-whitehead Does this look interesting? It involves a unit test in tests/unit/style/parsing/ that verifies that parsing a transition-duration property with a negative time value is not allowed, and modifying the parsing code to make the test pass. The tests can be run with ./mach test-unit -p style.
@simon-whitehead
Copy link
Contributor

@simon-whitehead simon-whitehead commented Feb 14, 2017

Yes it does @jdm. There is some interesting markup scattered through this file though which I am not familiar with. Do you have resources for this? Looks good otherwise - happy to have a go at it!
@jdm
Copy link
Member

@jdm jdm commented Feb 14, 2017

It's a mako template, so it's a python-based templating language.
@jdm jdm added the C-assigned label Feb 14, 2017
@simon-whitehead
Copy link
Contributor

@simon-whitehead simon-whitehead commented Feb 14, 2017

Great thanks. Good to know just so I don't break anything :) I'll get started on this tonight hopefully!
@simon-whitehead
Copy link
Contributor

@simon-whitehead simon-whitehead commented Feb 18, 2017

I will get to this in the next few days - I've just had a busy week transitioning between day jobs. Sorry for the delay!
@simon-whitehead
Copy link
Contributor

@simon-whitehead simon-whitehead commented Feb 19, 2017 •
edited

Just to clarify, should I be altering parse in the PR to error on negative values? Or should I implement parse_non_negative in the PR and make that error instead? (parse_non_negative was mentioned in the original post)
simon-whitehead added a commit to simon-whitehead/servo that referenced this issue Feb 19, 2017
@simon-whitehead
Fixes servo#15343.
c561859 
Return an error when a parsed "transition-duration" value is negative.
@upsuper
Copy link
Member Author

@upsuper upsuper commented Feb 19, 2017

I think you should implement parse_non_negative rather than altering parse, since there are properties which do accept negative time values (e.g. transition-delay).
@simon-whitehead
Copy link
Contributor

@simon-whitehead simon-whitehead commented Feb 19, 2017

Sure, but the parse method is implemented on the transition-duration "type" (still trying to wrap my head around what to call these types that are declared via the mako template). Changing the logic in the specific function you linked to in the original post shouldn't change anything other than transition-duration, should it?
@upsuper
Copy link
Member Author

@upsuper upsuper commented Feb 19, 2017

Oh, you mean that one. I thought you are talking about Time::parse. I think the better solution would be implement a Time::parse_non_negative and make transition-duration's parse use it. There are other properties which may need to parse non-negative time as well, e.g. animation-duration.
@simon-whitehead
Copy link
Contributor

@simon-whitehead simon-whitehead commented Feb 19, 2017 •
edited

Great. That sounds like a better idea. Thanks!
@simon-whitehead simon-whitehead mentioned this issue Feb 20, 2017
Closed
4 of 5 tasks complete
bors-servo added a commit that referenced this issue Feb 23, 2017
@bors-servo
Auto merge of #15658 - simon-whitehead:fix-15343, r=emilio
Loading status checks…
ba45384 
Fix #15343

<!-- Please describe your changes on the following line: -->
Adds `Time::parse_non_negative` which errors on negative values. This new method is now used by `transition_duration::parse`.

---
<!-- 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 #15343

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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/15658)
<!-- Reviewable:end -->
@Manishearth Manishearth mentioned this issue Apr 11, 2017
Merged
4 of 4 tasks complete
@hiikezoe hiikezoe mentioned this issue May 12, 2017
Merged
4 of 4 tasks complete
bors-servo added a commit that referenced this issue May 12, 2017
@bors-servo
Auto merge of #16829 - hiikezoe:disallow-negative-duration, r=<try>
Loading status checks…
5fc9f71 
Disallow negative duration for animation and transition

<!-- 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 #15343

<!-- Either: -->
- [X] There are tests for these changes written by @simon-whitehead . Thank you!

<!-- 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. -->
bors-servo added a commit that referenced this issue May 12, 2017
@bors-servo
Auto merge of #16829 - hiikezoe:disallow-negative-duration, r=<try>
Loading status checks…
9306821 
Disallow negative duration for animation and transition

<!-- 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 #15343

<!-- Either: -->
- [X] There are tests for these changes written by @simon-whitehead . Thank you!

<!-- 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/16829)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue May 13, 2017
@bors-servo
Auto merge of #16829 - hiikezoe:disallow-negative-duration, r=emilio
Loading status checks…
4613c03 
Disallow negative duration for animation and transition

<!-- 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 #15343

<!-- Either: -->
- [X] There are tests for these changes written by @simon-whitehead . Thank you!

<!-- 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/16829)
<!-- 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 C-assigned
Projects
None yet
Milestone
Stylo: Style syst...
Linked pull requests

Successfully merging a pull request may close this issue.

Disallow negative duration for animation and transition
Do not accept negative time values in transition durations
3 participants
@jdm @upsuper @simon-whitehead
You can’t perform that action at this time.