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 #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