fix: do not rely on `UIEvent.view` by ph-fritsche · Pull Request #20022 · microsoft/fluentui

Philipp Fritsche (ph-fritsche) · 2021-09-29T06:54:45Z

Pull request checklist

Addresses an existing issue: Fixes [react-northstar] TypeError: Cannot read property 'document' of null #19233
[N/A] Include a change request file using $ yarn change
yarn change says:

No change files are needed

Description of changes

Replace UIEvent.view as it does not exist at least on some events in major browsers.
See testing-library/user-event#709 (comment)

The event handlers in the utility are added on Window.
Therefore UIEvent.view is replaced with Event.currentTarget.

Focus areas to test

(optional)

DustyTheBot · 2021-09-29T07:02:19Z

	Warnings
⚠️	There are no updates provided to CHANGELOG. Ensure there are no publicly visible changes introduced by this PR.

Generated by 🚫 dangerJS against 559e69c

size-auditor · 2021-09-29T07:08:34Z

Asset size changes

Project	Bundle	Baseline Size	New Size	Difference
office-ui-fabric-react	fluentui-react-northstar-Slider	96.012 kB	96.021 kB	9 bytes
office-ui-fabric-react	fluentui-react-northstar-Menu	140.959 kB	140.968 kB	9 bytes
office-ui-fabric-react	fluentui-react-northstar-Dropdown	210.504 kB	210.513 kB	9 bytes
office-ui-fabric-react	fluentui-react-northstar-Tooltip	119.808 kB	119.817 kB	9 bytes
office-ui-fabric-react	fluentui-react-northstar-Toolbar	187.539 kB	187.548 kB	9 bytes
office-ui-fabric-react	fluentui-react-northstar-Datepicker	200.075 kB	200.084 kB	9 bytes
office-ui-fabric-react	fluentui-react-northstar-Chat	164.612 kB	164.621 kB	9 bytes
office-ui-fabric-react	fluentui-react-northstar-Carousel	115.515 kB	115.524 kB	9 bytes
office-ui-fabric-react	fluentui-react-northstar-MenuButton	175.064 kB	175.073 kB	9 bytes
office-ui-fabric-react	fluentui-react-northstar-Popup	145.105 kB	145.114 kB	9 bytes
office-ui-fabric-react	fluentui-react-northstar-Provider	93.151 kB	93.16 kB	9 bytes
office-ui-fabric-react	fluentui-react-northstar-SplitButton	191.191 kB	191.2 kB	9 bytes

Over Tolerance (1024 B) Over Baseline Below Baseline New Removed 1 kB = 1000 B

Baseline commit: e726d8b364e68d28821710ea168b3ee0f7a23345 (build)

codesandbox-ci · 2021-09-29T07:09:25Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 559e69c:

Sandbox	Source
@fluentui/react 8 starter	Configuration
@fluentui/react-components 9 starter	Configuration

Fluent UI Team (fabricteam) · 2021-09-29T07:10:56Z

📊 Bundle size report

_{🤖 This report was generated against e726d8b364e68d28821710ea168b3ee0f7a23345}

Fluent UI Team (fabricteam) · 2021-09-29T07:34:09Z

