Skip to content

fix(many): add rich content styles for select options#31138

Open
thetaPC wants to merge 42 commits into
nextfrom
FW-7290
Open

fix(many): add rich content styles for select options#31138
thetaPC wants to merge 42 commits into
nextfrom
FW-7290

Conversation

@thetaPC
Copy link
Copy Markdown
Contributor

@thetaPC thetaPC commented May 12, 2026

Issue number: resolves internal


What is the current behavior?

  • ion-select-option doesn't expose labelPlacement or justify props
  • The ionic theme's overlay rendering doesn't match the latest design
  • <ion-icon> placed inside an ion-select-option renders empty in Vue/React (and other frameworks that bind custom-element props as DOM properties)
  • Slotted children in select-option-start / select-option-end aren't size-capped consistently: oversized content
  • Several theme style files (alert.{ios,md,ionic}.scss, action-sheet.ionic.scss, select-{modal,popover}.{ios,md,ionic}.scss) import the wrong shared partials

What is the new behavior?

  • Add labelPlacement and justify props to ion-select-option and pass them through to alert, popover, and modal overlay paths
  • Update the ionic theme's alert, popover, and modal overlay styles to match the current design
  • Add per-theme slot-size limitations so oversized slotted content is capped to keep option rows visually uniform within an overlay
  • Audit and correct the @use / @import chains in the affected theme stylesheets so each theme file pulls in the right shared partials (mixins, theme globals, common base) and no longer relies on transitive imports that weren't guaranteed
  • Add reflectPropertiesToAttributes to core/src/utils/sanitization/ and call it from getOptionContent immediately before cloneNode. The helper mirrors a registered set of custom-element DOM properties (icon, name, src, ios, md on ion-icon) onto attributes so the cloned overlay copy renders correctly regardless of how the framework bound the prop.
  • Add test pages and Playwright snapshots for the overlays

Does this introduce a breaking change?

  • Yes
  • No

Other information

Dev build: 8.8.9-dev.11779411201.1a483e09

Preview:

@vercel
Copy link
Copy Markdown

vercel Bot commented May 12, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
ionic-framework Ready Ready Preview, Comment May 30, 2026 3:14am

Request Review

* - Chromium and WebKit suppress it because of hasTouch and isMobile.
* - Headless Firefox doesn't detect input devices and reports no hover
* capability regardless of context options, so override it via
* launchOptions.firefoxUserPrefs. Bit values: 4 = FINE (mouse),
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overriding it would lead to a global change so I didn't want to attempt that in this PR.

Comment on lines +212 to +225
.action-sheet-button.ion-focused:not(.ion-activated) {
color: var(--button-color-focused);

&::after {
background: var(--button-background-focused);

opacity: var(--button-background-focused-opacity);
}

&.action-sheet-selected::after {
background: var(--button-background-focused, var(--button-background-selected));

opacity: var(--button-background-focused-opacity, var(--button-background-selected-opacity));
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By restructuring it this way, we are making sure that this style will be just a copy and paste for the Modular Ionic migration.

$action-sheet-ios-button-translucent-filter: saturate(120%);

/// @prop - Maximum size of slotted children rendered in a select option's start/end slot
$action-sheet-ios-select-option-slot-size: dynamic-font-max(24px, 2);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on what I found through iOS code and that it scales up.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a screenshot of how iOS looks?

Here is an example I took from my Contacts app:

CleanShot 2026-05-28 at 18 09 36

It looks closer to 30px.

*/
.select-option-start > *,
.select-option-end > * {
max-height: 24px;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on MD3

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The avatar looks pretty small compared to md3:

Ionic MD3
ionic md3

MD2 is also a bit bigger:

Image

but maybe because they don't show it by checkboxes?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used the designs from input since it made more sense to me to compare it to another form control. Thoughts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ionic theme didn't have any screenshots for alert.

@thetaPC thetaPC marked this pull request as ready for review May 22, 2026 03:25
@thetaPC thetaPC requested a review from a team as a code owner May 22, 2026 03:25
@thetaPC thetaPC requested a review from brandyscarney May 22, 2026 03:25
@thetaPC thetaPC changed the title fix(select-option): add rich content styles fix(many): add rich content styles for select options May 22, 2026
Comment on lines -117 to -124
:host(.in-select-modal) {
--background-focused: #{globals.$ion-bg-neutral-subtlest-press};
--background-focused-opacity: 0;
}

:host(.in-select-modal.ion-focused) .item-native {
--border-radius: #{globals.$ion-border-radius-400};
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These can live under the select-modal.ionic.scss file

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to match latest designs

Copy link
Copy Markdown
Member

@brandyscarney brandyscarney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking really good, great work! I mostly have questions about some styles and requests for tests. There were a lot of files on this so sorry if I repeated myself anywhere.

overflow: hidden;
}

.action-sheet-icon {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the right size for icons? It looks kind of small:

Image

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm basing it off the tab button icon size and the radio box size as it seems that 24px is the consistent size for elements like this: ccdd136

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there no padding around this when the local test shows padding:

Image

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there no padding around this when the local test shows padding:

Image

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there no padding around this when the local test shows padding:

Image

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inner label text is not vertically aligned for all themes:

Image

You need to add the following:

.action-sheet-button-label-text {
    display: flex;
    align-items: center;
}

It would probably look nicer to have a gap here as well.

Copy link
Copy Markdown
Contributor Author

@thetaPC thetaPC May 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -3,3 +3,6 @@

// iOS Select Popover
// --------------------------------------------------

/// @prop - Maximum size of slotted children rendered in a select option's start/end slot
$select-popover-ios-select-option-slot-size: dynamic-font-max(24px, 2);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this as a variable?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inner label text is not vertically aligned for all themes:

Image

You need to add the following:

.select-option-label-text {
    align-items: center;
}


private closeModal() {
private closeModal(isOptionDisabled = false) {
if (isOptionDisabled) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as popover - why is this suddenly needed and do we have tests for it?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the same concern with the avatar size as I commented on action sheet and popover.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the same concern with the avatar size as I commented on action sheet and popover.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: angular @ionic/angular package package: core @ionic/core package package: vue @ionic/vue package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants