RFC(keyboard-key): Handle keyboard keys in more modern and tree shakeable way #18587

ling1726 · 2021-06-16T13:46:25Z

Please view in preview mode

Pull request checklist

Addresses an existing issue: Fixes #0000
Include a change request file using $ yarn change

Description of changes

(give an overview)

Focus areas to test

(optional)

__Please view in preview mode__

size-auditor · 2021-06-16T14:05:47Z

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: bf6fd1290ea6c6861a5d80f704e0d6d0cadb71e0 (build)

fabricteam · 2021-06-16T14:27:59Z

Perf Analysis (`@fluentui/react`)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
Avatar	mount	792	820	5000
BaseButton	mount	919	893	5000
Breadcrumb	mount	2634	2609	1000
ButtonNext	mount	527	538	5000
Checkbox	mount	1503	1508	5000
CheckboxBase	mount	1305	1276	5000
ChoiceGroup	mount	4751	4774	5000
ComboBox	mount	969	984	1000
CommandBar	mount	10249	10252	1000
ContextualMenu	mount	6370	6340	1000
DefaultButton	mount	1128	1153	5000
DetailsRow	mount	3702	3658	5000
DetailsRowFast	mount	3729	3801	5000
DetailsRowNoStyles	mount	3631	3509	5000
Dialog	mount	2194	2184	1000
DocumentCardTitle	mount	137	152	1000
Dropdown	mount	3244	3206	5000
FocusTrapZone	mount	1764	1763	5000
FocusZone	mount	1781	1807	5000
IconButton	mount	1887	1707	5000
Label	mount	334	338	5000
Layer	mount	1784	1818	5000
Link	mount	463	443	5000
MakeStyles	mount	1798	1811	50000
MenuButton	mount	1452	1486	5000
MessageBar	mount	2005	2046	5000
Nav	mount	3272	3236	1000
OverflowSet	mount	1028	1023	5000
Panel	mount	2074	2066	1000
Persona	mount	810	817	1000
Pivot	mount	1417	1398	1000
PrimaryButton	mount	1283	1256	5000
Rating	mount	7595	7532	5000
SearchBox	mount	1306	1279	5000
Shimmer	mount	2518	2574	5000
Slider	mount	1969	1962	5000
SpinButton	mount	4941	4900	5000
Spinner	mount	428	410	5000
SplitButton	mount	3216	3150	5000
Stack	mount	489	486	5000
StackWithIntrinsicChildren	mount	1489	1527	5000
StackWithTextChildren	mount	4442	4467	5000
SwatchColorPicker	mount	10165	10123	5000
Tabs	mount	1414	1408	1000
TagPicker	mount	2400	2448	5000
TeachingBubble	mount	11852	11888	5000
Text	mount	423	422	5000
TextField	mount	1414	1364	5000
ThemeProvider	mount	1183	1196	5000
ThemeProvider	virtual-rerender	621	610	5000
ThemeProviderNext	mount	7165	7101	5000
Toggle	mount	822	809	5000
buttonNative	mount	111	122	5000

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

⚠️ 1 potential perf regressions detected

Potential regressions comparing to master

Scenario	Current PR Ticks	Baseline Ticks	Ratio	Regression Analysis
AlertMinimalPerf.default	267	272	0.98:1	analysis

Perf tests with no regressions

