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

fix(animations): getAnimationStyle causes exceptions in older browsers #29709

Closed
wants to merge 1 commit into from

Conversation

Serginho
Copy link
Contributor

@Serginho Serginho commented Apr 4, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #24094

What is the new behavior?

Animation doesn't break in old browsers now

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@Serginho Serginho requested a review from as a code owner Apr 4, 2019
@Serginho Serginho force-pushed the fix_getAnimation_style branch from 6a6e992 to 94d1810 Compare Apr 4, 2019
@ngbot ngbot bot added this to the needsTriage milestone Apr 4, 2019
@kara kara added action: review target: patch This PR is targeted for the next patch release labels Feb 28, 2020
@pullapprove pullapprove bot requested a review from matsko Feb 28, 2020
@kara kara removed the request for review from Feb 28, 2020
@matsko
Copy link
Contributor

@matsko matsko commented Feb 28, 2020

@Serginho sorry this took us forever to get back to you on.

Could you add a test to this? You would need to export the function you modified and import it into https://github.com/angular/angular/blob/master/packages/animations/browser/test/render/css_keyframes/element_animation_style_handler_spec.ts

Then test by mocking out the a fake element a style property.

@Serginho Serginho force-pushed the fix_getAnimation_style branch 2 times, most recently from fb2f0a8 to bd07373 Compare Mar 1, 2020
@Serginho
Copy link
Contributor Author

@Serginho Serginho commented Mar 1, 2020

@matsko I didn't do any additional test because getAnimationStyle is already tested, for example here:

assertStyle(element, 'animation-name', 'fooAnimation');
handler.apply();
assertStyle(element, 'animation-name', 'fooAnimation, barAnimation');

So I assume you want a testcase for this bug, and this is what I did in the updated commit.
The fix is squashed, formatted, and rebased to master.

Let me know if you need something else.

@Serginho Serginho force-pushed the fix_getAnimation_style branch 3 times, most recently from 6f9210a to 9625229 Compare Mar 1, 2020
@petebacondarwin petebacondarwin requested review from petebacondarwin and removed request for matsko Aug 20, 2020
@pullapprove pullapprove bot requested a review from crisbeto Aug 20, 2020
Copy link
Member

@petebacondarwin petebacondarwin left a comment

LGTM - please can you add some more information to the commit message body?

@petebacondarwin petebacondarwin added the action: presubmit A standard presubmit is running / required label Nov 10, 2020
@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Nov 10, 2020

@Serginho - would you mind rebasing this PR on top of master. Then I will try to chase reviewers to get it merged.

@Serginho Serginho force-pushed the fix_getAnimation_style branch from 9625229 to 8865d0c Compare Nov 16, 2020
Serginho added a commit to Serginho/angular that referenced this issue Nov 16, 2020
PR angular#29709 getAnimationStyle causes exceptions in older browsers
@Serginho Serginho force-pushed the fix_getAnimation_style branch from 8865d0c to ba35a0f Compare Nov 16, 2020
@Serginho
Copy link
Contributor Author

@Serginho Serginho commented Nov 17, 2020

@petebacondarwin Sure.

Rebased and ready to merge.

Copy link
Member

@petebacondarwin petebacondarwin left a comment

Thanks for rebasing @Serginho. Just one minor correction in the test.

PR angular#29709 getAnimationStyle causes exceptions in older browsers
@Serginho Serginho force-pushed the fix_getAnimation_style branch from ba35a0f to 2e26399 Compare Nov 17, 2020
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Great! Thanks.

@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Nov 18, 2020

Presubmit.

@AndrewKushnir AndrewKushnir added action: merge PR author is ready for this to merge and removed action: review action: presubmit A standard presubmit is running / required labels Nov 20, 2020
AndrewKushnir pushed a commit that referenced this issue Nov 20, 2020
#29709)

PR #29709 getAnimationStyle causes exceptions in older browsers

PR Close #29709
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Nov 20, 2020

@Serginho this PR is now merged, thanks for contributing to Angular!

@angular-automatic-lock-bot
Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Dec 21, 2020

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge PR author is ready for this to merge cla: yes comp: animations target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants