Skip to content

Commit 7879ec5

Browse files
ptmkennyShaneK
andauthored
fix(select): derive option text from all child nodes (#31382)
Issue number: resolves #31381 --------- <!-- Please do not submit updates to dependencies unless it fixes an issue. --> <!-- Please try to limit your pull request to one type (bugfix, feature, etc). Submit multiple pull requests if needed. --> ## What is the current behavior? <!-- Please describe the current behavior that you are modifying. --> `ion-select` derives the text it displays for the selected option, and that option's contribution to the button's `aria-label`, from the option's child nodes. In v9 that derivation is wrong whenever an option has more than one child node, or has its text wrapped in an element. | `ion-select-option` content | browser renders | v8 | v9 | | --- | --- | --- | --- | | `Star` | `Star` | `Star` | `Star` | | two sibling text nodes, `{'★'}{'Star'}` | `★Star` | `★Star` | `★ Star` | | `A <b>Star</b>` | `A Star` | `A Star` | `A` | | `<b>Star</b>` | `Star` | `Star` | *(empty)* | Two distinct defects: 1. **A space is inserted between adjacent text nodes.** Every framework renders `{icon}{label}` as two sibling text nodes with no whitespace between them, so any option with an emoji, flag, or icon prefix gains a space that is not in the DOM. 2. **Text wrapped in an element is dropped entirely.** An option whose content is `<b>Star</b>`, `<span>Star</span>`, or an i18n component's wrapper element renders as an **empty select** with an empty accessible name, silently. This is the more damaging of the two. Both affect the visible selected text and the `aria-label`, and both propagate to every overlay interface — `createAlertInputs`, `createActionSheetButtons`, and `createOverlaySelectOptions` read the option through the same helper. In the linked reproduction, opening the first select shows an alert radio labelled `★ Star`. There is no error or warning; the text is just wrong. ## What is the new behavior? <!-- Please describe the behavior or changes that are being added by this PR. --> The selected text and `aria-label` should match what the browser renders for the option's content, which is what v8 produced via `textContent`: `★Star` for two adjacent text nodes, and `A Star` for `A <b>Star</b>`. - - - ## Does this introduce a breaking change? - [ ] Yes - [X] No (It fixes an unreported breaking change made in #31241) <!-- If this introduces a breaking change: 1. Describe the impact and migration path for existing applications below. 2. Update the BREAKING.md file with the breaking change. 3. Add "BREAKING CHANGE: [...]" to the commit description when merging. See https://github.com/ionic-team/ionic-framework/blob/main/docs/CONTRIBUTING.md#footer for more information. --> ## Other information <!-- Any other information that is important to this PR such as screenshots of how the component looks before and after the change. --> --------- Co-authored-by: Shane <shane@shanessite.net>
1 parent 51a2694 commit 7879ec5

3 files changed

Lines changed: 318 additions & 42 deletions

File tree

‎core/src/components/select/select.tsx‎

Lines changed: 51 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ import { printIonWarning } from '@utils/logging';
1515
import { actionSheetController, alertController, popoverController, modalController } from '@utils/overlays';
1616
import type { OverlaySelect } from '@utils/overlays-interface';
1717
import { isRTL } from '@utils/rtl';
18-
import { reflectPropertiesToAttributes, sanitizeDOMTree } from '@utils/sanitization';
18+
import { blockedTags, reflectPropertiesToAttributes, sanitizeDOMTree } from '@utils/sanitization';
1919
import { createSlotMutationController } from '@utils/slot-mutation-controller';
2020
import type { SlotMutationController } from '@utils/slot-mutation-controller';
2121
import { createColorClasses, hostContext } from '@utils/theme';
@@ -1481,33 +1481,14 @@ const textForValue = (
14811481
}
14821482

14831483
/**
1484-
* When custom HTML is enabled, extract only the default slot content.
1485-
* This ensures aria-label and other text-only contexts read only
1486-
* the relevant option text.
1484+
* Every text-only context reads only the default slot, so the start
1485+
* and end slots stay out of the `aria-label` and the overlay labels.
1486+
* Both config paths derive that text through the same helper, so they
1487+
* cannot disagree about what an option's text is. `null` marks an
1488+
* option with no text, which is dropped from the joined text of a
1489+
* `multiple` select rather than joined in as an empty entry.
14871490
*/
1488-
if (customHTMLEnabled) {
1489-
const content = getOptionContent(selectOpt);
1490-
1491-
if (typeof content === 'string') {
1492-
return content;
1493-
}
1494-
1495-
/**
1496-
* Elements were found in the default slot, extract and concatenate
1497-
* their text content while trimming whitespace.
1498-
*/
1499-
if (content) {
1500-
const texts = Array.from(content.childNodes)
1501-
.map((n) => n.textContent?.trim())
1502-
.filter((t) => t);
1503-
return texts.join(' ') || null;
1504-
}
1505-
1506-
// Empty option
1507-
return null;
1508-
}
1509-
1510-
return getDefaultSlotPlainText(selectOpt);
1491+
return getDefaultSlotPlainText(selectOpt) || null;
15111492
};
15121493