Scenario	Current PR Ticks	Baseline Ticks	Ratio
AttachmentMinimalPerf.default	159	146	1.09:1
AvatarMinimalPerf.default	210	194	1.08:1
ChatDuplicateMessagesPerf.default	299	280	1.07:1
ReactionMinimalPerf.default	389	362	1.07:1
AccordionMinimalPerf.default	156	147	1.06:1
ListMinimalPerf.default	519	490	1.06:1
ListNestedPerf.default	553	521	1.06:1
AnimationMinimalPerf.default	415	397	1.05:1
GridMinimalPerf.default	347	329	1.05:1
HeaderSlotsPerf.default	791	750	1.05:1
ImageMinimalPerf.default	384	365	1.05:1
SegmentMinimalPerf.default	350	333	1.05:1
TreeWith60ListItems.default	175	166	1.05:1
ButtonSlotsPerf.default	566	544	1.04:1
DividerMinimalPerf.default	372	358	1.04:1
RefMinimalPerf.default	238	229	1.04:1
CardMinimalPerf.default	565	550	1.03:1
RadioGroupMinimalPerf.default	452	437	1.03:1
TextMinimalPerf.default	350	341	1.03:1
TextAreaMinimalPerf.default	500	486	1.03:1
TooltipMinimalPerf.default	1012	986	1.03:1
AttachmentSlotsPerf.default	1089	1070	1.02:1
ButtonMinimalPerf.default	164	160	1.02:1
CarouselMinimalPerf.default	475	464	1.02:1
DropdownMinimalPerf.default	3113	3043	1.02:1
InputMinimalPerf.default	1275	1248	1.02:1
LabelMinimalPerf.default	395	388	1.02:1
LoaderMinimalPerf.default	690	677	1.02:1
BoxMinimalPerf.default	342	338	1.01:1
ButtonOverridesMissPerf.default	1672	1649	1.01:1
DatepickerMinimalPerf.default	5489	5435	1.01:1
EmbedMinimalPerf.default	4123	4075	1.01:1
FlexMinimalPerf.default	292	288	1.01:1
LayoutMinimalPerf.default	366	362	1.01:1
PopupMinimalPerf.default	578	571	1.01:1
PortalMinimalPerf.default	181	180	1.01:1
ProviderMinimalPerf.default	956	948	1.01:1
SplitButtonMinimalPerf.default	3740	3685	1.01:1
CustomToolbarPrototype.default	3809	3773	1.01:1
ToolbarMinimalPerf.default	956	943	1.01:1
VideoMinimalPerf.default	639	631	1.01:1
DialogMinimalPerf.default	757	755	1:1
ItemLayoutMinimalPerf.default	1248	1245	1:1
MenuMinimalPerf.default	845	848	1:1
ProviderMergeThemesPerf.default	1653	1653	1:1
SkeletonMinimalPerf.default	344	343	1:1
StatusMinimalPerf.default	673	674	1:1
TableManyItemsPerf.default	1899	1906	1:1
TreeMinimalPerf.default	790	788	1:1
ChatMinimalPerf.default	640	646	0.99:1
CheckboxMinimalPerf.default	2725	2740	0.99:1
DropdownManyItemsPerf.default	689	697	0.99:1
FormMinimalPerf.default	411	415	0.99:1
MenuButtonMinimalPerf.default	1571	1588	0.99:1
SliderMinimalPerf.default	1556	1574	0.99:1
TableMinimalPerf.default	396	398	0.99:1
ListWith60ListItems.default	637	648	0.98:1
RosterPerf.default	1130	1156	0.98:1
ListCommonPerf.default	619	640	0.97:1
HeaderMinimalPerf.default	347	361	0.96:1
IconMinimalPerf.default	607	631	0.96:1
ChatWithPopoverPerf.default	347	366	0.95:1

khmakoto

As the one who ported keyboard-key into the repo and fought to minimize bundle size impact (it was originally 3.5K instead of 2K) I very much like the idea of moving on and using key instead of keyCode now that we don't have to deal with IE11.

Option 3 would be my go-to but curious of what other people think should be the way to go.

rfcs/convergence/deprecate-keyboard-key-package.md

ecraig12345 · 2021-06-16T19:43:43Z

rfcs/convergence/deprecate-keyboard-key-package.md

+
+#### Option 1: Keep using keyboard-key
+
+We can copy the contents of keyboard-key internally into the v8 `react` folder. We have already done this previously with v7. This lets us modify keyboard-key freely without breaking v8.


v8 wasn't using keyboard-key at all until a previous convergence attempt, so I think we really ought to just get rid of it from v8 (and go back to whatever the previous implementation was)

However previous versions of v8/7 will use this and some partners (Teams) have a hard resolution for a specific version of keyboard-key this makes any changes we make there hard for customers to consume

however if you mean remove the package from the repo completely ... sounds good to me

What I mean is:

