fix: prevent pixel-fraction overflow from v8 TextField focus indicator #31314
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
|
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. |
fabricteam
reviewed
May 8, 2024
| @@ -0,0 +1,7 @@ | |||
| { | |||
There was a problem hiding this comment.
🕵 fluentuiv8 Open the Visual Regressions report to inspect the affected screenshots
DatePicker 10 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| DatePicker.Required.hover datepicker.chromium.png | 7 | Changed |
| DatePicker.Required.default.chromium.png | 7 | Changed |
| DatePicker.Underlined and Required.default.chromium.png | 7 | Changed |
| DatePicker.Underlined and Required.hover datepicker.chromium.png | 7 | Changed |
| DatePicker.Required.hover day.chromium.png | 7 | Changed |
| DatePicker.Required.hover month.chromium.png | 7 | Changed |
| DatePicker.Required.click.chromium.png | 7 | Changed |
| DatePicker.Underlined and Required.click.chromium.png | 7 | Changed |
| DatePicker.Underlined and Required.hover month.chromium.png | 7 | Changed |
| DatePicker.Underlined and Required.hover day.chromium.png | 7 | Changed |
TextField 13 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| TextField.Multiline nonresizable.click.chromium.png | 2 | Changed |
| TextField.Multiline.click.chromium.png | 2 | Changed |
| TextField.Suffix.click.chromium.png | 2 | Changed |
| TextField.Error - RTL.click.chromium.png | 2 | Changed |
| TextField.Icon - RTL.click.chromium.png | 2 | Changed |
| TextField.Placeholder - RTL.click.chromium.png | 2 | Changed |
| TextField.Error.click.chromium.png | 2 | Changed |
| TextField.Placeholder.click.chromium.png | 2 | Changed |
| TextField.Icon.click.chromium.png | 2 | Changed |
| TextField.Root.click.chromium.png | 2 | Changed |
| TextField.Required.click.chromium.png | 2 | Changed |
| TextField.Multiline - RTL.click.chromium.png | 2 | Changed |
| TextField.Suffix - RTL.click.chromium.png | 2 | Changed |
There was a problem hiding this comment.
oh no, looks like the required asterisk renders outside and having overflow hidden hides it 😅
📊 Bundle size report
Unchanged fixtures
|
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| BaseButton | mount | 632 | 646 | 5000 | |
| Breadcrumb | mount | 1704 | 1687 | 1000 | |
| Checkbox | mount | 1731 | 1697 | 5000 | |
| CheckboxBase | mount | 1500 | 1481 | 5000 | |
| ChoiceGroup | mount | 2935 | 2977 | 5000 | |
| ComboBox | mount | 695 | 658 | 1000 | |
| CommandBar | mount | 6597 | 6528 | 1000 | |
| ContextualMenu | mount | 14556 | 14591 | 1000 | |
| DefaultButton | mount | 774 | 822 | 5000 | |
| DetailsRow | mount | 2204 | 2232 | 5000 | |
| DetailsRowFast | mount | 2281 | 2206 | 5000 | |
| DetailsRowNoStyles | mount | 2071 | 2047 | 5000 | |
| Dialog | mount | 2667 | 2827 | 1000 | |
| DocumentCardTitle | mount | 235 | 235 | 1000 | |
| Dropdown | mount | 2022 | 1997 | 5000 | |
| FocusTrapZone | mount | 1192 | 1187 | 5000 | |
| FocusZone | mount | 1092 | 1092 | 5000 | |
| GroupedList | mount | 37977 | 42425 | 2 | |
| GroupedList | virtual-rerender | 18142 | 20341 | 2 | |
| GroupedList | virtual-rerender-with-unmount | 51474 | 51541 | 2 | |
| GroupedListV2 | mount | 221 | 231 | 2 | |
| GroupedListV2 | virtual-rerender | 215 | 213 | 2 | |
| GroupedListV2 | virtual-rerender-with-unmount | 226 | 230 | 2 | |
| IconButton | mount | 1141 | 1155 | 5000 | |
| Label | mount | 351 | 354 | 5000 | |
| Layer | mount | 2774 | 2759 | 5000 | |
| Link | mount | 402 | 383 | 5000 | |
| MenuButton | mount | 987 | 971 | 5000 | |
| MessageBar | mount | 21843 | 21836 | 5000 | |
| Nav | mount | 2063 | 2037 | 1000 | |
| OverflowSet | mount | 786 | 807 | 5000 | |
| Panel | mount | 1820 | 1844 | 1000 | |
| Persona | mount | 770 | 750 | 1000 | |
| Pivot | mount | 919 | 890 | 1000 | |
| PrimaryButton | mount | 957 | 917 | 5000 | |
| Rating | mount | 4737 | 4624 | 5000 | |
| SearchBox | mount | 910 | 922 | 5000 | |
| Shimmer | mount | 1926 | 1922 | 5000 | |
| Slider | mount | 1367 | 1365 | 5000 | |
| SpinButton | mount | 2954 | 3028 | 5000 | |
| Spinner | mount | 403 | 387 | 5000 | |
| SplitButton | mount | 1897 | 1889 | 5000 | |
| Stack | mount | 420 | 417 | 5000 | |
| StackWithIntrinsicChildren | mount | 884 | 889 | 5000 | |
| StackWithTextChildren | mount | 2631 | 2685 | 5000 | |
| SwatchColorPicker | mount | 6386 | 6354 | 5000 | |
| TagPicker | mount | 1476 | 1463 | 5000 | |
| Text | mount | 376 | 383 | 5000 | |
| TextField | mount | 959 | 925 | 5000 | |
| ThemeProvider | mount | 862 | 836 | 5000 | |
| ThemeProvider | virtual-rerender | 584 | 599 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 1308 | 1271 | 5000 | |
| Toggle | mount | 612 | 616 | 5000 | |
| buttonNative | mount | 203 | 195 | 5000 |
sopranopillow
previously approved these changes
May 9, 2024
sopranopillow
dismissed
their stale review
There are some issues with the required asterisk that should be addressed before merging
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
Sometimes at certain zoom levels, some browsers calculate the width of the
::afterfocus indicator on TextField to be fractions of a pixel larger than the root node. That can result in conditional scroll overflow when the textfield gets focus.New Behavior
Adds
overflow: hiddento the node that has the focus indicator::afterstyle. That node only wraps the input element, so there shouldn't be any side effects to hiding overflow.Related Issue