15131494
/**
@@ -1570,9 +1551,13 @@ const getOptionContent = (
15701551
return null;
15711552
}
15721553

1573-
// Return plain text if no elements are found
1554+
/**
1555+
* Return plain text if no elements are found. This reads the option the
1556+
* same way the non-custom-HTML path does, so the two do not disagree
1557+
* about what an option's text is.
1558+
*/
15741559
if (!slotName && nodes.every((n) => n.nodeType === Node.TEXT_NODE)) {
1575-
return nodes.map((n) => n.textContent?.trim()).join(' ') || null;
1560+
return getDefaultSlotPlainText(option) || null;
15761561
}
15771562

15781563
/**
@@ -1636,22 +1621,47 @@ const getOptionDefaultSlot = (option: HTMLIonSelectOptionElement): Node[] | null
16361621
return defaultSlotNodes;
16371622
};
16381623

1624+
/**
1625+
* Concatenates the text a node renders, skipping the subtrees of tags
1626+
* whose contents the browser never paints (`script`, `style`, and the
1627+
* rest of `blockedTags`). `textContent` includes those, so reading it
1628+
* directly would put stylesheet or script source into the select text
1629+
* and the `aria-label`.
1630+
*
1631+
* @param node - The node to read text from.
1632+
* @returns The node's rendered text.
1633+
*/
1634+
const getRenderedTextContent = (node: Node): string => {
1635+
if (node.nodeType === Node.TEXT_NODE) {
1636+
return node.textContent ?? '';
1637+
}
1638+
1639+
if (node.nodeType !== Node.ELEMENT_NODE) {
1640+
return '';
1641+
}
1642+
1643+
if (blockedTags.includes((node as Element).tagName.toLowerCase())) {
1644+
return '';
1645+
}
1646+
1647+
return Array.from(node.childNodes)
1648+
.map((child) => getRenderedTextContent(child))
1649+
.join('');
1650+
};
1651+
16391652
/**
16401653
* Extracts plain text from only the default slot of an option,
1641-
* excluding content assigned to named slots (start/end).
1654+
* excluding content assigned to named slots (start/end). Text is
1655+
* concatenated with no separator and collapsible whitespace is
1656+
* collapsed, approximating how the browser renders the option.
1657+
* NBSP is not collapsible, so it is preserved.
1658+
*
1659+
* @param option - The `ion-select-option` element to read text from.
1660+
* @returns The option's default slot text.
16421661
*/
16431662
const getDefaultSlotPlainText = (option: HTMLIonSelectOptionElement): string => {
1644-
const texts = Array.from(option.childNodes)
1645-
.filter((node) => {
1646-
if (node.nodeType === Node.ELEMENT_NODE) {
1647-
return !(node as HTMLElement).hasAttribute('slot');
1648-
}
1649-
return node.nodeType === Node.TEXT_NODE;
1650-
})
1651-
.filter((node) => node.nodeType === Node.TEXT_NODE)
1652-
.map((n) => n.textContent?.trim())
1653-
.filter((t) => t);
1654-
return texts.join(' ');
1663+
const text = (getOptionDefaultSlot(option) ?? []).map((node) => getRenderedTextContent(node)).join('');
1664+
return text.replace(/[ \t\n\r\f]+/g, ' ').replace(/^[ \t\n\r\f]+|[ \t\n\r\f]+$/g, '');
16551665
};
16561666

16571667
/**

‎core/src/components/select/test/rich-content-option/select.e2e.ts‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -430,7 +430,7 @@ configs({ modes: ['md'] }).forEach(({ title, config }) => {
430430
*/
431431
configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => {
432432
test.describe(title('select: rich content options'), () => {
433-
test('it should only render text nodes when `innerHTMLTemplatesEnabled` is disabled', async ({ page }) => {
433+
test('should not render markup when `innerHTMLTemplatesEnabled` is disabled', async ({ page }) => {
434434
await page.setContent(
435435
`
436436
<ion-select id="alert-select" label="Alert" placeholder="Select one" interface="alert">
@@ -466,6 +466,12 @@ configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => {
466466
await expect(endContainer).toHaveCount(0);
467467
await expect(span).toHaveCount(0);
468468

469+
/**
470+
* The span is not rendered, but the text it wrapped still reads as
471+
* text, so the option is not silently emptied out.
472+
*/
473+
await expect(firstOption).toContainText('Full Content This is a span element');
474+
469475
// Click on the first option
470476
await firstOption.click();
471477

@@ -479,6 +485,12 @@ configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => {
479485
const selectTextSpan = selectText.locator('.span-style');
480486

481487
await expect(selectTextSpan).toHaveCount(0);
488+
489+
/**
490+
* Only the default slot is read, so the text of the `start` and `end`
491+
* slots stays out of the selected text.
492+
*/
493+
await expect(selectText).toHaveText('Full Content This is a span element');
482494
});
483495
});
484496
});

0 commit comments

Comments
 (0)