Remove the keyboard-key dep from v8 and revert to old implementation. I can't see this causing any problems for partners, even if they have specific resolutions for keyboard-key.

TBD for v0, whatever seems appropriate to you

Possibly remove the package from the repo, if nothing in the repo is using the current version and we don't expect to need to release fixes for it

ecraig12345 · 2021-06-16T19:45:33Z

rfcs/convergence/deprecate-keyboard-key-package.md

+
+### Use e.key
+
+Since we no longer target support for IE11, the usage of `e.key` is simplified and no longer needs to have special handling for IE11. We can modify an existing package or create a new one that simply stores all of the keys as constants which are easily tree shaken.


Do we actually have this documented anywhere (such as in another RFC)? It keeps coming up but AFAIK there hasn't been officially documented/agreed upon. (To be clear I'd like to drop it!! Just want to make sure we've thought through the partner requirements and other implications and are all on the same page.)

I have heard this from @JustSlone, can you please confirm and perhaps I'll spin up a quick RFC so we have this down in writing

separate RFC for how to handle keys in general for modern web would be great 🙌.

I mean this table speaks for itself so that might be our RFC? :D https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key#browser_compatibility

I've added @Hotell 's link to the RFC. I wouldn't go for a separate RFC but rather make the new package compatible with both key and keyCode. I'll update the RFC to reflect this

wouldn't adding keyCode "support" moves us back where we started regarding bundle size etc ?

not really, it's rather the current keyboard-key package stores all those keycodes in layers of dictionaries used by utilities there such as getCode that make the contents un-treeshakeable

gotcha thx!

Co-authored-by: Makoto Morimoto <Humberto.Morimoto@microsoft.com>

ling1726 · 2021-06-17T07:09:54Z

As the one who ported keyboard-key into the repo and fought to minimize bundle size impact (it was originally 3.5K instead of 2K) I very much like the idea of moving on and using key instead of keyCode now that we don't have to deal with IE11.

Option 3 would be my go-to but curious of what other people think should be the way to go.

Creating a new package definitely sounds good, it will result in clearer imports and will have no major dependency issues for customers, since either they need the extra dependency or not. However I am not an expert in that area, @ecraig12345 any thoughts if are there any dependency related problems partners might have by creating a new package ?

ecraig12345 · 2021-06-17T17:44:02Z

As the one who ported keyboard-key into the repo and fought to minimize bundle size impact (it was originally 3.5K instead of 2K) I very much like the idea of moving on and using key instead of keyCode now that we don't have to deal with IE11.
Option 3 would be my go-to but curious of what other people think should be the way to go.

Creating a new package definitely sounds good, it will result in clearer imports and will have no major dependency issues for customers, since either they need the extra dependency or not. However I am not an expert in that area, @ecraig12345 any thoughts if are there any dependency related problems partners might have by creating a new package ?

Generally it's okay to create new packages and add them as deps. It can be a minor headache for certain partners, but this is relatively uncommon (and those customers have likely not updated to v8 or converged yet anyway). However per my other comment about removing the keyboard-key dep from v8, I'm not sure this will matter.

Hotell · 2021-06-21T10:31:20Z

rfcs/convergence/deprecate-keyboard-key-package.md

+
+### Cons
+
+- Managing the dependency issues in HVCs and products that have mismatching versions of fluent packages that depend on keyboard-key


Vnext is in alpha, while this may become temporarily unpleasant experience for our team (need to apologise?... ), I don't think this should be considered as con.

Hotell · 2021-06-21T10:41:41Z

Can this RFC be updated with proposal that we gonna go forward?

Overall I like proposal no 3 - @fluentui/react-keyboard-key 👍

while that react is super confusing as this package has nothing to do with react, it will follow our vNext convention

…726/fluentui into rfc/converged-keyboard-key

ling1726 · 2021-06-23T12:14:00Z

Can this RFC be updated with proposal that we gonna go forward?

Overall I like proposal no 3 - @fluentui/react-keyboard-key 👍

while that react is super confusing as this package has nothing to do with react, it will follow our vNext convention

Moved other solutions to disarded section in the RFC