Perf Analysis (`@fluentui/react`)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
Avatar	mount	933	937	5000
BaseButton	mount	917	952	5000
Breadcrumb	mount	2629	2674	1000
ButtonNext	mount	452	460	5000
Checkbox	mount	1513	1511	5000
CheckboxBase	mount	1322	1333	5000
ChoiceGroup	mount	4803	4899	5000
ComboBox	mount	982	981	1000
CommandBar	mount	10373	10393	1000
ContextualMenu	mount	6585	6395	1000
DefaultButton	mount	1147	1223	5000
DetailsRow	mount	3770	3881	5000
DetailsRowFast	mount	3913	3855	5000
DetailsRowNoStyles	mount	3540	3584	5000
Dialog	mount	2505	2515	1000
DocumentCardTitle	mount	165	145	1000
Dropdown	mount	3234	3407	5000
FluentProviderNext	mount	7644	7688	5000
FluentProviderWithTheme	mount	363	372	10
FluentProviderWithTheme	virtual-rerender	100	100	10
FluentProviderWithTheme	virtual-rerender-with-unmount	504	527	10
FocusTrapZone	mount	1771	1877	5000
FocusZone	mount	1852	1807	5000
IconButton	mount	1979	1855	5000
Label	mount	341	349	5000
Layer	mount	3011	2999	5000
Link	mount	495	469	5000
MakeStyles	mount	1888	1890	50000
MenuButton	mount	1491	1471	5000
MessageBar	mount	2005	1998	5000
Nav	mount	3388	3342	1000
OverflowSet	mount	1140	1126	5000
Panel	mount	2367	2356	1000
Persona	mount	879	832	1000
Pivot	mount	1477	1432	1000
PrimaryButton	mount	1287	1305	5000
Rating	mount	8038	7885	5000
SearchBox	mount	1361	1335	5000
Shimmer	mount	2534	2675	5000
Slider	mount	2055	1935	5000
SpinButton	mount	5171	5089	5000
Spinner	mount	423	438	5000
SplitButton	mount	3189	3202	5000
Stack	mount	553	502	5000
StackWithIntrinsicChildren	mount	1693	1759	5000
StackWithTextChildren	mount	4858	4769	5000
SwatchColorPicker	mount	10510	10775	5000
Tabs	mount	1420	1476	1000
TagPicker	mount	2798	2682	5000
TeachingBubble	mount	13098	13402	5000
Text	mount	406	448	5000
TextField	mount	1420	1369	5000
ThemeProvider	mount	1201	1203	5000
ThemeProvider	virtual-rerender	628	622	5000
ThemeProvider	virtual-rerender-with-unmount	1865	1930	5000
Toggle	mount	797	800	5000
buttonNative	mount	116	126	5000

Perf Analysis (`@fluentui/react-northstar`)

Perf tests with no regressions

Scenario	Current PR Ticks	Baseline Ticks	Ratio
AttachmentMinimalPerf.default	172	153	1.12:1
ListCommonPerf.default	704	627	1.12:1
TableMinimalPerf.default	448	401	1.12:1
RadioGroupMinimalPerf.default	489	454	1.08:1
IconMinimalPerf.default	659	610	1.08:1
ButtonMinimalPerf.default	177	166	1.07:1
ChatDuplicateMessagesPerf.default	319	298	1.07:1
TextMinimalPerf.default	362	337	1.07:1
AlertMinimalPerf.default	291	274	1.06:1
AvatarMinimalPerf.default	202	190	1.06:1
BoxMinimalPerf.default	362	343	1.06:1
ListMinimalPerf.default	555	526	1.06:1
ToolbarMinimalPerf.default	1011	957	1.06:1
TreeWith60ListItems.default	201	189	1.06:1
ButtonSlotsPerf.default	574	549	1.05:1
CardMinimalPerf.default	557	537	1.04:1
DividerMinimalPerf.default	368	353	1.04:1
GridMinimalPerf.default	351	339	1.04:1
LabelMinimalPerf.default	391	376	1.04:1
PortalMinimalPerf.default	183	176	1.04:1
RefMinimalPerf.default	243	233	1.04:1
SliderMinimalPerf.default	1813	1751	1.04:1
AccordionMinimalPerf.default	162	158	1.03:1
ChatMinimalPerf.default	654	638	1.03:1
DialogMinimalPerf.default	775	754	1.03:1
LayoutMinimalPerf.default	374	362	1.03:1
ListWith60ListItems.default	653	632	1.03:1
ReactionMinimalPerf.default	398	385	1.03:1
SplitButtonMinimalPerf.default	4404	4279	1.03:1
CheckboxMinimalPerf.default	2848	2781	1.02:1
CarouselMinimalPerf.default	478	475	1.01:1
DropdownMinimalPerf.default	3284	3256	1.01:1
LoaderMinimalPerf.default	711	703	1.01:1
PopupMinimalPerf.default	599	591	1.01:1
DatepickerMinimalPerf.default	5617	5603	1:1
EmbedMinimalPerf.default	4345	4335	1:1
ItemLayoutMinimalPerf.default	1223	1228	1:1
RosterPerf.default	1187	1191	1:1
SegmentMinimalPerf.default	341	341	1:1
InputMinimalPerf.default	1358	1370	0.99:1
ProviderMergeThemesPerf.default	1754	1778	0.99:1
StatusMinimalPerf.default	683	688	0.99:1
TableManyItemsPerf.default	1928	1939	0.99:1
CustomToolbarPrototype.default	4149	4199	0.99:1
TooltipMinimalPerf.default	1038	1053	0.99:1
VideoMinimalPerf.default	635	642	0.99:1
DropdownManyItemsPerf.default	677	692	0.98:1
MenuMinimalPerf.default	836	854	0.98:1
MenuButtonMinimalPerf.default	1673	1706	0.98:1
AttachmentSlotsPerf.default	1138	1168	0.97:1
ButtonOverridesMissPerf.default	1792	1839	0.97:1
ChatWithPopoverPerf.default	381	391	0.97:1
HeaderSlotsPerf.default	753	779	0.97:1
ImageMinimalPerf.default	367	378	0.97:1
TreeMinimalPerf.default	783	808	0.97:1
AnimationMinimalPerf.default	404	422	0.96:1
SkeletonMinimalPerf.default	360	376	0.96:1
TextAreaMinimalPerf.default	476	496	0.96:1
ListNestedPerf.default	564	595	0.95:1
ProviderMinimalPerf.default	1142	1206	0.95:1
HeaderMinimalPerf.default	373	399	0.93:1
FlexMinimalPerf.default	273	300	0.91:1
FormMinimalPerf.default	387	442	0.88:1

