Skip to content

Conversation

@tjgupta
Copy link
Contributor

@tjgupta tjgupta commented Jun 12, 2025

Description

This change removes a layout thrashing (style invalidation/recalculation) found in the render function of the Combobox component.

Motivation and context

Removes a performance bottleneck.

Related issue(s)

Screenshots (if appropriate)

Before:
Combobox style recalc

After:
Combobox no style recalc

Combobox min-width

Author's checklist

  • I have read the CONTRIBUTING and PULL_REQUESTS documents.
  • I have reviewed at the Accessibility Practices for this feature, see: Aria Practices
  • I have added automated tests to cover my changes.
  • I have included a well-written changeset if my change needs to be published.
  • I have included updated documentation if my change required it.

Reviewer's checklist

  • Includes a Github Issue with appropriate flag or Jira ticket number without a link
  • Includes thoughtfully written changeset if changes suggested include patch, minor, or major features
  • Automated tests cover all use cases and follow best practices for writing
  • Validated on all supported browsers
  • All VRTs are approved before the author can update Golden Hash

Manual review test cases

  • Descriptive Test Statement

    1. Go here
    2. Do this action
    3. Expect this result
  • Descriptive Test Statement

    1. Go here
    2. Do this action
    3. Expect this result

Device review

  • Did it pass in Desktop?
  • Did it pass in (emulated) Mobile?
  • Did it pass in (emulated) iPad?

@changeset-bot
Copy link

changeset-bot bot commented Jun 12, 2025

🦋 Changeset detected

Latest commit: e5abda5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 84 packages
Name Type
@spectrum-web-components/combobox Patch
@spectrum-web-components/bundle Patch
documentation Patch
@spectrum-web-components/eslint-plugin Patch
@spectrum-web-components/accordion Patch
@spectrum-web-components/action-bar Patch
@spectrum-web-components/action-button Patch
@spectrum-web-components/action-group Patch
@spectrum-web-components/action-menu Patch
@spectrum-web-components/alert-banner Patch
@spectrum-web-components/alert-dialog Patch
@spectrum-web-components/asset Patch
@spectrum-web-components/avatar Patch
@spectrum-web-components/badge Patch
@spectrum-web-components/breadcrumbs Patch
@spectrum-web-components/button-group Patch
@spectrum-web-components/button Patch
@spectrum-web-components/card Patch
@spectrum-web-components/checkbox Patch
@spectrum-web-components/clear-button Patch
@spectrum-web-components/close-button Patch
@spectrum-web-components/coachmark Patch
@spectrum-web-components/color-area Patch
@spectrum-web-components/color-field Patch
@spectrum-web-components/color-handle Patch
@spectrum-web-components/color-loupe Patch
@spectrum-web-components/color-slider Patch
@spectrum-web-components/color-wheel Patch
@spectrum-web-components/contextual-help Patch
@spectrum-web-components/dialog Patch
@spectrum-web-components/divider Patch
@spectrum-web-components/dropzone Patch
@spectrum-web-components/field-group Patch
@spectrum-web-components/field-label Patch
@spectrum-web-components/help-text Patch
@spectrum-web-components/icon Patch
@spectrum-web-components/icons-ui Patch
@spectrum-web-components/icons-workflow Patch
@spectrum-web-components/icons Patch
@spectrum-web-components/iconset Patch
@spectrum-web-components/illustrated-message Patch
@spectrum-web-components/infield-button Patch
@spectrum-web-components/link Patch
@spectrum-web-components/menu Patch
@spectrum-web-components/meter Patch
@spectrum-web-components/modal Patch
@spectrum-web-components/number-field Patch
@spectrum-web-components/overlay Patch
@spectrum-web-components/picker-button Patch
@spectrum-web-components/picker Patch
@spectrum-web-components/popover Patch
@spectrum-web-components/progress-bar Patch
@spectrum-web-components/progress-circle Patch
@spectrum-web-components/radio Patch
@spectrum-web-components/search Patch
@spectrum-web-components/sidenav Patch
@spectrum-web-components/slider Patch
@spectrum-web-components/split-view Patch
@spectrum-web-components/status-light Patch
@spectrum-web-components/swatch Patch
@spectrum-web-components/switch Patch
@spectrum-web-components/table Patch
@spectrum-web-components/tabs Patch
@spectrum-web-components/tags Patch
@spectrum-web-components/textfield Patch
@spectrum-web-components/thumbnail Patch
@spectrum-web-components/toast Patch
@spectrum-web-components/tooltip Patch
@spectrum-web-components/top-nav Patch
@spectrum-web-components/tray Patch
@spectrum-web-components/underlay Patch
@spectrum-web-components/custom-vars-viewer Patch
@spectrum-web-components/story-decorator Patch
@spectrum-web-components/vrt-compare Patch
@spectrum-web-components/base Patch
@spectrum-web-components/grid Patch
@spectrum-web-components/opacity-checkerboard Patch
@spectrum-web-components/reactive-controllers Patch
@spectrum-web-components/shared Patch
@spectrum-web-components/styles Patch
@spectrum-web-components/theme Patch
@spectrum-web-components/truncated Patch
example-project-rollup Patch
example-project-webpack Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

"@spectrum-web-components/picker-button": "1.6.0",
"@spectrum-web-components/popover": "1.6.0",
"@spectrum-web-components/progress-circle": "1.6.0",
"@spectrum-web-components/reactive-controllers": "1.6.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't really related to my change, but the precommit hook was failing the linter without this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the extraneous dependency check found this. This is cool!

@tjgupta tjgupta force-pushed the tjgupta/fix-combobox-perf branch from 029dc01 to 2f6ca61 Compare June 12, 2025 20:41
@rubencarvalho rubencarvalho force-pushed the tjgupta/fix-combobox-perf branch from f3c9ef8 to 2f6ca61 Compare June 12, 2025 22:07
@github-actions
Copy link
Contributor

Branch preview

Review the following VRT differences

When a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:

If the changes are expected, update the current_golden_images_cache hash in the circleci config to accept the new images. Instructions are included in that file.
If the changes are unexpected, you can investigate the cause of the differences and update the code accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 12, 2025

Tachometer results

Chrome

combobox permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 781 kB 37.92ms - 38.46ms - slower ❌
34% - 39%
9.69ms - 10.74ms
branch 754 kB 27.53ms - 28.42ms faster ✔
25% - 28%
9.69ms - 10.74ms
-

light-dom-test permalink

Version Bytes Avg Time vs remote vs branch
npm latest 781 kB 373.51ms - 382.84ms - slower ❌
16% - 20%
52.10ms - 63.65ms
branch 754 kB 316.90ms - 323.69ms faster ✔
14% - 17%
52.10ms - 63.65ms
-
Firefox

combobox permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 781 kB 62.59ms - 64.21ms - slower ❌
31% - 36%
14.83ms - 16.93ms
branch 754 kB 46.85ms - 48.19ms faster ✔
24% - 26%
14.83ms - 16.93ms
-

light-dom-test permalink

Version Bytes Avg Time vs remote vs branch
npm latest 781 kB 784.59ms - 802.33ms - slower ❌
5% - 8%
38.02ms - 60.42ms
branch 754 kB 737.39ms - 751.09ms faster ✔
5% - 8%
38.02ms - 60.42ms
-

@rubencarvalho rubencarvalho force-pushed the tjgupta/fix-combobox-perf branch from b4e7408 to e29da59 Compare June 12, 2025 22:15
@tjgupta tjgupta marked this pull request as ready for review June 13, 2025 14:44
@tjgupta tjgupta requested a review from a team as a code owner June 13, 2025 14:44
Copy link
Contributor

@Rajdeepc Rajdeepc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for doing this.

@Rajdeepc Rajdeepc changed the title perf(sp-combobox): remove layout thrash in render fix(combobox): remove layout thrash in render Jun 16, 2025
Copy link
Contributor

@rubencarvalho rubencarvalho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this contribution! 👍

@Rajdeepc Rajdeepc merged commit cc6e91e into main Jun 18, 2025
22 of 23 checks passed
@Rajdeepc Rajdeepc deleted the tjgupta/fix-combobox-perf branch June 18, 2025 08:46
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.

[Bug]: Combobox causes layout thrash during render()

4 participants