layershifter · 2021-06-25T10:00:56Z

rfcs/convergence/deprecate-keyboard-key-package.md

+In order to comfortably use both `key` and `keyCode` during a transition phase and also in the future if we ever find out one might be preferrable to another, we should implement the new package to be compatible with both. This can be done with a `key` and `keycode` naming convention.
+
+```typescript
+import { ArrowDownKey, ArrowDownKeyCode } from '@fluentui/react-keyboard-key';


I agree that we should create package. Have keyboard-keys been considered as a name? 😅

the name IMHO is not a huge deal, but keyboard-keys is possible.

layershifter · 2021-06-25T10:03:57Z

rfcs/convergence/deprecate-keyboard-key-package.md

+```typescript
+import { ArrowDownKey, ArrowDownKeyCode } from '@fluentui/react-keyboard-key';
+```
+


Current package (keyboard-key) contains getKey() and getCode() helpers for compatibility reasons. Do I correctly get that a new package will contain only constants with keys and their codes? If so, can you please add this statement in the proposal?

yes there will be no more helpers, since the helpers helped to contribute to the lack of tree shaking for the current package.

There is really not that much work to do to call e.key or e.keyCode now that we support modern browsers

Initializes the auto-generated artifacts for the new `@fluentui/keyboard-keys` package as proposed in microsoft#18587 **Does not publish the package immediately**

Initializes the auto-generated artifacts for the new `@fluentui/keyboard-keys` package as proposed in #18587 **Does not publish the package immediately**

This PR implements what was proposed in microsoft#18587 and exports all named `key` values specified by w3c. Also adds legacy keycode exports as a deep import. Happy to take any suggestions to avoid this. Originallly this was the plan: ```ts export * as keyCodes from './keyCodes' ``` but unfortunately API extractor does not support this syntax microsoft/rushstack#2780

* feat(keyboard-keys): Export keys and legacy keyCodes This PR implements what was proposed in #18587 and exports all named `key` values specified by w3c. Also adds legacy keycode exports as a deep import. Happy to take any suggestions to avoid this. Originallly this was the plan: ```ts export * as keyCodes from './keyCodes' ``` but unfortunately API extractor does not support this syntax microsoft/rushstack#2780 * remove deep export * update fixture

Initializes the auto-generated artifacts for the new `@fluentui/keyboard-keys` package as proposed in microsoft#18587 **Does not publish the package immediately**

* feat(keyboard-keys): Export keys and legacy keyCodes This PR implements what was proposed in microsoft#18587 and exports all named `key` values specified by w3c. Also adds legacy keycode exports as a deep import. Happy to take any suggestions to avoid this. Originallly this was the plan: ```ts export * as keyCodes from './keyCodes' ``` but unfortunately API extractor does not support this syntax microsoft/rushstack#2780 * remove deep export * update fixture

RFC(keyboard-key): Replace with something more modern and shakeable

85bf6e4

__Please view in preview mode__

msft-fluent-ui-bot assigned miroslavstastny Jun 16, 2021

ling1726 added the Type: RFC Request for Feedback label Jun 16, 2021

update

544c658

ling1726 requested review from a team June 16, 2021 13:49

ling1726 added 4 commits June 16, 2021 15:50

update

a9af20f

update

6379ba6

update

a7fc844

update

8974553

ling1726 changed the title ~~RFC(keyboard-key): Replace with something more modern and shakeable~~ RFC(keyboard-key): Handle keyboard keys in more modern and tree shakeable way Jun 16, 2021

khmakoto reviewed Jun 16, 2021

View reviewed changes

rfcs/convergence/deprecate-keyboard-key-package.md Outdated Show resolved Hide resolved

khmakoto approved these changes Jun 16, 2021

View reviewed changes

ecraig12345 approved these changes Jun 16, 2021

View reviewed changes

Update rfcs/convergence/deprecate-keyboard-key-package.md

0b746c8

Co-authored-by: Makoto Morimoto <Humberto.Morimoto@microsoft.com>

bsunderhus approved these changes Jun 21, 2021

View reviewed changes

Hotell reviewed Jun 21, 2021

View reviewed changes

ling1726 added 2 commits June 23, 2021 14:04

update RFC with decision

979fc60

Merge branch 'rfc/converged-keyboard-key' of https://github.com/ling1…

d762f29

…726/fluentui into rfc/converged-keyboard-key

ling1726 added 2 commits June 23, 2021 14:16

add mdn support page

51f5568

update compat

bfd22c6

layershifter reviewed Jun 25, 2021

View reviewed changes

ling1726 added 2 commits June 25, 2021 13:51

no longer support helpers

c989111

rename file

d4c9a52

ling1726 enabled auto-merge (squash) June 25, 2021 11:56

ling1726 merged commit 8e42137 into microsoft:master Jun 25, 2021

bsunderhus mentioned this pull request Jun 29, 2021

react-button 1st rule of ARIA refactor #18582

Closed

1 task

ling1726 mentioned this pull request Jul 2, 2021

feat(keyboard-keys): Init new package (not published) #18815

Merged

2 tasks

ling1726 mentioned this pull request Jul 11, 2021

feat(keyboard-keys): Export keys and legacy keyCodes #18896

Merged

2 tasks

This was referenced Jul 20, 2021

chore(a11y-testing): use new keyboard-keys package #19012

Merged

v9: replace keyboard-key with keyboard-keys #19013

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC(keyboard-key): Handle keyboard keys in more modern and tree shakeable way #18587

RFC(keyboard-key): Handle keyboard keys in more modern and tree shakeable way #18587

ling1726 commented Jun 16, 2021 •

edited

Loading

size-auditor bot commented Jun 16, 2021 •

edited

Loading

fabricteam commented Jun 16, 2021 •

edited

Loading

khmakoto left a comment

ecraig12345 Jun 16, 2021

ling1726 Jun 17, 2021

ling1726 Jun 17, 2021

ecraig12345 Jun 17, 2021

ecraig12345 Jun 16, 2021

ling1726 Jun 17, 2021

Hotell Jun 21, 2021 •

edited

Loading

ling1726 Jun 23, 2021

Hotell Jun 23, 2021

ling1726 Jun 24, 2021

Hotell Jun 24, 2021

ling1726 commented Jun 17, 2021

ecraig12345 commented Jun 17, 2021 •

edited

Loading

Hotell Jun 21, 2021

Hotell commented Jun 21, 2021

ling1726 commented Jun 23, 2021

layershifter Jun 25, 2021

ling1726 Jun 25, 2021

layershifter Jun 25, 2021

ling1726 Jun 25, 2021


		#### Option 1: Keep using keyboard-key

		We can copy the contents of keyboard-key internally into the v8 `react` folder. We have already done this previously with v7. This lets us modify keyboard-key freely without breaking v8.


		### Use e.key

		Since we no longer target support for IE11, the usage of `e.key` is simplified and no longer needs to have special handling for IE11. We can modify an existing package or create a new one that simply stores all of the keys as constants which are easily tree shaken.


		### Cons

		- Managing the dependency issues in HVCs and products that have mismatching versions of fluent packages that depend on keyboard-key

RFC(keyboard-key): Handle keyboard keys in more modern and tree shakeable way #18587

RFC(keyboard-key): Handle keyboard keys in more modern and tree shakeable way #18587

Conversation

ling1726 commented Jun 16, 2021 • edited Loading

Pull request checklist

Description of changes

Focus areas to test

size-auditor bot commented Jun 16, 2021 • edited Loading

Asset size changes

fabricteam commented Jun 16, 2021 • edited Loading

Perf Analysis (@fluentui/react-northstar)

Potential regressions comparing to master

khmakoto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hotell Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ling1726 commented Jun 17, 2021

ecraig12345 commented Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

Hotell commented Jun 21, 2021

ling1726 commented Jun 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ling1726 commented Jun 16, 2021 •

edited

Loading

size-auditor bot commented Jun 16, 2021 •

edited

Loading

fabricteam commented Jun 16, 2021 •

edited

Loading

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

Hotell Jun 21, 2021 •

edited

Loading

ecraig12345 commented Jun 17, 2021 •

edited

Loading