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

fix: do not use outlineStyle: none in createCustomFocusIndicatorStyle #26089

Merged
merged 13 commits into from
May 5, 2023

Conversation

smhigley
Copy link
Contributor

@smhigley smhigley commented Dec 22, 2022

Edited -- now the outlineStyle exists in createFocusOutlineStyle, not createCustomFocusIndicatorStyle

Previous Behavior

When createCustomFocusIndicatorStyle is called twice within a component's styles hook, the second call creates :focus/:focus-visible outline styles that override any styles in the first call of the hook, based on the order in which classes are applied to the component.

An example is when the first call does this:

createCustomFocusIndicatorStyle({
    borderColor: tokens.colorTransparentStroke,
    borderRadius: tokens.borderRadiusMedium,
    outlineColor: tokens.colorTransparentStroke,
    outlineStyle: 'solid',
    outlineWidth: tokens.strokeWidthThick,
    boxShadow: `
      ${tokens.shadow4},
      0 0 0 2px ${tokens.colorStrokeFocus2}
    `,
    zIndex: 1,
}),

and the second call does this:

// circular border radius
createCustomFocusIndicatorStyle({
    ...shorthands.borderRadius(tokens.borderRadiusCircular),
}),

The second call overrides the outlineStyle defined in the first call, because the second className is applied after the first. An example of this is in useButtonStyles, where the base style calls createCustomFocusIndicatorStyle once, and size and appearance variants call it again style adjustments.

This issue shows how using a large button broke the high contrast mode focus indicator: #26079

New Behavior

This PR removes outlineStyle: none from createCustomFocusIndicatorStyle and moves it to createFocusOutlineStyle, where the ::after style is created and is visible in high contrast mode.

Related Issue(s)

Fixes #26079, #26077, #26062

@size-auditor
Copy link

size-auditor bot commented Dec 22, 2022

Asset size changes

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

Baseline commit: a59a1056a6110af48fd7a7ab189ced2a6a9567d1 (build)

@fabricteam
Copy link
Collaborator

fabricteam commented Dec 22, 2022

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-alert
Alert
93.799 kB
22.502 kB
93.344 kB
22.431 kB
-455 B
-71 B
react-button
Button
37.179 kB
9.534 kB
36.724 kB
9.458 kB
-455 B
-76 B
react-button
CompoundButton
44.328 kB
11.016 kB
43.873 kB
10.939 kB
-455 B
-77 B
react-button
MenuButton
41.866 kB
10.861 kB
41.411 kB
10.791 kB
-455 B
-70 B
react-button
SplitButton
50.254 kB
12.441 kB
49.635 kB
12.364 kB
-619 B
-77 B
react-button
ToggleButton
55.397 kB
11.435 kB
54.814 kB
11.358 kB
-583 B
-77 B
react-components
react-components: Button, FluentProvider & webLightTheme
65.31 kB
17.931 kB
64.855 kB
17.852 kB
-455 B
-79 B
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
204.34 kB
57.141 kB
203.885 kB
57.081 kB
-455 B
-60 B
react-link
Link
12.357 kB
5.091 kB
12.304 kB
5.061 kB
-53 B
-30 B
react-table
DataGrid
147.659 kB
40.567 kB
147.531 kB
40.517 kB
-128 B
-50 B
react-table
Table as DataGrid
130.218 kB
33.128 kB
130.09 kB
33.08 kB
-128 B
-48 B
react-table
Table (Selection only)
78.132 kB
19.127 kB
78.004 kB
19.079 kB
-128 B
-48 B
react-table
Table (Sort only)
77.462 kB
18.939 kB
77.334 kB
18.891 kB
-128 B
-48 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-accordion
Accordion (including children components)
86.012 kB
26.086 kB
react-avatar
Avatar
57.564 kB
14.976 kB
react-avatar
AvatarGroup
15.632 kB
6.258 kB
react-avatar
AvatarGroupItem
73.778 kB
19.491 kB
react-card
Card - All
86.061 kB
24.345 kB
react-card
Card
80.997 kB
22.895 kB
react-card
CardFooter
9.158 kB
3.844 kB
react-card
CardHeader
11.048 kB
4.538 kB
react-card
CardPreview
9.963 kB
4.192 kB
react-checkbox
Checkbox
34.216 kB
10.784 kB
react-combobox
Combobox (including child components)
87.199 kB
28.095 kB
react-combobox
Dropdown (including child components)
85.583 kB
27.692 kB
react-components
react-components: FluentProvider & webLightTheme
36.086 kB
11.9 kB
react-datepicker-compat
DatePicker Compat
220.251 kB
58.471 kB
react-dialog
Dialog (including children components)
90.867 kB
27.047 kB
react-infobutton
InfoButton
127.925 kB
39.003 kB
react-infobutton
InfoLabel
131.208 kB
39.996 kB
react-menu
Menu (including children components)
128.202 kB
39.18 kB
react-menu
Menu (including selectable components)
131.186 kB
39.696 kB
react-persona
Persona
64.485 kB
16.9 kB
react-popover
Popover
114.917 kB
35.388 kB
react-portal
Portal
11.649 kB
4.262 kB
react-portal-compat
PortalCompatProvider
6.446 kB
2.186 kB
react-provider
FluentProvider
18.033 kB
6.666 kB
react-radio
Radio
27.282 kB
8.661 kB
react-radio
RadioGroup
11.312 kB
4.71 kB
react-slider
Slider
34.112 kB
11.018 kB
react-switch
Switch
29.806 kB
9.274 kB
react-table
Table (Primitives only)
44.348 kB
12.347 kB
react-tooltip
Tooltip
46.656 kB
16.369 kB
🤖 This report was generated against a59a1056a6110af48fd7a7ab189ced2a6a9567d1

@fabricteam
Copy link
Collaborator

fabricteam commented Dec 22, 2022

Perf Analysis (@fluentui/react-components)

Scenario Render type Master Ticks PR Ticks Iterations Status
InfoButton mount 18 22 5000 Possible regression
All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 688 700 5000
Button mount 375 383 5000
Field mount 1261 1250 5000
FluentProvider mount 904 905 5000
FluentProviderWithTheme mount 114 117 10
FluentProviderWithTheme virtual-rerender 98 97 10
FluentProviderWithTheme virtual-rerender-with-unmount 102 105 10
InfoButton mount 18 22 5000 Possible regression
MakeStyles mount 1103 1106 50000
Persona mount 2030 1935 5000
SpinButton mount 1570 1568 5000

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 22, 2022

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 7bc671c:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration
flamboyant-nash-g9ncl0 Issue #26079
peaceful-bartik-p7p9eu Issue #26079
stoic-wave-b3lx77 Issue #26079

@smhigley
Copy link
Contributor Author

updated createFocusOutlineStyle to support enableOutline again after chatting w/ @ling1726, since we realized it was already part of the public api signature 🙈

@smhigley smhigley requested a review from ling1726 January 28, 2023 00:10
@smhigley smhigley requested review from a team, behowell and sopranopillow as code owners January 30, 2023 21:58
@ling1726 ling1726 merged commit 81b87f4 into microsoft:master May 5, 2023
@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/react-docsite-components@v8.12.6 has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/react-monaco-editor@v1.7.83 has been released which incorporates this pull request.:tada:

Handy links:

marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request May 10, 2023
* master: (29 commits)
  feat(react-drawer): create DrawerBody component (microsoft#27581)
  feat(react-tree): TreeItem itemType restructure (microsoft#27799)
  feat: Implement state management for toasts (microsoft#27800)
  bugfix: fix VisibleFlatTreeItemGenerator omitting visible items (microsoft#27802)
  fix: overflowManager should always dispatch initial state (microsoft#27756)
  fix(react-badge): Remove white border around presence badge when on a dark background (microsoft#27780)
  react-tags: rename dismissable to dismissible (microsoft#27798)
  chore: update ownership of react-tags (microsoft#27795)
  applying package updates
  feat(react-tags): Replace `avatar` prop with `media`; polish styles for spacing (microsoft#27725)
  RFC appearance migration (microsoft#24181)
  chore(react-skeleton): Release react-skeleton to stable (microsoft#27767)
  fix(Coachmark): Remove positioning regression and update bounds on resize (microsoft#27782)
  applying package updates
  fix(v8): explicitly publish dist folder after node 16 upgrade (microsoft#27769)
  applying package updates
  fix: do not use outlineStyle: none in createCustomFocusIndicatorStyle (microsoft#26089)
  fix: Scale pulse animation with percentages and flip wave animation's direction (microsoft#27654)
  Publish dist folder that got removed due to node 16 upgrade (microsoft#27764)
  fix(scripts-update-release-notes): properly handle git for-each-ref cmd call to not fail release notes update (microsoft#27757)
  ...
@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/react-avatar@v9.5.0 has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/react-table@v9.2.8 has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/react-link@v9.0.37 has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/react-tabster@v9.7.0 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: HC mode >Tooltip > focus outline inconsistency > Button with tooltip has focus indicator hardly visible
6 participants