Skip to content

DetailsList: Remove drag and drop border animation causing style mutations via mergeStyles#7125

Merged
dzearing merged 3 commits intomicrosoft:masterfrom
KevinTCoughlin:keco/fix-dl-styles-recalcs
Nov 16, 2018
Merged

DetailsList: Remove drag and drop border animation causing style mutations via mergeStyles#7125
dzearing merged 3 commits intomicrosoft:masterfrom
KevinTCoughlin:keco/fix-dl-styles-recalcs

Conversation

@KevinTCoughlin
Copy link
Copy Markdown
Member

@KevinTCoughlin KevinTCoughlin commented Nov 16, 2018

Pull request checklist

Description of changes

Removes the border-color animation from DetailsColumn which is not memoized due to the usage of key-frames. This is creating a new <styles/> tag per render (click, resize, etc.) causing performance issues.

The animation was originally added in #4857 and refactored to use mergeStyles as part of #5490.

Focus areas to test

How to Test

You can test this by examining the <style data-merge-styles="true" type="text/css"></style> which are added per interaction with DetailsList (anything that causes re-render). For example, clicking a row appends a node to <head> each time.

Sample Test Output

Below are the count of nodes in <head> before and after clicking 4 times on an alternating row which causes a re-render.

Before

  • Initial count on load:
document.head.childElementCount
17:32:14.266 84
  • Count after 4 clicks alternating rows
document.head.childElementCount
17:33:05.215 89

After

  • Initial count on load:
document.head.childElementCount
17:30:23.126 84
  • Count after 4 clicks alternating column header
document.head.childElementCount
17:33:50.589 85

The one style-sheet added here are the styles for DetailsList including focus, check, etc. So the increase in node count by 1 on initial click is expected behavior.

Microsoft Reviewers: Open in CodeFlow

@KevinTCoughlin
Copy link
Copy Markdown
Member Author

KevinTCoughlin commented Nov 16, 2018

@kenotron by chance, do you know how to trigger this animation? I'd like to repro it and see if we can fix using transition.

Edit: Good! Screener caught it. So it is still repro-able, will try in a bit.

@dzearing dzearing merged commit aa9193c into microsoft:master Nov 16, 2018
@dzearing
Copy link
Copy Markdown
Member

@KevinTCoughlin Happy to take a separate PR to add it back, but approving and merging this as it's a bad bug.

@KevinTCoughlin
Copy link
Copy Markdown
Member Author

FWIW I grepped the rest of OUIFR and did not see other usages of keyframes that could cause a similar problem https://github.com/OfficeDev/office-ui-fabric-react/search?l=TypeScript&q=keyframes%28

🎉

@msft-github-bot
Copy link
Copy Markdown
Contributor

🎉office-ui-fabric-react@v6.105.0 has been released which incorporates this pull request.:tada:

Handy Links:

@betrue-final-final
Copy link
Copy Markdown
Member

Hey @dzearing and @KevinTCoughlin, what exactly did we remove here? I'd love to see what it was.

@dzearing
Copy link
Copy Markdown
Member

@betrue-final-final still not sure how to reproduce this scenario completely but it seems like a border "pulse" from theme colored to transparent over a couple seconds.

We needed to remove it because it's causing a serious perf bug, but we will need to re-evaluate the right fix separately. @KevinTCoughlin will follow up with @yiminwu on this issue and see what needs to be done.

@betrue-final-final
Copy link
Copy Markdown
Member

That doesn't seem intentional then. We didn't design a pulse for drag and drop.

KevinTCoughlin added a commit to KevinTCoughlin/office-ui-fabric-react that referenced this pull request Nov 19, 2018
@arkogupta
Copy link
Copy Markdown
Contributor

@KevinTCoughlin @kenotron @yiminwu Any update on using transition ( or something else?) to add back the animation effect ?

@KevinTCoughlin
Copy link
Copy Markdown
Member Author

@KevinTCoughlin @kenotron @yiminwu Any update on using transition ( or something else?) to add back the animation effect ?

We just got DetailsList drag & drop working yesterday in #7144 which allows us to see the previous behavior. I've opened issue #7173 to track re-adding this CSS effect. Let's move the discussion there if need be instead of this closed pull request. Feel free to tackle the issue if you wish.

@KevinTCoughlin KevinTCoughlin deleted the keco/fix-dl-styles-recalcs branch November 29, 2018 18:49
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Merge-Styles bloating document head

6 participants