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): process shorthand margin and padding styles correctly #35701

Closed
wants to merge 2 commits into from

Conversation

matsko
Copy link
Contributor

@matsko matsko commented Feb 26, 2020

Prior to this patch, the margin and padding properties were not
detected properly by Firefox due to them being shorthand properties.
This patch ensures that both margin and padding are converted
read as top right bottom left in the event that the shorthand
property detection fails for auto-styling in Angular animations.

Fix #35463 (FW-1886)

@matsko matsko added area: animations target: patch This PR is targeted for the next patch release labels Feb 26, 2020
@matsko matsko requested a review from kara February 26, 2020 22:59
@ngbot ngbot bot added this to the needsTriage milestone Feb 26, 2020
@matsko matsko force-pushed the optimize_animation_code branch 5 times, most recently from c4ba8f5 to 55a9fb4 Compare February 27, 2020 21:16
@matsko
Copy link
Contributor Author

matsko commented Feb 27, 2020

@matsko
Copy link
Contributor Author

matsko commented Feb 28, 2020

Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

LGTM once the type cast is fixed

Comment on lines +246 to +248
* style value for the element. Firefox doesn't support reading the shorthand
* forms of margin/padding and for this reason this function needs to account
* for that.
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 documenting this 👍

if (value.length === 0 && (prop === 'margin' || prop === 'padding')) {
// reconstruct the padding/margin value as `top right bottom left`
const propTop = (prop + 'Top') as 'marginTop' | 'paddingTop';
const propRight = (prop + 'Right') as 'marginRight' | 'paddingLeft';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const propRight = (prop + 'Right') as 'marginRight' | 'paddingLeft';
const propRight = (prop + 'Right') as 'marginRight' | 'paddingRight';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in the fixup

@kara kara 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 28, 2020
…ectly

Prior to this patch, the `margin` and `padding` properties were not
detected properly by Firefox due to them being shorthand properties.
This patch ensures that both `margin` and `padding` are converted
read as `top right bottom left` in the event that the shorthand
property detection fails for auto-styling in Angular animations.

Fix angular#35463 (FW-1886)
@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 28, 2020
@atscott atscott closed this in 35c9f0d Mar 2, 2020
atscott pushed a commit that referenced this pull request Mar 2, 2020
…ectly (#35701)

Prior to this patch, the `margin` and `padding` properties were not
detected properly by Firefox due to them being shorthand properties.
This patch ensures that both `margin` and `padding` are converted
read as `top right bottom left` in the event that the shorthand
property detection fails for auto-styling in Angular animations.

Fix #35463 (FW-1886)

PR Close #35701
atscott added a commit to atscott/angular that referenced this pull request Mar 4, 2020
…les correctly (angular#35701)"

This reverts commit 35c9f0d, breaks
internal tests
atscott added a commit that referenced this pull request Mar 4, 2020
…les correctly (#35701)" (#35847)

This reverts commit 35c9f0d, breaks
internal tests

PR Close #35847
atscott added a commit that referenced this pull request Mar 4, 2020
…les correctly (#35701)" (#35847)

This reverts commit 35c9f0d, breaks
internal tests

PR Close #35847
@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 Apr 2, 2020
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
Development

Successfully merging this pull request may close these issues.

Item with animation not being removed during exit animation on Firefox
3 participants