Keytips: Align keytipData with visible instance for dupes #28522
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…plicate registrations)
|
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 1dcaf74:
|
fabricteam
reviewed
Jul 13, 2023
📊 Bundle size report🤖 This report was generated against 8d06e6d07e96f556e72b7a597664f14bef933eac |
Asset size changes
Baseline commit: 8d06e6d07e96f556e72b7a597664f14bef933eac (build) |
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| BaseButton | mount | 615 | 623 | 5000 | |
| Breadcrumb | mount | 1659 | 1648 | 1000 | |
| Checkbox | mount | 1684 | 1669 | 5000 | |
| CheckboxBase | mount | 1459 | 1473 | 5000 | |
| ChoiceGroup | mount | 2950 | 2973 | 5000 | |
| ComboBox | mount | 665 | 641 | 1000 | |
| CommandBar | mount | 6151 | 6178 | 1000 | |
| ContextualMenu | mount | 11595 | 11758 | 1000 | |
| DefaultButton | mount | 744 | 754 | 5000 | |
| DetailsRow | mount | 2186 | 2172 | 5000 | |
| DetailsRowFast | mount | 2137 | 2219 | 5000 | |
| DetailsRowNoStyles | mount | 1983 | 2019 | 5000 | |
| Dialog | mount | 2597 | 2786 | 1000 | |
| DocumentCardTitle | mount | 227 | 210 | 1000 | |
| Dropdown | mount | 1982 | 1965 | 5000 | |
| FocusTrapZone | mount | 1109 | 1098 | 5000 | |
| FocusZone | mount | 1049 | 1056 | 5000 | |
| GroupedList | mount | 36896 | 41375 | 2 | |
| GroupedList | virtual-rerender | 19625 | 19668 | 2 | |
| GroupedList | virtual-rerender-with-unmount | 50021 | 50060 | 2 | |
| GroupedListV2 | mount | 228 | 221 | 2 | |
| GroupedListV2 | virtual-rerender | 210 | 208 | 2 | |
| GroupedListV2 | virtual-rerender-with-unmount | 220 | 228 | 2 | |
| IconButton | mount | 1096 | 1090 | 5000 | |
| Label | mount | 322 | 322 | 5000 | |
| Layer | mount | 2707 | 2723 | 5000 | |
| Link | mount | 395 | 390 | 5000 | |
| MenuButton | mount | 912 | 922 | 5000 | |
| MessageBar | mount | 21229 | 21197 | 5000 | |
| Nav | mount | 1899 | 1909 | 1000 | |
| OverflowSet | mount | 780 | 788 | 5000 | |
| Panel | mount | 1791 | 1750 | 1000 | |
| Persona | mount | 715 | 753 | 1000 | |
| Pivot | mount | 860 | 868 | 1000 | |
| PrimaryButton | mount | 830 | 834 | 5000 | |
| Rating | mount | 4603 | 4565 | 5000 | |
| SearchBox | mount | 917 | 922 | 5000 | |
| Shimmer | mount | 1845 | 1896 | 5000 | |
| Slider | mount | 1307 | 1317 | 5000 | |
| SpinButton | mount | 2862 | 2834 | 5000 | |
| Spinner | mount | 388 | 385 | 5000 | |
| SplitButton | mount | 1829 | 1818 | 5000 | |
| Stack | mount | 400 | 401 | 5000 | |
| StackWithIntrinsicChildren | mount | 849 | 867 | 5000 | |
| StackWithTextChildren | mount | 2648 | 2663 | 5000 | |
| SwatchColorPicker | mount | 6067 | 6030 | 5000 | |
| TagPicker | mount | 1442 | 1444 | 5000 | |
| Text | mount | 370 | 368 | 5000 | |
| TextField | mount | 908 | 916 | 5000 | |
| ThemeProvider | mount | 838 | 827 | 5000 | |
| ThemeProvider | virtual-rerender | 587 | 589 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 1288 | 1258 | 5000 | |
| Toggle | mount | 610 | 600 | 5000 | |
| buttonNative | mount | 193 | 197 | 5000 |
khmakoto
approved these changes
Jul 17, 2023
marcosmoura
added a commit
to marcosmoura/fluentui
that referenced
this pull request
Jul 18, 2023
* master: ci(bundlesize): opt out using --since as lightrail doesnt support this behaviour and needs everything build on every PR (microsoft#28545) feat: automatically add v9 package stories to public docsite and correctly create codesandbox demo source code (microsoft#28528) applying package updates Keytips: Align keytipData with visible instance for dupes (microsoft#28522) applying package updates V8 Fluent2 Theme: Spinner sizes (microsoft#28512) feat: add extra-tiny size value to size prop (microsoft#28249) feat(react-infobutton): Remove InfoIcon from react-infobutton (microsoft#28534) Made Breadcrumb package public (microsoft#28549) X bars showing incorrect data when the values are large- bug 8380 (microsoft#28510) feat(tools): implement `prepare-initial-release` generator (microsoft#28505) feat: release react-breadcrumb to preview (microsoft#28402) applying package updates fix: v8 SplitButton and split MenuItem have two touch targets when checkable (microsoft#28523) fix(react-infobutton): Apply aria-owns only when the popover is open and cleanup infobutton stories (microsoft#28463) fix: Pivot overflow role uses tab (microsoft#28409) Migrate react-search to preview (microsoft#28531)
makopch-ms
added a commit
to makopch-ms/fluentui
that referenced
this pull request
Aug 24, 2023
…crosoft#28522)" This reverts commit bbba728.
khmakoto
pushed a commit
that referenced
this pull request
Aug 24, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previous Behavior
Currently, keytips have the ability to render only for visible instances of target elements, but if there are multiple of the same registration (e.g. keytipData), the first one is always used. This can result in the keytip not being positioned correctly for the instance that's visible. Note, this is related to responsive scaling where the same control can be rendered with the same keytip tree multiple times where only one instance is visible at a time
New Behavior
With this change, the fetching of the keytipData will now return the instance that's associated with the corresponding visible element instance. This makes the keytipData that's used align with how keytips already choose where to be visible (e.g. which element is their target). This new code will only run if there are multiple keytips with the exact same registration (same keytip sequence (e.g. tree and element keytip)) which can happen for responsive scaling.