Skip to content

fix(Toolbar): close previous submenu when opening another submenu#24836

Merged
miroslavstastny merged 6 commits intomicrosoft:masterfrom
miroslavstastny:fix/northstar-toolbar-outside-click
Sep 27, 2022
Merged

fix(Toolbar): close previous submenu when opening another submenu#24836
miroslavstastny merged 6 commits intomicrosoft:masterfrom
miroslavstastny:fix/northstar-toolbar-outside-click

Conversation

@miroslavstastny
Copy link
Member

@miroslavstastny miroslavstastny commented Sep 16, 2022 •
edited
Loading

Current Behavior

To close a Toolbar submenu when another submenu is opened, there was a DOM event listener applied to document in bubble phase.
At the same time, in ToolbarMenuItem React click handler, the click event propagation is stopped when there is a menu (or popup) attached to the item.

In React < 17, React attaches its event listeners to the document element. With that, even though the event propagation is stopped in the click handler, it is still processed by the outside click handler as the two event listeners are attached to the same DOM element.

In React 17, React attaches its event listeners to the mount root, which is a child of the document. Now, when the event propagation is stopped in the click handler (React root in DOM) it does not trigger outside click handler (document in DOM). For that reason the old submenu is not closed.

New Behavior

The outside click handler was moved to a capture phase and is invoked before the item click handler. The old menu is closed as expected.

Related Issue(s)

Fixes #24833

@miroslavstastny miroslavstastny self-assigned this Sep 16, 2022
@github-actions github-actions bot added this to the July Project Cycle Q3 2022 milestone Sep 16, 2022
@miroslavstastny miroslavstastny added the Ready for VR Used to trigger screener checks for PRs label Sep 16, 2022
@miroslavstastny
Copy link
Member Author

miroslavstastny commented Sep 16, 2022

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Sep 16, 2022

Azure Pipelines successfully started running 3 pipeline(s).

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 16, 2022 •
edited
Loading

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 84e0df1:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration
toolbar-submenu-react17 Issue #24833

@size-auditor
Copy link

size-auditor bot commented Sep 16, 2022 •
edited
Loading

Asset size changes

Project Bundle Baseline Size New Size Difference
office-ui-fabric-react fluentui-react-northstar-Toolbar 181.355 kB 181.366 kB ExceedsBaseline     11 bytes

ExceedsTolerance Over Tolerance (1024 B) ExceedsBaseline Over Baseline BelowBaseline Below Baseline New New Deleted  Removed 1 kB = 1000 B

Baseline commit: 0e50bf23a6604793d0eaa2626c87dbec9cd600d8 (build)

@fabricteam
Copy link
Collaborator

fabricteam commented Sep 16, 2022 •
edited
Loading

📊 Bundle size report

🤖 This report was generated against 0e50bf23a6604793d0eaa2626c87dbec9cd600d8

@miroslavstastny
Copy link
Member Author

miroslavstastny commented Sep 16, 2022

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Sep 16, 2022

Azure Pipelines successfully started running 3 pipeline(s).

@fabricteam
Copy link
Collaborator

