-
Notifications
You must be signed in to change notification settings - Fork 256
fix(combobox, breadcrumbs): propagate lang/dir for a single item's language without breaking layout #6496
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
base: main
Are you sure you want to change the base?
fix(combobox, breadcrumbs): propagate lang/dir for a single item's language without breaking layout #6496
Changes from all commits
0c29a6d
5089709
8bc14ff
5cfc294
40b5aa7
4bab8d6
dab9096
25aaefd
8704c0a
ce65713
c8cc576
18cddae
db8bdb7
d7c30ef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| --- | ||
| '@spectrum-web-components/combobox': patch | ||
| '@spectrum-web-components/breadcrumbs': patch | ||
| '@spectrum-web-components/base': patch | ||
| '@spectrum-web-components/reactive-controllers': patch | ||
| --- | ||
|
|
||
| **Fixed**: Propagate `lang`/`dir` for a single item's language without breaking layout: | ||
|
|
||
| - Combobox: forwards `lang`/`dir` from slotted `<sp-menu-item>` (or `.options` data) onto the rendered popover `<sp-menu-item>`, and syncs the input's own `lang` to the committed option's language for correct pronunciation | ||
| - Breadcrumbs: forwards the same `lang`/`dir` propagation to the "More items" overflow menu | ||
| - `BreadcrumbItem` now forwards `lang`/`dir` to `#item-link` only, so a single item's language does not flip its own layout or mirror its separator's chevron; the separator tracks the ambient direction (nearest ancestor `dir`, or the document default) instead of the host's own `dir` attribute, including live updates when an ancestor's `dir` changes after mount | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,7 @@ parameters: | |
| # 3. Commit this change to the PR branch where the changes exist. | ||
| current_golden_images_hash: | ||
| type: string | ||
| default: 75b8c3cdc71a773889e3bdd1032f89ff73c12475 | ||
| default: 269f7b7535187364b8879232b07e2d7bfba7e02f | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please remove this hash. Once you get two approvals we generally encourage authors to update this hash |
||
|
|
||
| commands: | ||
| halt-if-not-affected: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,9 @@ import { | |
| } from '@spectrum-web-components/base'; | ||
| import { property } from '@spectrum-web-components/base/src/decorators.js'; | ||
| import { ifDefined } from '@spectrum-web-components/base/src/directives.js'; | ||
| import { normalizeDir } from '@spectrum-web-components/base/src/normalize-dir.js'; | ||
| import chevronStyles from '@spectrum-web-components/icon/src/spectrum-icon-chevron.css.js'; | ||
| import { observeAttribute } from '@spectrum-web-components/reactive-controllers/src/AttributeObserver.js'; | ||
| import { Focusable } from '@spectrum-web-components/shared/src/focusable.js'; | ||
| import { LikeAnchor } from '@spectrum-web-components/shared/src/like-anchor.js'; | ||
|
|
||
|
|
@@ -30,6 +32,30 @@ export interface BreadcrumbSelectDetail { | |
| value: string; | ||
| } | ||
|
|
||
| // Walks up through `parentElement`, crossing shadow-root boundaries via | ||
| // `getRootNode().host` — needed because `Breadcrumbs.renderMenu()` places | ||
| // an "is-menu" `BreadcrumbItem` inside its own shadow root rather than as | ||
| // a light-DOM child, so a plain `parentElement` walk would stop there | ||
| // without ever reaching `<sp-breadcrumbs>` itself. | ||
| function* ancestorElements(start: Element): Generator<Element> { | ||
|
majornista marked this conversation as resolved.
|
||
| let node: Node = start; | ||
| for (;;) { | ||
| const parent = (node as Element).parentElement; | ||
| if (parent) { | ||
| yield parent; | ||
| node = parent; | ||
| continue; | ||
| } | ||
| const root = node.getRootNode(); | ||
| if (root instanceof ShadowRoot) { | ||
| yield root.host; | ||
| node = root.host; | ||
| continue; | ||
| } | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| export class BreadcrumbItem extends LikeAnchor(Focusable) { | ||
| public static override get styles(): CSSResultArray { | ||
| return [styles, chevronStyles]; | ||
|
|
@@ -49,12 +75,49 @@ export class BreadcrumbItem extends LikeAnchor(Focusable) { | |
| return this.shadowRoot.querySelector('#item-link') as HTMLElement; | ||
| } | ||
|
|
||
| // `renderLink()` and `renderSeparator()` bake this host's own `lang`/`dir` | ||
| // and the ambient `direction` into explicit attributes at render time. | ||
| // Neither `lang` nor `dir` is a reactive Lit property here, so — unlike | ||
| // plain CSS inheritance — none of that updates on its own if this host's | ||
| // own `lang`/`dir` change, or an ancestor's `dir` changes, after mount. | ||
| public static override get observedAttributes(): string[] { | ||
| return [...super.observedAttributes, 'dir', 'lang']; | ||
| } | ||
|
|
||
| public override attributeChangedCallback( | ||
| name: string, | ||
| old: string | null, | ||
| value: string | null | ||
| ): void { | ||
| super.attributeChangedCallback(name, old, value); | ||
| if (name === 'dir' || name === 'lang') { | ||
| this.requestUpdate(); | ||
| } | ||
| } | ||
|
|
||
| // `attributeChangedCallback` only reaches this host's own attributes, not | ||
| // an ancestor's `dir` — react to those via the shared `AttributeObserver` | ||
| // singleton instead of a `MutationObserver` per item. | ||
| private ancestorDirUnsubscribes: (() => void)[] = []; | ||
|
|
||
| override connectedCallback(): void { | ||
| super.connectedCallback(); | ||
|
|
||
| if (!this.hasAttribute('role')) { | ||
| this.setAttribute('role', 'listitem'); | ||
| } | ||
|
|
||
| for (const ancestor of ancestorElements(this)) { | ||
| this.ancestorDirUnsubscribes.push( | ||
| observeAttribute(ancestor, 'dir', () => this.requestUpdate()) | ||
| ); | ||
| } | ||
|
majornista marked this conversation as resolved.
|
||
| } | ||
|
|
||
| override disconnectedCallback(): void { | ||
| this.ancestorDirUnsubscribes.forEach((unsubscribe) => unsubscribe()); | ||
| this.ancestorDirUnsubscribes = []; | ||
| super.disconnectedCallback(); | ||
| } | ||
|
|
||
| private announceSelected(value: string): void { | ||
|
|
@@ -90,10 +153,19 @@ export class BreadcrumbItem extends LikeAnchor(Focusable) { | |
| } | ||
|
|
||
| protected renderLink(): TemplateResult { | ||
| // Forward `lang`/`dir` from the host onto the link so a single item's | ||
| // language only affects its own text rendering, not the host's own | ||
| // `direction` (which the separator mirrors via `:dir(rtl)`); see | ||
| // `:host([dir]) { direction: inherit; }` in breadcrumb-item.css. Read the raw | ||
| // `dir` attribute rather than `this.dir` — `SpectrumElement` overrides | ||
| // the native `dir` getter to return the *computed* CSS direction, which | ||
| // is exactly what `direction: inherit` decouples from the attribute. | ||
| return html` | ||
| <a | ||
| id="item-link" | ||
| href=${ifDefined(!this.isLastOfType ? this.href : undefined)} | ||
| lang=${ifDefined(this.lang || undefined)} | ||
| dir=${ifDefined(normalizeDir(this.getAttribute('dir')))} | ||
| tabindex="0" | ||
| aria-current=${ifDefined(this.isLastOfType ? 'page' : undefined)} | ||
| @keydown=${this.handleKeyDown} | ||
|
|
@@ -105,11 +177,20 @@ export class BreadcrumbItem extends LikeAnchor(Focusable) { | |
| } | ||
|
|
||
| private renderSeparator(): TemplateResult { | ||
| // `:dir()` resolves directionality by walking up the `dir` *attribute* | ||
| // chain, independent of the CSS `direction` property — so it still | ||
| // picks up this host's own `dir` (set for `#item-link`'s content, per | ||
| // `renderLink()`) even though `:host([dir]) { direction: inherit; }` keeps the | ||
| // host's own *computed* direction tied to the ambient context. Read that | ||
| // computed value and set it explicitly here so `:dir()` on `#separator` | ||
| // resolves from its own accurate attribute instead of the host's. | ||
| const ambientDir = getComputedStyle(this).direction as 'ltr' | 'rtl'; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: |
||
| return html` | ||
| <sp-icon-chevron100 | ||
| id="separator" | ||
| size="xs" | ||
| class="spectrum-UIIcon-ChevronRight100" | ||
| dir=${ambientDir} | ||
| ></sp-icon-chevron100> | ||
| `; | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.