Conversation
… for select options
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ShaneK
left a comment
There was a problem hiding this comment.
Looking really good! Fairly minor change requests with one intention verification request
| * form/navigation-hijack attributes (`formaction`, `action`, `target`), | ||
| * which are therefore stripped. | ||
| */ | ||
| const allowedAttributes = [ |
There was a problem hiding this comment.
This allowedAttributes array is shared with sanitizeDOMString, so growing it from 6 entries to ~70 (plus aria-*/data-* wholesale) changes the sanitization output for every existing consumer, not just the new select path. ion-toast, ion-loading, the ion-alert message, ion-refresher-content, and ion-infinite-scroll-content all run innerHTML through this when innerHTMLTemplatesEnabled is on, so names like type, value, width, height, mode, and theme now survive where they were stripped before. The dangerous-vector handling is actually tighter than before so I don't think this is unsafe, but is the wider allowlist intended for those other components too? Worth a regression test covering one of them, and a line in the PR description so anyone bisecting a future toast/loading rendering difference knows where to look.
| const Tag = useSpan ? 'span' : 'div'; | ||
| const keyPrefix = `${className}-${id}`; | ||
|
|
||
| sanitizeDOMTree(content); |
There was a problem hiding this comment.
renderClonedContent calls sanitizeDOMTree(content) here, and the new "should sanitize an HTMLElement label that bypassed ion-select" spec shows this is the only sanitization pass for vanilla-JS callers that hand an HTMLElement straight to renderOptionLabel without going through getOptionContent. But the cloneToVNode comment just above says "pure structural conversion, no security filtering" and "Highly recommended to pre-sanitize the source DOM", which reads like sanitization always happens upstream. Someone could delete this call as redundant and reopen an XSS hole on the direct-HTMLElement path. Could you add a short note above it explaining that this is the only sanitize pass for direct HTMLElement callers and shouldn't be removed?
| @@ -23,6 +30,43 @@ export class SelectOption implements ComponentInterface { | |||
| */ | |||
| @Prop() value?: any | null; | |||
There was a problem hiding this comment.
The disabled JSDoc just above (This property does not apply when interface="action-sheet" as ion-action-sheet does not allow for disabled buttons.) is now stale. This PR wires option.disabled through to the action-sheet button: createActionSheetButtons sets it, the button renders disabled={b.disabled}, there's new disabled styling in action-sheet.scss, and an e2e test covers it. The comment says the opposite of what ships now. Can you update it?
Issue number: resolves ionic-team#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 ionic-team#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>
Issue number: resolves #29890
What is the current behavior?
Select only allows plain text to be passed within it's options.
What is the new behavior?
ion-select-optioninnerHTMLTemplatesEnabledistruestartandendnamed slots for content that appears in the overlay interface but not in the selected textdescriptionprop for text rendered below the option label in the overlaylabelPlacementandjustifyprops toion-select-optionand pass them through to alert, popover, and modal overlay pathsion-selectstart/endslot contentaria-labelderives plain text only from the default slot, ensuring screen readers don't read slotted or element content--select-text-media-width,--select-text-media-height, etc.)Overlay interfaces (
alert,action-sheet,select-popover,select-modal)start,end,description, HTML label) consistently via the sharedrenderOptionLabelutilityActionSheetButton,AlertInput,SelectPopoverOption,SelectModalOption) are unchanged — rich content fields are on internal extended interfaces (SelectActionSheetButton,SelectAlertInput,SelectOverlayOption)sanitizeDOMStringbefore DOM injection to prevent XSSUtilities
select-option-render.tsx: shared render utility for option labels across all overlay componentsgetOptionContent: extracts and clones slot content fromion-select-optionfor overlays and selected textgetDefaultSlotPlainText— extracts plain text from the default slot, used for labels and ariareflectPropertiesToAttributestocore/src/utils/sanitization/and call it fromgetOptionContentimmediately beforecloneNode. The helper mirrors a registered set of custom-element DOM properties (icon,name,src,ios,mdonion-icon) onto attributes so the cloned overlay copy renders correctly regardless of how the framework bound the prop.Tests
Does this introduce a breaking change?
No, developers will be able to continue using plain text for select options as usual.
Other information
Dev build:
8.8.12-dev.11782342444.186b6e9cPreview