fabricteam commented Sep 26, 2022 •
edited
Loading

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
AttachmentMinimalPerf.default 124 102 1.22:1
ChatWithPopoverPerf.default 310 262 1.18:1
HeaderSlotsPerf.default 621 537 1.16:1
DropdownManyItemsPerf.default 552 484 1.14:1
FlexMinimalPerf.default 230 206 1.12:1
TreeWith60ListItems.default 127 113 1.12:1
AvatarMinimalPerf.default 160 146 1.1:1
DropdownMinimalPerf.default 2177 1978 1.1:1
TextAreaMinimalPerf.default 370 337 1.1:1
SkeletonMinimalPerf.default 257 238 1.08:1
ButtonMinimalPerf.default 123 116 1.06:1
EmbedMinimalPerf.default 2798 2645 1.06:1
LayoutMinimalPerf.default 262 250 1.05:1
MenuMinimalPerf.default 624 595 1.05:1
RosterPerf.default 1609 1537 1.05:1
ReactionMinimalPerf.default 276 263 1.05:1
SliderMinimalPerf.default 1186 1133 1.05:1
DividerMinimalPerf.default 256 245 1.04:1
PortalMinimalPerf.default 124 119 1.04:1
TableManyItemsPerf.default 1472 1415 1.04:1
AlertMinimalPerf.default 189 183 1.03:1
ImageMinimalPerf.default 273 266 1.03:1
RefMinimalPerf.default 157 153 1.03:1
TextMinimalPerf.default 247 240 1.03:1
AccordionMinimalPerf.default 103 101 1.02:1
CarouselMinimalPerf.default 334 328 1.02:1
ChatDuplicateMessagesPerf.default 194 190 1.02:1
ItemLayoutMinimalPerf.default 912 893 1.02:1
ListWith60ListItems.default 462 455 1.02:1
MenuButtonMinimalPerf.default 1254 1234 1.02:1
PopupMinimalPerf.default 462 455 1.02:1
AttachmentSlotsPerf.default 808 802 1.01:1
InputMinimalPerf.default 834 824 1.01:1
ListMinimalPerf.default 368 364 1.01:1
ProviderMinimalPerf.default 299 295 1.01:1
StatusMinimalPerf.default 491 485 1.01:1
CustomToolbarPrototype.default 2000 1975 1.01:1
ToolbarMinimalPerf.default 669 665 1.01:1
GridMinimalPerf.default 239 239 1:1
HeaderMinimalPerf.default 290 291 1:1
LoaderMinimalPerf.default 485 485 1:1
TableMinimalPerf.default 307 306 1:1
TreeMinimalPerf.default 665 662 1:1
AnimationMinimalPerf.default 383 386 0.99:1
CheckboxMinimalPerf.default 1521 1531 0.99:1
LabelMinimalPerf.default 270 272 0.99:1
ProviderMergeThemesPerf.default 938 949 0.99:1
TooltipMinimalPerf.default 1753 1778 0.99:1
VideoMinimalPerf.default 538 548 0.98:1
ChatMinimalPerf.default 531 546 0.97:1
DatepickerMinimalPerf.default 4246 4359 0.97:1
ButtonOverridesMissPerf.default 959 999 0.96:1
SplitButtonMinimalPerf.default 3243 3369 0.96:1
IconMinimalPerf.default 500 522 0.96:1
SegmentMinimalPerf.default 255 269 0.95:1
CardMinimalPerf.default 376 398 0.94:1
ButtonSlotsPerf.default 388 416 0.93:1
FormMinimalPerf.default 278 300 0.93:1
ListCommonPerf.default 445 477 0.93:1
DialogMinimalPerf.default 562 622 0.9:1
RadioGroupMinimalPerf.default 313 350 0.89:1
BoxMinimalPerf.default 236 269 0.88:1
ListNestedPerf.default 391 444 0.88:1

@miroslavstastny miroslavstastny marked this pull request as ready for review September 26, 2022 15:01
@miroslavstastny miroslavstastny requested a review from a team as a code owner September 26, 2022 15:01
@miroslavstastny miroslavstastny merged commit e58b334 into microsoft:master Sep 27, 2022
@miroslavstastny miroslavstastny deleted the fix/northstar-toolbar-outside-click branch September 27, 2022 07:06
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request Sep 28, 2022
@marcosmoura
Merge branch 'master' into marcosmoura/feat/selectable-cards
419ff48 
* master: (21 commits)
  chore: Migrate react-avatar to use new build (microsoft#24969)
  applying package updates
  chore(react-input, react-textarea): Deprecating filled with shadow appearance variants (microsoft#24900)
  fix: v8 Dropdown no longer sets incorrect and unnecessary aria-activedescendant (microsoft#24593)
  feat: v0 Tooltip migration from v9 (microsoft#24908)
  chore: bump devDeps to fix critical security vulnerability (microsoft#24891)
  Fixing Tree chart issues (microsoft#24752)
  init: new package react-avatar-context (microsoft#24968)
  ci(.github): add issues write permisions to triage-bot worflow (microsoft#24963)
  applying package updates
  fix(Toolbar): close previous submenu when opening another submenu (microsoft#24836)
  fix: update non-focus-trap Popover role to be group (microsoft#24897)
  feat: Avatar's aria label includes 'active' or 'inactive' when using the active prop (microsoft#24901)
  feat(scripts): implement triage-bot module (microsoft#24911)
  chore: bump @octokit/rest to v18 (microsoft#24919)
  stress test: add "build-fixture" command (microsoft#24928)
  BREAKING-CHANGE: new ChatMessageContent for style caching (microsoft#24691)
  bugfix: fix changefile to properly update version of react-components with a patch (microsoft#24949)
  feat(scripts): enable strict checking for additional sub-folders(packages) (microsoft#24526)
  chore: exports DialogContent as unstable (microsoft#24943)
  ...
NotWoods pushed a commit to NotWoods/fluentui that referenced this pull request Nov 18, 2022
@miroslavstastny @NotWoods
fix(Toolbar): close previous submenu when opening another submenu (mi…
1437fe0 
…crosoft#24836)

* fix(Toolbar): close previous submenu when opening another submenu

* bump screener

* fix screener test selectors

* Listen for outside click in capture

* changelog
@khmakoto khmakoto added the Fluent UI react-northstar (v0) Work related to Fluent UI V0 label Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@layershifter layershifter layershifter approved these changes

Assignees

@miroslavstastny miroslavstastny

Labels

Fluent UI react-northstar (v0) Work related to Fluent UI V0 Ready for VR Used to trigger screener checks for PRs

Projects

None yet

Milestone

September Project Cycle Q1 2022

Development

Successfully merging this pull request may close these issues.

[Bug]: Toolbar submenu does not close when another submenu opens

5 participants

@miroslavstastny @fabricteam @layershifter @khmakoto @msft-fluent-ui-bot