ling1726 · 2021-09-29T09:13:12Z

  eventTarget.addEventListener('keyup', eventBuffer, true);
 };

+type UIEventOnElement<T> = UIEvent & { currentTarget: T }


#20024 Adds correct typings to the event listeners, so this manual typing is no longer required

ling1726

The usage of event.view should be correct since the events used by whatInput should all be of type UIEvent, is there an event used here that does not support event.view

Please take a look at this testing-library/user-event#709 (comment)

Philipp Fritsche (ph-fritsche) · 2021-09-29T09:33:18Z

There is InputEvent in Chrome which does not implement the view property.
JSDOM does not implement it on any UIEvent.
See testing-library/user-event#709 (comment)

Feel free to close this if a workaround for JSDOM is out of scope.

ling1726 · 2021-09-29T09:39:06Z

There is InputEvent in Chrome which does not implement the view property. JSDOM does not implement it on any UIEvent. See testing-library/user-event#709 (comment)

Feel free to close this if a workaround for JSDOM is out of scope.

Is Fluent completely incompatible with JSDOM ? can a workaround for testing-library be to manually set the view property when firing events ?

Philipp Fritsche (ph-fritsche) · 2021-09-29T10:10:50Z

Yes, we could add view to all UIEvents, but as we try to be environment agnostic adding code to mimick environment features should be out of scope (unless needed for the library itself to work).

Also there are workarounds for most cases in consuming projects:

On Window use event.currentTarget.
On Document use event.currentTarget.defaultView
On HTMLElement use event.currentTarget.ownerDocument.defaultView

With the the current implementation of whatInput Fluentui would cause errors on any of those UIEvent in Jsdom.
As the solution with currentTarget is equally conformant to the specs, I think it causes the least harm.

ling1726 · 2021-09-29T13:29:45Z

With the the current implementation of whatInput Fluentui would cause errors on any of those UIEvent in Jsdom. As the solution with currentTarget is equally conformant to the specs, I think it causes the least harm.

This seems like a problem for JS DOM, in this case is it possible to add a guard before the usage of doUpdate ? This way it minimizes any potential impact for applications

Philipp Fritsche (ph-fritsche) · 2021-09-29T15:52:33Z

I don't think adding code to guard against this bug is worth it. The replacement with currentTarget works the same in this instance without adding more statements.
If you don't feel comfortable changing the implementation, just keep it and people could tweak the event themselves in testing environment until this is fixed in jsdom.

ling1726 · 2021-09-30T06:27:06Z

Closing since this is an issue with JS DOM, since the events Fluent uses should support event.view

fix: do not rely on UIEvent.view

559e69c

Philipp Fritsche (ph-fritsche) requested review from Juraj Kapsiar (jurokapsiar), Oleksandr Fediashov (layershifter), ling1726, Miroslav Stastny (miroslavstastny) and Roman Sudarikov (pompomon) as code owners September 29, 2021 06:54

Fluent UI Bot (msft-fluent-ui-bot) added the Fluent UI react-northstar (v0) Work related to Fluent UI V0 label Sep 29, 2021

Fluent UI Bot (msft-fluent-ui-bot) assigned Geoff Cox (Microsoft) (GeoffCoxMSFT) Sep 29, 2021

ling1726 reviewed Sep 29, 2021

View reviewed changes

ling1726 closed this Sep 30, 2021

ling1726 mentioned this pull request Nov 4, 2021

fix(whatInput): Stop event.view usage in tests #20492

Merged

2 tasks done

Rob Snow (snowystinger) mentioned this pull request Aug 12, 2026

call initUIEvent with the correct view testing-library/user-event#1306

Open

1 of 2 tasks

Navigation Menu

Repository navigation

fix: do not rely on `UIEvent.view` - #20022

fix: do not rely on `UIEvent.view`#20022
Philipp Fritsche (ph-fritsche) wants to merge 1 commit into
microsoft:masterfrom
ph-fritsche:fix-uievent-view

Philipp Fritsche (ph-fritsche) commented Sep 29, 2021

Uh oh!

DustyTheBot commented Sep 29, 2021

Uh oh!

size-auditor Bot commented Sep 29, 2021

Uh oh!

codesandbox-ci Bot commented Sep 29, 2021

Uh oh!

Fluent UI Team (fabricteam) commented Sep 29, 2021

Uh oh!

Fluent UI Team (fabricteam) commented Sep 29, 2021

Uh oh!

ling1726 Sep 29, 2021

Uh oh!

ling1726 left a comment

Uh oh!

Philipp Fritsche (ph-fritsche) commented Sep 29, 2021

Uh oh!

ling1726 commented Sep 29, 2021

Uh oh!

Philipp Fritsche (ph-fritsche) commented Sep 29, 2021 •

edited

Loading

Uh oh!

ling1726 commented Sep 29, 2021

Uh oh!

Philipp Fritsche (ph-fritsche) commented Sep 29, 2021

Uh oh!

ling1726 commented Sep 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

Philipp Fritsche (ph-fritsche) commented Sep 29, 2021

Pull request checklist

Description of changes

Focus areas to test

Uh oh!

DustyTheBot commented Sep 29, 2021

Uh oh!

size-auditor Bot commented Sep 29, 2021

Asset size changes

Uh oh!

codesandbox-ci Bot commented Sep 29, 2021

Uh oh!

Fluent UI Team (fabricteam) commented Sep 29, 2021

📊 Bundle size report

Uh oh!

Fluent UI Team (fabricteam) commented Sep 29, 2021

Perf Analysis (@fluentui/react)

Perf Analysis (@fluentui/react-northstar)

Uh oh!

ling1726 Sep 29, 2021

Choose a reason for hiding this comment

Uh oh!

ling1726 left a comment

Choose a reason for hiding this comment

Uh oh!

Philipp Fritsche (ph-fritsche) commented Sep 29, 2021

Uh oh!

ling1726 commented Sep 29, 2021

Uh oh!

Philipp Fritsche (ph-fritsche) commented Sep 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ling1726 commented Sep 29, 2021

Uh oh!

Philipp Fritsche (ph-fritsche) commented Sep 29, 2021

Uh oh!

ling1726 commented Sep 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Perf Analysis (`@fluentui/react`)

Perf Analysis (`@fluentui/react-northstar`)

Philipp Fritsche (ph-fritsche) commented Sep 29, 2021 •

edited

Loading