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

[Northstar][Dropdown] Fix styling mutation when merging themes #24787

Merged
merged 3 commits into from
Sep 16, 2022

Conversation

petrjaros
Copy link
Contributor

Current Behavior

When overriding :active or :hover state it mutates the style object. Style override of single Dropdown stay permanent, and the style is then applied to all unrelated Dropdowns.

New Behavior

Styling of one Dropdown does not leak into other Dropdowns.

@ghost
Copy link

ghost commented Sep 14, 2022

CLA assistant check
All CLA requirements met.

@fabricteam
Copy link
Collaborator

fabricteam commented Sep 14, 2022

📊 Bundle size report

🤖 This report was generated against e23edb70bfcde6fc5f82b676a1ddcf8d5a854dbd

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 14, 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 97c53a8:

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

@fabricteam
Copy link
Collaborator

fabricteam commented Sep 14, 2022

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
ChatWithPopoverPerf.default 307 283 1.08:1
ListNestedPerf.default 480 451 1.06:1
ListWith60ListItems.default 501 480 1.04:1
LoaderMinimalPerf.default 528 510 1.04:1
RefMinimalPerf.default 190 183 1.04:1
AttachmentSlotsPerf.default 872 844 1.03:1
DividerMinimalPerf.default 318 308 1.03:1
GridMinimalPerf.default 297 289 1.03:1
HeaderMinimalPerf.default 322 312 1.03:1
HeaderSlotsPerf.default 692 674 1.03:1
SkeletonMinimalPerf.default 308 299 1.03:1
TextAreaMinimalPerf.default 417 405 1.03:1
AnimationMinimalPerf.default 473 466 1.02:1
BoxMinimalPerf.default 296 289 1.02:1
ButtonOverridesMissPerf.default 1033 1010 1.02:1
DropdownManyItemsPerf.default 541 533 1.02:1
LabelMinimalPerf.default 341 335 1.02:1
MenuButtonMinimalPerf.default 1370 1349 1.02:1
RadioGroupMinimalPerf.default 396 387 1.02:1
TextMinimalPerf.default 308 301 1.02:1
TreeWith60ListItems.default 131 129 1.02:1
AlertMinimalPerf.default 222 220 1.01:1
CheckboxMinimalPerf.default 1543 1526 1.01:1
DialogMinimalPerf.default 693 689 1.01:1
DropdownMinimalPerf.default 2176 2148 1.01:1
ImageMinimalPerf.default 341 336 1.01:1
InputMinimalPerf.default 851 843 1.01:1
ListCommonPerf.default 509 504 1.01:1
TableMinimalPerf.default 359 355 1.01:1
ToolbarMinimalPerf.default 796 791 1.01:1
TreeMinimalPerf.default 705 701 1.01:1
AccordionMinimalPerf.default 126 126 1:1
CarouselMinimalPerf.default 355 356 1:1
ChatMinimalPerf.default 641 639 1:1
DatepickerMinimalPerf.default 4679 4668 1:1
FormMinimalPerf.default 325 324 1:1
ItemLayoutMinimalPerf.default 967 964 1:1
RosterPerf.default 1714 1715 1:1
PopupMinimalPerf.default 556 554 1:1
ProviderMergeThemesPerf.default 997 993 1:1
ReactionMinimalPerf.default 335 335 1:1
SliderMinimalPerf.default 1226 1225 1:1
StatusMinimalPerf.default 607 610 1:1
TableManyItemsPerf.default 1569 1563 1:1
TooltipMinimalPerf.default 1863 1854 1:1
AvatarMinimalPerf.default 160 162 0.99:1
ButtonMinimalPerf.default 133 135 0.99:1
CardMinimalPerf.default 460 467 0.99:1
ChatDuplicateMessagesPerf.default 228 230 0.99:1
EmbedMinimalPerf.default 2631 2650 0.99:1
ListMinimalPerf.default 442 446 0.99:1
MenuMinimalPerf.default 734 739 0.99:1
PortalMinimalPerf.default 133 134 0.99:1
SplitButtonMinimalPerf.default 3256 3284 0.99:1
CustomToolbarPrototype.default 2148 2161 0.99:1
FlexMinimalPerf.default 251 255 0.98:1
LayoutMinimalPerf.default 313 318 0.98:1
ProviderMinimalPerf.default 314 321 0.98:1
VideoMinimalPerf.default 607 618 0.98:1
AttachmentMinimalPerf.default 125 129 0.97:1
ButtonSlotsPerf.default 422 433 0.97:1
SegmentMinimalPerf.default 296 305 0.97:1
IconMinimalPerf.default 550 585 0.94:1

