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

Projects
None yet
6 participants
@matsko
Copy link
Member

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 angular/fw-animations as code owners Feb 21, 2019

@googlebot googlebot added the cla: yes label Feb 21, 2019

@matsko matsko force-pushed the matsko:manual_styles branch from 82c8e85 to a75194a Feb 21, 2019

@ngbot ngbot bot added this to the needsTriage milestone Feb 22, 2019

@matsko

This comment has been minimized.

@matsko matsko force-pushed the matsko:manual_styles branch from a75194a to a0959a6 Feb 22, 2019

@matsko

This comment has been minimized.

@mhevery
Copy link
Member

mhevery left a comment

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`

This comment has been minimized.

Copy link
@mhevery

mhevery Feb 23, 2019

Member

Thanks for the docs.

}
}

const enum SpecialCasedStylesState {

This comment has been minimized.

Copy link
@mhevery

mhevery Feb 23, 2019

Member

this could use some docs.

return result;
}

function isSpecialStyle(prop: string) {

This comment has been minimized.

Copy link
@mhevery

mhevery Feb 23, 2019

Member

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);

This comment has been minimized.

Copy link
@mhevery

mhevery Feb 23, 2019

Member

packageNonAnimatableStyles makes a lot more sense than special

fix(animations): ensure `position` and `display` styles are handled o…
…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 #24923
Closes #25635

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

@matsko matsko force-pushed the matsko:manual_styles branch from a0959a6 to de7432a Feb 26, 2019

fixup! fix(animations): ensure `position` and `display` styles are ha…
…ndled outside of keyframes/web-animations
@matsko

This comment has been minimized.

@benlesh benlesh closed this in a6ae759 Feb 27, 2019

benlesh added a commit that referenced this pull request Feb 27, 2019

fix(animations): ensure `position` and `display` styles are handled o…
…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

This comment has been minimized.

Copy link

scttcper commented Mar 12, 2019

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

@matsko matsko deleted the matsko:manual_styles branch Mar 12, 2019

@matsko

This comment has been minimized.

Copy link
Member 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;

This comment has been minimized.

Copy link
@dharders

dharders Mar 15, 2019

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.