fix(combobox, breadcrumbs): propagate lang/dir for a single item's language without breaking layout - #6496
Open
majornista wants to merge 14 commits into
Open
fix(combobox, breadcrumbs): propagate lang/dir for a single item's language without breaking layout#6496majornista wants to merge 14 commits into
majornista wants to merge 14 commits into
Conversation
majornista
added
bug
🦋 Changeset detectedLatest commit: 0830b1b The changes in this PR will be included in the next version bump. This PR includes changesets to release 83 packages
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 |
Contributor
📚 Branch Preview Links🔍 Gen1 Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
Collaborator
Coverage Report for CI Build 30754734209Warning No base build found for commit Coverage: 96.263%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
This PR adds “language of parts” support to 1st-gen sp-combobox and sp-breadcrumbs by propagating per-item lang/dir into internally-rendered menu items, and by scoping sp-breadcrumb-item’s lang/dir so a single item’s language doesn’t flip breadcrumb layout/separators.
Changes:
- Combobox: propagate option
lang/dirinto rendered popover items and sync the input’slangto the committed selection. - Breadcrumbs: propagate item
lang/dirinto overflow menu items; adjustsp-breadcrumb-itemso separators mirror based on ambient direction rather than the item’s owndir. - Add targeted tests and Storybook stories demonstrating mixed
lang/dirlists, plus update 1st-gen version typings.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| 1st-gen/tools/base/src/version.d.ts | Updates declared versions to match implementation. |
| 1st-gen/packages/combobox/test/combobox-a11y.test.ts | Adds coverage for lang/dir propagation and input lang syncing. |
| 1st-gen/packages/combobox/stories/combobox.stories.ts | Adds languageOfParts story demonstrating per-item language/direction. |
| 1st-gen/packages/combobox/src/Combobox.ts | Implements option lang/dir forwarding and input lang binding. |
| 1st-gen/packages/breadcrumbs/test/breadcrumbs.test.ts | Adds overflow menu propagation tests and ancestor dir-change regression test. |
| 1st-gen/packages/breadcrumbs/test/breadcrumb-item.test.ts | Adds tests ensuring per-item dir doesn’t break layout and separators track ambient direction. |
| 1st-gen/packages/breadcrumbs/stories/breadcrumbs.stories.ts | Adds LanguageOfParts story for overflow menu propagation. |
| 1st-gen/packages/breadcrumbs/src/Breadcrumbs.ts | Captures per-item lang/dir and forwards into overflow menu items. |
| 1st-gen/packages/breadcrumbs/src/BreadcrumbItem.ts | Scopes lang/dir onto #item-link and computes separator direction from ambient context with ancestor observation. |
| 1st-gen/packages/breadcrumbs/src/breadcrumb-item.css | Ensures host layout direction inherits ambient direction even when host has dir. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
majornista
force-pushed
the
mijordan/SWC-2359-combobox-lang-of-parts
branch
from
July 10, 2026 22:02
90ccb0c to
a77171e
Compare
majornista
force-pushed
the
mijordan/SWC-2359-combobox-lang-of-parts
branch
from
July 10, 2026 22:07
0377af4 to
3c7d781
Compare
majornista
force-pushed
the
mijordan/SWC-2359-combobox-lang-of-parts
branch
from
July 10, 2026 22:28
3c7d781 to
ff9409d
Compare
majornista
force-pushed
the
mijordan/SWC-2359-combobox-lang-of-parts
branch
from
July 10, 2026 22:30
c017bce to
0e255f5
Compare
Contributor
|
@majornista can you revert the golden_hash token so that we can review the VRT failures as part of our review? that is usually the last commit after getting two approvals. |
majornista
force-pushed
the
mijordan/SWC-2359-combobox-lang-of-parts
branch
2 times, most recently
from
July 15, 2026 13:33
2958240 to
a2a4405
Compare
Rajdeepc
previously requested changes
Jul 20, 2026
Rajdeepc
left a comment
Rajdeepc
left a comment
Contributor
There was a problem hiding this comment.
Reviewed from an architecture/ownership lens (public API, consistency with existing patterns, a11y, release risk). Left 3 inline comments — two are blocking asks around reusing an existing pattern instead of introducing a new one, one is a question about scope. Core diagnosis of the underlying bug (SpectrumElement's dir getter returning computed style, :dir() resolving via the attribute chain) is correct and well tested; not re-litigating that part.
Rajdeepc
reviewed
Jul 21, 2026
Rajdeepc
left a comment
Rajdeepc
left a comment
Contributor
There was a problem hiding this comment.
Re-reviewed after the latest commits — the shared-observer refactor, the native attributeChangedCallback switch, and the input dir fix are all in and look good (the dir="auto" approach for uncommitted RTL search text is better than what I'd suggested). One new thing surfaced by the button-clearance follow-up fix, left inline below.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 20 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
1st-gen/tools/reactive-controllers/test/attribute-observer.test.ts:21
- The helper comment says it flushes MutationObserver callbacks “by yielding a microtask”, but the implementation yields a macrotask via
setTimeout(). Either update the comment or switch the helper to an actual microtask flush so the intent matches the code.
/**
* Flush MutationObserver callbacks by yielding a microtask.
*/
const flushObserver = (): Promise<void> =>
new Promise((resolve) => setTimeout(resolve, 0));
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
1st-gen/tools/reactive-controllers/src/AttributeObserver.ts:89
reconnect()drainssharedObserver.takeRecords()after the new listener is added to the registry. If there are already-queued mutation records for the same (target, attribute) (attribute changed just beforeobserveAttribute()but the observer callback hasn’t fired yet), those pre-subscription records will be delivered to the newly registered listener, causing spurious updates.
Drain queued records before adding the listener so only existing subscribers see earlier mutations.
listeners.add(listener);
reconnect();
Contributor
Author
|
@caseyisonit, @Rajdeepc, @rise-erpelding this PR still needs review or approval. |
…nguage without breaking layout
Previously, setting `lang`/`dir` on a slotted `<sp-menu-item>` (Combobox)
or `<sp-breadcrumb-item>` (Breadcrumbs) had no effect on the rendered
popover/overflow-menu counterpart, since both components synthesize new
elements from extracted data rather than reusing the originals.
- Combobox: forwards `lang`/`dir` from slotted items (or `.options` data)
onto the rendered popover `<sp-menu-item>`, and now syncs the input's
own `lang` to the committed option's language for correct pronunciation.
- Breadcrumbs: same propagation for the "More items" overflow menu.
- BreadcrumbItem: forwards `lang`/`dir` to `#item-link` only, so a single
item's language does not flip its own layout or its separator's
mirrored chevron. The separator now explicitly tracks the *ambient*
direction (nearest ancestor `dir`, or the document default) instead of
inheriting the host's own `dir` attribute via `:dir()`, including live
updates when an ancestor's `dir` changes after mount, and across the
shadow-root boundary for the overflow-menu wrapper item.
Along the way, worked around two non-obvious base-class behaviors that
caused regressions mid-fix: `SpectrumElement` overrides `dir` to return
computed CSS direction rather than the attribute (must read
`getAttribute('dir')` for authored values), and CSS `:dir()` resolves
directionality via the attribute chain, independent of the `direction`
property.
Adds Storybook stories (`languageOfParts` / `LanguageOfParts`) and test
coverage for each fix, each verified to fail without its corresponding
code change.
Jira: SWC-2359
fix: refactor `dir` attribute normalization into a utility method
fix(combobox): use correct type shape
Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-Authored-By: Michael Jordan <michael@majordan.net>
PR adds new storybook stories, so we need a new golden_images_hash
This reverts commit b4394d3.
- BreadcrumbItem allocated one MutationObserver per instance to watch every ancestor for `dir` changes plus itself for `dir`/`lang`; a deeply nested breadcrumb trail meant O(items x ancestor depth) observer registrations, all re-watching the same shared ancestors - add `observeAttribute()` to reactive-controllers: a single shared MutationObserver keyed by (target, attribute), for watching elements a component doesn't own (ancestors, `document.documentElement`) - route LanguageResolutionController's `addLangListener` through the same primitive instead of its own bespoke single-target observer, so there's one idiom for this pattern instead of two - BreadcrumbItem now only uses the shared observer for ancestor `dir`; its own `dir`/`lang` changes go through the native `observedAttributes`/`attributeChangedCallback` lifecycle hooks (see Tooltip.ts), which need no observer object at all Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- calculateBreadcrumbItemsWidth() only re-snapshots each item's lang/dir into the cached `items` array when items are added/removed or the layout recalculates (maxVisibleItems/compact change); renderMenu() always renders the overflow menu's <sp-menu-item> copies from that cache - since BreadcrumbItem now reacts live to its own post-mount lang/dir changes, the visible breadcrumb could update while its overflow-menu copy stayed stale indefinitely, with no trigger to ever resync it - add watchItemAttributes(), reusing the shared observeAttribute() singleton, to patch just the changed item's cached lang/dir in place whenever the corresponding live item's lang/dir attribute changes - add a regression test covering the overflow-menu resync Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- syncing the input's `lang` to the committed option (added earlier) fixes pronunciation, but left `dir` unsynced, so a genuinely RTL value like Hebrew/Arabic still rendered in a directionally-LTR input with misleading caret placement/text alignment - use the native `dir="auto"` on the input instead of mirroring the selected option's own `dir`: it derives directionality live from the input's actual value via the browser's bidi algorithm, so typed-but -not-yet-committed RTL search text gets correct alignment too, not just a value that has already resolved to a committed option - scoped to just the input element, so it can't affect surrounding combobox layout (icons, popover positioning) - add a test covering both the uncommitted-RTL-text and reverts-to-ltr cases fix(combobox): keep button-clearance padding on its physical side - #input's dir="auto" (added earlier) lets its own computed direction diverge from the host's stable ambient direction whenever the value is RTL script, but .button isn't given dir="auto" and stays anchored to the host's direction alone - #input's button-clearance padding-inline-start/-end are logical properties resolved against #input's own direction, so they flipped out of step with .button's actual (physically fixed) side once an RTL value made #input's direction diverge, letting text render under the button - capture each state's clearance value (default, invalid, pending, quiet, and their combinations) once per spectrum-combobox.css's existing #input rules, then apply them as physical padding-left/ padding-right keyed to the host's own :dir(), so the clearance always matches where .button actually sits regardless of #input's own dir="auto"-driven direction - add a regression test confirming the padding stays anchored to its physical side even after #input's own direction flips Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This test relies on a MutationObserver-driven resync in Breadcrumbs.watchItemAttributes(). MutationObserver callbacks run asynchronously, so await elementUpdated(el) immediately after setAttribute() can race and become flaky (it may resolve before the observer fires and schedules an update). Adding a frame/microtask boundary before awaiting elementUpdated(el) makes the test deterministic. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…ch text" This reverts commit 40c70c8.
- the previous dir="auto" approach let #input's own computed direction diverge from the host's stable ambient direction for RTL values, which required physical padding overrides to keep its button-clearance space from drifting out of step with .button's fixed position - unicode-bidi: plaintext shapes/aligns each bidi paragraph from its own content without changing the input's direction property at all, so it solves the original caret/alignment concern without ever risking that divergence, no dir attribute or padding overrides needed - add a test confirming both that unicode-bidi: plaintext is applied and that direction/padding-left/padding-right all stay unchanged for an rtl value, verifying this approach can't reintroduce the button-collision bug Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…/reactive-controllers This changeset bumps `combobox`/breadcrumbs, but this PR also changes published dependencies (`@spectrum-web-components/base` adds `normalize-dir` exports and `@spectrum-web-components/reactive-controllers` adds `AttributeObserver`). If those packages aren’t released too, the new runtime imports (e.g. `@spectrum-web-components/base/src/normalize-dir.js`, `@spectrum-web-components/reactive-controllers/src/AttributeObserver.js`) can be missing for consumers, causing module-resolution failures. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…onnect - reconnect() called MutationObserver.disconnect() unconditionally whenever a target/attribute was registered or unregistered; per spec, disconnect() also clears any records already queued but not yet delivered to the callback - a mutation that landed right before an unrelated component's mount/unmount triggered reconnect() (disconnect + re-observe) could therefore have its record silently dropped, with no later mutation to generate a replacement, so the listener would never fire for it - factor the record-processing logic used by the MutationObserver's own callback into processRecords(), and call it once more with takeRecords() immediately before disconnect() in reconnect(), so any already-queued records are still delivered - add a dedicated test file covering observeAttribute() directly, including a regression test that queues a mutation and triggers reconnect() before it would otherwise be delivered, confirming the listener still fires Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- flushObserver() uses setTimeout(resolve, 0), which resolves as a macrotask, but its comment described it as yielding a microtask - the mechanism itself is correct (a macrotask is the stronger guarantee here, since it's only scheduled after the microtask queue, including any pending MutationObserver callback, has fully drained), so fix the comment's wording rather than the implementation - applies the same correction to language-resolution.test.ts's identical helper for consistency Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
PR adds new storybook stories, so we need a new golden_images_hash
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds "language of parts" support to
sp-comboboxandsp-breadcrumbs: consumers can setlang/diron an individual slotted item (e.g. a language name rendered in its own script) and have it render correctly, without breaking the surrounding component's layout.<sp-menu-item lang="he" dir="rtl">(or aComboboxOptionwithlang/dir) now propagates those attributes to its rendered counterpart in the popover. The text input itself also adopts thelangof the currently-selected option, so screen readers pronounce the displayed value correctly.lang/dirnow apply only to#item-link(the text), not the host — so one item's language doesn't flip its own layout or its separator's chevron relative to its siblings. The chevron now tracks the ambient direction (nearest ancestor'sdir, or the document default) instead of the item's owndirattribute, and stays correct if an ancestor'sdirchanges after the item has already mounted, or for the overflow-menu wrapper item (which is rendered insideBreadcrumbs' own shadow root rather than as a light-DOM child).Two non-obvious base-class behaviors caused regressions mid-implementation and are now specifically guarded against (with tests that fail without the fix):
SpectrumElementoverrides the nativedirgetter to return computed CSS direction rather than the attribute, so authoreddirvalues must be read viagetAttribute('dir').:dir()resolves directionality via the DOM'sdirattribute chain, entirely independent of thedirectionproperty — sodirection: inheritalone doesn't stop a mismatched attribute from being picked up by descendant:dir()selectors.Motivation and context
Combobox and Breadcrumbs synthesize new elements from extracted data (rather than reusing the original slotted elements) when rendering their popover/overflow menu, so any
lang/dirset on the original item was silently dropped — breaking pronunciation and bidi text shaping for mixed-language lists (e.g. a language picker).Related issue(s)
Screenshots (if appropriate)
N/A — no visual/layout changes to default (single-language) usage. New Storybook stories (
languageOfParts,LanguageOfParts) demonstrate the fix; a screenshot/VRT baseline update may be worth attaching before merge.Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Combobox popover items and input keep an item's own language
languageOfParts<sp-menu-item>in the popover to carry the samelang/diras its source; select the Hebrew or Arabic option and expect the input's ownlangattribute to update to matchBreadcrumbs overflow menu keeps an item's own language
LanguageOfParts(forces the overflow menu viamax-visible-items)<sp-menu-item>to carry the samelang/diras the source<sp-breadcrumb-item>A single item's language doesn't break the breadcrumb trail's layout
LanguageOfPartsstory, locate the Hebrew/Arabic breadcrumb items amid the (LTR) traildir="rtl"on<sp-breadcrumbs>via devtools after the story has renderedDevice review
Accessibility testing checklist
Keyboard (required — document steps below)
languageOfPartslang/dirchangesScreen reader (required — document steps below)
languageOfPartsand arrow through the popover optionsLanguageOfParts, opening the "More items" overflow menu