@size-auditor
Copy link

size-auditor bot commented Sep 14, 2022

Asset size changes

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

Baseline commit: e23edb70bfcde6fc5f82b676a1ddcf8d5a854dbd (build)

@layershifter
Copy link
Member

Can you please provide a reproduction case for this behavior?

@petrjaros
Copy link
Contributor Author

Here is the codesandbox: https://codesandbox.io/s/fluent-ui-example-forked-c58ijl?file=/example.js

  1. Open the link. You should see two Dropdowns. Try to click both. All text should stay black. No blue text.
  2. Uncomment the commented line.
  3. Click dropdowns

Actual behavior:
Both Dropdowns have blue text color when clicking or when focused.
When you comment the line again, the wrong styling stays in both dropdowns even if it should not be applied at all.

Expected behavior:
The blue styling should not leak into unrelated components nor focus state.

@petrjaros
Copy link
Contributor Author

I have accidentally created the example with older version of Fluent, but the latest (0.64.0) is also affected.

@layershifter
Copy link
Member

@petrjaros thanks for providing the repro.

The root reason is there:

return _.merge(
typeof slotA === 'function' ? slotA(styleParam) : slotA,
typeof slotB === 'function' ? slotB(styleParam) : slotB,
);

_.merge in Lodash mutates origin:

Note: This method mutates object.

https://lodash.com/docs/4.17.15#merge


I talked with a team and we agreed to merge this PR instead of fixing that call to avoid potential memory issues. Especially taking into account that this call was there for ages.

@layershifter layershifter enabled auto-merge (squash) September 15, 2022 09:44
@layershifter
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

auto-merge was automatically disabled September 16, 2022 08:26

Pull request was closed

@layershifter layershifter reopened this Sep 16, 2022
@layershifter layershifter reopened this Sep 16, 2022
@layershifter layershifter merged commit 9c8ab6e into microsoft:master Sep 16, 2022
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request Sep 19, 2022
* master: (28 commits)
  fix: use trigger prop for aria-haspopup (microsoft#24794)
  chore(react-dialog): scaffold DialogContent component (microsoft#24844)
  chore: Northstar screener should read from screenerStates.json (microsoft#24848)
  applying package updates
  (web components) Standardize focus treatment (microsoft#24771)
  Divider - allow default prop override (microsoft#24840)
  GroupedList: fix virtualization (unstable preview) (microsoft#24460)
  fix: Add explicit children prop to TeachingBubble to support React 18 (microsoft#24823)
  feat: Adds `visible` prop to `TableCellActions` (microsoft#24831)
  [Northstar][Dropdown] Fix styling mutation when merging themes (microsoft#24787)
  fix: export `tableCellActionsClassNames` from unstable (microsoft#24830)
  bugfix(react-dialog): Adds color style to DialogSurface (microsoft#24832)
  applying package updates
  Prevent group toggling from selecting the whole group (microsoft#24822)
  feat(react-textarea): Add shadow variant of filled appearance (microsoft#24512)
  applying package updates
  Adding lib-commonjs top-level entries to exports map (microsoft#24792)
  Created shim packages (microsoft#24780)
  feat(react-menu): replace keydown handlers by useARIAButtonShorthand on MenuItem (microsoft#24738)
  fix: update version mismatches triggered by v9 release (microsoft#24812)
  ...
@khmakoto khmakoto added Fluent UI react-northstar (v0) Work related to Fluent UI V0 and removed Fluent UI react-northstar labels Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fluent UI react-northstar (v0) Work related to Fluent UI V0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants