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(react-menu): revert menuitem focus ring back to border #28698

Merged
merged 2 commits into from
Aug 1, 2023

Conversation

YuanboXue-Amber
Copy link
Contributor

@YuanboXue-Amber YuanboXue-Amber commented Aug 1, 2023

revert #28685

Why outlined focus ring is not used in this specific case:
When Menu is default open, on page load it focuses on the first item, browser will render a default :focus-visible ring with css outline styles. If we don't want this, we need to do two things:

  1. set default focus ring to none by ':focus-visible': { outlineStyle: 'none' }
  2. update our focus ring selector from [&data-fui-focus-visible]: style to [&data-fui-focus-visible]:focus: style, to be more specific and win over the style we added in step 1.
    But this brings new issue - we have consumers overriding focus ring style by using just :focus-visible. If we update our own focus ring selector to be more specific, it breaks their overrides.

Since we don't want focus ring on default load (which will break all vr test), and we don't want to make focus ring selector more specific, we can only revert back to border focus ring.

@YuanboXue-Amber YuanboXue-Amber marked this pull request as ready for review August 1, 2023 11:57
@YuanboXue-Amber YuanboXue-Amber requested a review from a team as a code owner August 1, 2023 11:57
@fabricteam
Copy link
Collaborator

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 606 628 5000
Button mount 308 291 5000
Field mount 1076 1093 5000
FluentProvider mount 639 661 5000
FluentProviderWithTheme mount 77 92 10
FluentProviderWithTheme virtual-rerender 80 83 10
FluentProviderWithTheme virtual-rerender-with-unmount 81 85 10
InfoButton mount 11 17 5000
MakeStyles mount 853 852 50000
Persona mount 1667 1666 5000
SpinButton mount 1405 1338 5000

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 1, 2023

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 77bc5d3:

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

@fabricteam
Copy link
Collaborator

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
206.112 kB
57.404 kB
208.091 kB
57.972 kB
1.979 kB
568 B
react-menu
Menu (including children components)
134.181 kB
40.988 kB
136.16 kB
41.497 kB
1.979 kB
509 B
react-menu
Menu (including selectable components)
136.945 kB
41.467 kB
138.924 kB
41.999 kB
1.979 kB
532 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-components
react-components: Button, FluentProvider & webLightTheme
67.544 kB
18.216 kB
react-components
react-components: FluentProvider & webLightTheme
36.409 kB
12.01 kB
react-portal-compat
PortalCompatProvider
6.48 kB
2.203 kB
🤖 This report was generated against ac8385d586881c06a91811791190b393eefd837c

@size-auditor
Copy link

size-auditor bot commented Aug 1, 2023

Asset size changes

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

Baseline commit: ac8385d586881c06a91811791190b393eefd837c (build)

@fabricteam
Copy link
Collaborator

🕵 fluentuiv9 No visual regressions between this PR and main

@YuanboXue-Amber YuanboXue-Amber merged commit 66117c3 into microsoft:master Aug 1, 2023
21 checks passed
@YuanboXue-Amber YuanboXue-Amber deleted the revert branch August 1, 2023 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants