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

animation-iteration-count property is a number instead of integer. #14851

Merged
merged 1 commit into from Jan 5, 2017

Conversation

@hiikezoe
Copy link
Contributor

hiikezoe commented Jan 5, 2017

This is a revised PR for #14732.
@emilio?

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • There are tests for these changes

This change is Reviewable

@highfive
Copy link

highfive commented Jan 5, 2017

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

@highfive
Copy link

highfive commented Jan 5, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/longhand/box.mako.rs, components/style/animation.rs
  • @emilio: components/style/properties/longhand/box.mako.rs, components/style/animation.rs
@emilio
Copy link
Member

emilio commented Jan 5, 2017

This looks great.

One last thing this PR would need to do is to check the animation state more often (see the caller of tick()).

If you don't want to make that part of this PR (which is fine IMO since this doesn't regress functionality), please file an issue and cc me on it.

r=me with any of those

Thanks once again for the patch :)

@hiikezoe
Copy link
Contributor Author

hiikezoe commented Jan 5, 2017

@emilio can you please tell me about the animation state check in more detail? I don't quite understand what 'more often' means.

@emilio
Copy link
Member

emilio commented Jan 5, 2017

So right now we call tick once per animation iteration (which actually is kinda misleading).

With this patch, thinks like animation-iteration-count: 1.5 will parse but won't work I think, because we only check that number once per iteration, and we only increment it by 1.0.

@hiikezoe
Copy link
Contributor Author

hiikezoe commented Jan 5, 2017

Thank you, @emilio! Now I think I understand the problem. Filed #14858.

@emilio
Copy link
Member

emilio commented Jan 5, 2017

Awesome, thanks for filling it!

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Jan 5, 2017

📌 Commit f78cd2c has been approved by emilio

@highfive highfive assigned emilio and unassigned pcwalton Jan 5, 2017
@bors-servo
Copy link
Contributor

bors-servo commented Jan 5, 2017

Testing commit f78cd2c with merge 1d9bbfa...

bors-servo added a commit that referenced this pull request Jan 5, 2017
animation-iteration-count property is a number instead of integer.

<!-- Please describe your changes on the following line: -->
This is a revised PR  for #14732.
@emilio?
---
<!-- 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

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

<!-- 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/14851)
<!-- Reviewable:end -->
@bors-servo bors-servo merged commit f78cd2c into servo:master Jan 5, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@hiikezoe hiikezoe deleted the hiikezoe:float-iteration-count-rebased branch Jan 6, 2017
@hiikezoe hiikezoe mentioned this pull request Jan 6, 2017
3 of 3 tasks complete
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.

None yet

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