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): ensure position and display styles are handled outside of keyframes/web-animations #28911

Closed
wants to merge 2 commits into from

Conversation

matsko
Copy link
Contributor

@matsko matsko commented Feb 21, 2019

When web-animations and/or CSS keyframes are used for animations certain
CSS style values (such as display and position) may be ignored by a
keyframe-based animation. Angular should special-case these styles to
ensure that they get applied as inline styles throughout the duration of
the animation.

Closes #24923
Closes #25635

Jira Issue: FW-1091
Jira Issue: FW-1092

@matsko matsko requested review from a team as code owners February 21, 2019 23:28
@ngbot ngbot bot added this to the needsTriage milestone Feb 22, 2019
@matsko matsko added the target: patch This PR is targeted for the next patch release label Feb 22, 2019
@matsko
Copy link
Contributor Author

matsko commented Feb 22, 2019

@matsko
Copy link
Contributor Author

matsko commented Feb 22, 2019

Copy link
Contributor

@mhevery mhevery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, we some cleanup.

Please change all assurances of special to nonAnimatable as I think that makes the code more readable.

* will ignore them. This function is designed to detect those special cased styles and
* return a container that will be executed at the start and end of the animation.
*
* @returns an instance of `SpecialCasedStyles` if any special styles are detected otherwise `null`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the docs.

}
}

const enum SpecialCasedStylesState {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could use some docs.

return result;
}

function isSpecialStyle(prop: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of special could we use nonAnimatable that would be more descriptive.

@@ -66,7 +67,8 @@ export class WebAnimationsDriver implements AnimationDriver {

keyframes = keyframes.map(styles => copyStyles(styles, false));
keyframes = balancePreviousStylesIntoKeyframes(element, keyframes, previousStyles);
return new WebAnimationsPlayer(element, keyframes, playerOptions);
const specialStyles = packageSpecialStyles(element, keyframes);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

packageNonAnimatableStyles makes a lot more sense than special

@mhevery mhevery added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 23, 2019
…utside of keyframes/web-animations

When web-animations and/or CSS keyframes are used for animations certain
CSS style values (such as `display` and `position`) may be ignored by a
keyframe-based animation. Angular should special-case these styles to
ensure that they get applied as inline styles throughout the duration of
the animation.

Closes angular#24923
Closes angular#25635

Jira Issue: FW-1091
Jira Issue: FW-1092
…ndled outside of keyframes/web-animations
@matsko
Copy link
Contributor Author

matsko commented Feb 26, 2019

@matsko matsko added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Feb 27, 2019
@benlesh benlesh closed this in a6ae759 Feb 27, 2019
benlesh pushed a commit that referenced this pull request Feb 27, 2019
…utside of keyframes/web-animations (#28911)

When web-animations and/or CSS keyframes are used for animations certain
CSS style values (such as `display` and `position`) may be ignored by a
keyframe-based animation. Angular should special-case these styles to
ensure that they get applied as inline styles throughout the duration of
the animation.

Closes #24923
Closes #25635

Jira Issue: FW-1091
Jira Issue: FW-1092

PR Close #28911
@scttcper
Copy link

@mhevery @matsko this seems like a breaking change. Was using display in animations already deprecated?

@matsko matsko deleted the manual_styles branch March 12, 2019 22:36
@matsko
Copy link
Contributor Author

matsko commented Mar 12, 2019

@scttcper this PR is changing angular to permit the use of display and position (not blocking it). The problem before this patch was that web animations didn't allow the use of display at all, but CSS keyframes did, but it was the reverse for position. The styles are special cased in a different area of code (which is run at the same time as the animation) to ensure that both display and position are properly applied during the animation.

setStyles(this._element, this._endStyles);
this._endStyles = null;
}
this._state = SpecialCasedStylesState.Started;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matsko Should line 79 be

this._state = SpecialCasedStylesState.Finished; ?

I think the order of the SpecialCasedStylesState enum allowed this to still work. Please excuse me if this line was intentional for other purposes!

@angular-automatic-lock-bot
Copy link

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 Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: animations cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
6 participants