Skip to content

fix(pds-select): keep the inner select in step with changes to existing options - #917

Merged
QuintonJason merged 12 commits into
mainfrom
fix/select-option-sync
Oct 28, 2026
Merged

QuintonJason merged 12 commits into
mainfrom
fix/select-option-sync

Conversation

@QuintonJason

@QuintonJason QuintonJason commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Description

pds-select copies its slotted <option>/<optgroup> elements into its inner native <select>, and only re-copied on slotchange. That event fires only when top-level options are added or removed. So an existing option's text, value or disabled changing (a framework re-render, async label text), or an option added inside an <optgroup>, never reached the select. The dropdown could disagree with the markup the consumer rendered.

Linear: DSS-422. Sibling of DSS-420 / #915 (pds-tooltip showing a frozen copy of content that changes below the top level).

Fix

  • A MutationObserver on the host's light DOM (childList, subtree, attributes, characterData) schedules one copy per animation frame. It reacts only to mutations inside an option or optgroup: the host's own attributes and children (top-level adds and removes still go through slotchange) and the action slot's content are ignored.
  • handleSlotChange stays the single copy path:
    • Unchanged: when the copies match the select's current options, nothing happens.
    • Same shape (same tags in the same places, optgroup contents included): the select's own options are patched in place (attributes and text). An open picker keeps its highlight, a multiple select its range anchor, and a screen reader its place.
    • Shape changed: the options are replaced, as before.
  • After a copy, updateSelectedOption reselects the current value. If no option has that value any more (say it was renamed), nothing is shown selected (selectedIndex = -1, as a native select does) and value is kept: no silent change, no pdsSelectChange. The form value and validity are recomputed after any copy that changed something.
  • The observer starts in componentDidLoad and only for a connected element. On reconnect it restarts and schedules a catch-up copy. On disconnect it stops and cancels a pending frame.
  • The redundant single-value pre-select in the copy loop is gone; updateSelectedOption owns selection.

Review: a 3-reviewer Pine gauntlet (code, security, design) ran on this branch: 0 blockers. All SHOULD FIX and most CONSIDER findings applied (in-place patching, unmatched values, reconnect catch-up, a narrower observer, the isConnected guard, tighter tests, docs). A human pass from pixelflips found and fixed a first-render regression (the no-match blank state is now scoped to options changing under an already-matched value), a characterData-on-a-comment-node crash in the observer filter, a componentDidLoad/first-slotchange race (closed the same way the reconnect path already does), a docs overclaim, and tightened two tests.

Trade-offs

  • Bundle size: the "All CDN component chunks" budget is raised to 362 KB. It measures 361.02 kB gzipped against a 360 kB limit; main measures 359.74 kB, so 0.26 kB of headroom. This change adds about 1.3 kB to that measurement: the check globs p-*.entry.js, which counts the select twice (the ESM chunk and the SystemJS *.system.entry.js chunk). After trimming, the code is already compact. The budget is raised to 362 KB here and in fix(pds-tooltip): keep an open tooltip in step with its slotted content #915 (signed off), sized for both PRs, so each passes on its own. Whichever merges second will conflict on that one line in .size-limit.json; re-check the measured size then and readjust the budget if needed.
  • Top-level adds and removes still go through slotchange; the observer ignores the host's own children so they aren't copied twice.
  • "Nothing selected when no option matches" applies after a copy only. Setting value directly to something with no option behaves as before.

Fixes DSS-422

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • This change requires a documentation update

How Has This Been Tested?

  • unit tests: pds-select.spec.tsx 51/51 (13 new: watches at any depth; text, value and disabled patched in place with the same node; optgroup add rebuilds; value kept selected; renamed selected value → selectedIndex -1, value kept, no event; the first copy still falls back to the first option when the value matches nothing, like a native select; calls updateFormValue once per changing copy, not on every mutation; one copy per frame; unchanged copies leave nodes alone; host attributes/children and the action slot ignored (including a characterData mutation on a comment node, which crashed before); catch-up after reconnect; observer disconnected and pending frame cancelled on removal).
  • e2e tests: pds-select.e2e.ts 14/14. The existing-option-text-change case times out against main's component and passes with this fix. New: a required select with its selected option renamed away matches :invalid — the one case the unit test can't cover, since updateFormValue is a no-op under isSpecTest().
  • accessibility tests
  • tested manually
  • other: nx run @pine-ds/core:build regenerates nothing (no API change); ESLint 0 errors (the 5 warnings are pre-existing lines); size-limit as above.

Manual a11y check to do before merge (not done yet):

  1. Open the picker, then change a visible option's text from the console: the highlighted option and the open list should stay put.
  2. In a multiple select, Shift+click a range, change an option's text, then Shift+click again: the range should extend from the same anchor.
  3. VoiceOver (Safari) and NVDA (Firefox): focus the select, arrow to an option, change another option's text: focus and the reading position shouldn't jump; then rename the selected option's value and check what's announced.

Test Configuration:

  • Pine versions: main @ 4.8.0
  • OS: macOS
  • Browsers: Chromium (Stencil e2e)
  • Screen readers: none yet (see manual check)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing tests pass locally with my changes
  • Design has QA'ed and approved this PR

Note

Medium Risk
Touches form-associated selection, validity, and DOM sync on every option mutation; behavior is well-tested but affects core select UX and validation edge cases.

Overview
Fixes pds-select showing a stale dropdown when consumers update slotted <option> / <optgroup> markup without adding or removing top-level options (framework re-renders, async labels, nested optgroup changes)—cases slotchange alone never handled.

A MutationObserver on the host light DOM watches option/optgroup mutations (batched per animation frame) and reuses handleSlotChange as the single sync path: skip when markup is unchanged, patch attributes/text in place when structure matches (preserving open-picker highlight and screen-reader position), or rebuild when shape changes. After updates, selection follows the current value; if no option matches anymore, the UI shows nothing selected (selectedIndex = -1) while keeping value and not firing pdsSelectChange, with form validity refreshed. Observer lifecycle covers load, reconnect catch-up, and disconnect cleanup.

Docs add an “Options stay in sync” usage note; new unit and e2e tests cover in-place updates, unmatched values, async reload, and required :invalid when the selected option’s value is renamed away.

Reviewed by Cursor Bugbot for commit 3b7670d. Bugbot is set up for automated code reviews on this repo. Configure here.

@QuintonJason QuintonJason self-assigned this Oct 28, 2026
@netlify

netlify Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for pine-design-system ready!

Name Link
🔨 Latest commit 3b7670d
🔍 Latest deploy log https://app.netlify.com/projects/pine-design-system/deploys/6ae25576bdc16ac38466ee8b
😎 Deploy Preview https://deploy-preview-917--pine-design-system.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@QuintonJason QuintonJason added the ran-gauntlet Multi-agent review gauntlet has been run on this branch label Oct 28, 2026
@github-actions github-actions Bot added bug Something isn't working package: core Changes have been made to the Core package labels Oct 28, 2026
@QuintonJason
QuintonJason marked this pull request as ready for review October 28, 2026 15:28
@QuintonJason
QuintonJason requested a review from a team as a code owner October 28, 2026 15:28

@pixelflips pixelflips left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The in-place patching and reconnect handling look solid. Notes inline: one behavior change on first render, two small gaps in the observer, a docs clause, and two spec nits.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 4450d49. Configure here.

@QuintonJason
QuintonJason merged commit a85559f into main Oct 28, 2026
27 checks passed
@QuintonJason
QuintonJason deleted the fix/select-option-sync branch October 28, 2026 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working package: core Changes have been made to the Core package ran-gauntlet Multi-agent review gauntlet has been run on this branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants