Navigation Menu

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

Jump to bottom

FocusZone: removing keydown listener correctly. #11689

Merged
merged 3 commits into from Jan 13, 2020
Merged

FocusZone: removing keydown listener correctly. #11689

merged 3 commits into from Jan 13, 2020

Conversation

dzearing
Copy link
Member

@dzearing dzearing commented Jan 13, 2020 •
edited by msft-github-bot

In nested FocusZones, a global keydown listener is added but not removed. This should address this and only add it on the first outer zone and remove it on the last outer zone.

Microsoft Reviewers: Open in CodeFlow

@JasonGore
Copy link
Member

JasonGore commented Jan 13, 2020 •
edited

The related PR also had a fix to componentWillUnmount, is that not needed here?

@JasonGore JasonGore changed the title FocusZone: removing keydown listerner correctly. FocusZone: removing keydown listener correctly. Jan 13, 2020
@msft-github-bot
Copy link
Contributor

msft-github-bot commented Jan 13, 2020 •
edited

Component Perf Analysis

No significant results to display.

All results

Scenario Master Ticks PR Ticks Status
BaseButton 742 728
BaseButton (experiments) 991 1033
DefaultButton 1024 973
DefaultButton (experiments) 1914 1945
DetailsRow 3381 3379
DetailsRow (fast icons) 3458 3407
DetailsRow without styles 3211 3186
DocumentCardTitle with truncation 1559 1519
MenuButton 1401 1352
MenuButton (experiments) 3560 3629
PrimaryButton 1191 1180
PrimaryButton (experiments) 2010 2003
SplitButton 2885 2860
SplitButton (experiments) 7179 7262
Stack 494 484
Stack with Intrinsic children 1121 1137
Stack with Text children 4323 4260
Text 395 383
Toggle 849 875
Toggle (experiments) 2329 2295
button 65 65

@size-auditor
Copy link

size-auditor bot commented Jan 13, 2020 •
edited

Asset size changes

Project Bundle Baseline Size New Size Difference
office-ui-fabric-react CommandBar 187.224 kB 187.228 kB ExceedsBaseline     4 bytes
office-ui-fabric-react ExtendedPicker 73.935 kB 73.939 kB ExceedsBaseline     4 bytes
office-ui-fabric-react OverflowSet 55.457 kB 55.461 kB ExceedsBaseline     4 bytes
office-ui-fabric-react Dialog 193.887 kB 193.891 kB ExceedsBaseline     4 bytes
office-ui-fabric-react DetailsList 209.638 kB 209.642 kB ExceedsBaseline     4 bytes
office-ui-fabric-react Pickers 268.113 kB 268.117 kB ExceedsBaseline     4 bytes
office-ui-fabric-react Pivot 172.773 kB 172.777 kB ExceedsBaseline     4 bytes
office-ui-fabric-react DatePicker 202.163 kB 202.167 kB ExceedsBaseline     4 bytes
office-ui-fabric-react ContextualMenu 147.268 kB 147.272 kB ExceedsBaseline     4 bytes
office-ui-fabric-react Nav 174.976 kB 174.98 kB ExceedsBaseline     4 bytes
office-ui-fabric-react Rating 71.455 kB 71.459 kB ExceedsBaseline     4 bytes
office-ui-fabric-react ComboBox 229.003 kB 229.007 kB ExceedsBaseline     4 bytes
office-ui-fabric-react Dropdown 216.649 kB 216.653 kB ExceedsBaseline     4 bytes
office-ui-fabric-react SearchBox 173.943 kB 173.947 kB ExceedsBaseline     4 bytes
office-ui-fabric-react MessageBar 175.618 kB 175.622 kB ExceedsBaseline     4 bytes
office-ui-fabric-react SelectedItemsList 215.175 kB 215.179 kB ExceedsBaseline     4 bytes
office-ui-fabric-react Panel 185.217 kB 185.221 kB ExceedsBaseline     4 bytes
office-ui-fabric-react DocumentCard 201.023 kB 201.027 kB ExceedsBaseline     4 bytes
office-ui-fabric-react Facepile 196.458 kB 196.462 kB ExceedsBaseline     4 bytes
office-ui-fabric-react ShimmeredDetailsList 220.166 kB 220.17 kB ExceedsBaseline     4 bytes
office-ui-fabric-react FloatingPicker 225.034 kB 225.038 kB ExceedsBaseline     4 bytes
office-ui-fabric-react SpinButton 179.154 kB 179.158 kB ExceedsBaseline     4 bytes
office-ui-fabric-react FocusZone 33.319 kB 33.323 kB ExceedsBaseline     4 bytes
office-ui-fabric-react Calendar 143.873 kB 143.877 kB ExceedsBaseline     4 bytes
office-ui-fabric-react Button 179.981 kB 179.985 kB ExceedsBaseline     4 bytes
office-ui-fabric-react SwatchColorPicker 177.645 kB 177.649 kB ExceedsBaseline     4 bytes
office-ui-fabric-react TeachingBubble 190.861 kB 190.865 kB ExceedsBaseline     4 bytes
office-ui-fabric-react Breadcrumb 184.96 kB 184.964 kB ExceedsBaseline     4 bytes
office-ui-fabric-react Grid 167.501 kB 167.505 kB ExceedsBaseline     4 bytes
office-ui-fabric-react GroupedList 117.848 kB 117.852 kB ExceedsBaseline     4 bytes

ExceedsTolerance Over Tolerance (1024 B) ExceedsBaseline Over Baseline BelowBaseline Below Baseline New New Deleted  Removed 1 kB = 1000 B

Baseline commit: f82bc1ef3402bda4ff6d49a2d44579bd27f36a49 (build)

@KevinTCoughlin
Copy link
Member

KevinTCoughlin commented Jan 13, 2020 •
edited

The related PR also had a fix to componentWillUnmount, is that not needed here?

As far as I can tell that change was only white-space related.

KevinTCoughlin
KevinTCoughlin approved these changes Jan 13, 2020
View reviewed changes
Copy link
Member

@KevinTCoughlin KevinTCoughlin left a comment

Choose a reason for hiding this comment

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

Thanks for cherry-picking this fix @dzearing! This will fix a % of leaks we see when profiling the list across OneDrive / SharePoint cc: @ThomasMichon @patmill

@KevinTCoughlin
Copy link
Member

The related PR also had a fix to componentWillUnmount, is that not needed here?

As far as I can tell that change was only white-space related.

Nevermind, @JasonGore you're right it is nested inside the if in both places in the original fix!

@dzearing
Copy link
Member Author

@JasonGore For symmetry, I moved the same dispose logic into the !innerzone block in unmount.

@dzearing dzearing merged commit 4feba45 into microsoft:master Jan 13, 2020
@msft-github-bot
Copy link
Contributor

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

Handy links:

@msft-github-bot
Copy link
Contributor

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

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KevinTCoughlin KevinTCoughlin KevinTCoughlin approved these changes

@JasonGore JasonGore JasonGore approved these changes

Assignees

@xugao xugao

Labels
Component: FocusZone
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@dzearing @JasonGore @msft-github-bot @KevinTCoughlin @xugao