From 0a00c7362e25a46b8e143a9a261d3affded40344 Mon Sep 17 00:00:00 2001 From: ShaneK Date: Thu, 20 Aug 2026 10:54:01 -0700 Subject: [PATCH] fix(item-sliding, segment-button): read props after frameworks assign them --- .../components/item-sliding/item-sliding.tsx | 23 ++-- .../test/basic/item-sliding.e2e.ts | 74 ++++++++++++ .../segment-button/segment-button.tsx | 20 ++-- .../test/disabled/segment-view.e2e.ts | 110 ++++++++++++++++++ core/src/utils/test/late-props/index.html | 48 ++++++++ core/src/utils/test/late-props/late-props.js | 69 +++++++++++ 6 files changed, 323 insertions(+), 21 deletions(-) create mode 100644 core/src/utils/test/late-props/index.html create mode 100644 core/src/utils/test/late-props/late-props.js diff --git a/core/src/components/item-sliding/item-sliding.tsx b/core/src/components/item-sliding/item-sliding.tsx index ba979164c47..d2bce0e0dfd 100644 --- a/core/src/components/item-sliding/item-sliding.tsx +++ b/core/src/components/item-sliding/item-sliding.tsx @@ -1,7 +1,7 @@ import type { ComponentInterface, EventEmitter } from '@stencil/core'; import { Component, Element, Event, Host, Method, Prop, State, Watch, h } from '@stencil/core'; import { findClosestIonContent, disableContentScrollY, resetContentScrollY } from '@utils/content'; -import { isEndSide } from '@utils/helpers'; +import { componentOnReady, isEndSide } from '@utils/helpers'; import { printIonWarning } from '@utils/logging'; import { watchForOptions } from '@utils/watch-options'; @@ -245,24 +245,21 @@ export class ItemSliding implements ComponentInterface { } private async updateOptions() { - const options = this.el.querySelectorAll('ion-item-options'); + const options = Array.from(this.el.querySelectorAll('ion-item-options')); + + /** + * Frameworks that assign element props after inserting the element haven't set + * `side` while `connectedCallback` runs, so reading it any earlier reports every + * option as `end`. + */ + await Promise.all(options.map((option) => new Promise((resolve) => componentOnReady(option, resolve)))); let sides = 0; // Reset left and right options in case they were removed this.leftOptions = this.rightOptions = undefined; - for (let i = 0; i < options.length; i++) { - const item = options.item(i); - - /** - * We cannot use the componentOnReady helper - * util here since we need to wait for all of these items - * to be ready before we set `this.sides` and `this.optsDirty`. - */ - // eslint-disable-next-line custom-rules/no-component-on-ready-method - const option = (item as any).componentOnReady !== undefined ? await item.componentOnReady() : item; - + for (const option of options) { const side = isEndSide(option.side ?? option.getAttribute('side')) ? 'end' : 'start'; if (side === 'start') { diff --git a/core/src/components/item-sliding/test/basic/item-sliding.e2e.ts b/core/src/components/item-sliding/test/basic/item-sliding.e2e.ts index 908155f0ea5..66797b3ad4e 100644 --- a/core/src/components/item-sliding/test/basic/item-sliding.e2e.ts +++ b/core/src/components/item-sliding/test/basic/item-sliding.e2e.ts @@ -1,4 +1,5 @@ import { expect } from '@playwright/test'; +import type { E2EPage } from '@utils/test/playwright'; import { configs, dragElementBy, test } from '@utils/test/playwright'; /** @@ -190,3 +191,76 @@ configs().forEach(({ title, screenshot, config }) => { }); }); }); + +/** + * ion-item-sliding reads `side` off each ion-item-options to decide which way the item + * can open. Frameworks that assign element props after inserting the element haven't set + * it while `connectedCallback` runs. + * + * The shared harness page is used because it loads the custom elements build, which is + * where that ordering applies. + * + * This behavior does not vary across modes or directions. + */ +configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => { + test.describe(title('item-sliding: basic'), () => { + const openStartOptions = async (page: E2EPage, lateProps: boolean) => { + await page.goto('/src/utils/test/late-props', config); + await page.waitForFunction(() => (window as any).harnessReady === true); + + await page.evaluate( + (late: boolean) => + (window as any).mountLateProps( + ['ion-content', 'ion-list', 'ion-item', 'ion-item-sliding', 'ion-item-options', 'ion-item-option'], + { + tag: 'ion-content', + children: [ + { + tag: 'ion-list', + children: [ + { + tag: 'ion-item-sliding', + children: [ + { tag: 'ion-item', children: [{ tag: 'p', children: ['No label'] }] }, + { + // Passing `side` as a prop lets `lateProps` control when it can be read. + tag: 'ion-item-options', + props: { side: 'start' }, + children: [{ tag: 'ion-item-option', children: ['Favorite'] }], + }, + ], + }, + ], + }, + ], + }, + late + ), + lateProps + ); + await page.waitForChanges(); + + const slidingItem = page.locator('ion-item-sliding'); + + // A positive drag pulls the item to the right, revealing the start options. + await dragElementBy(slidingItem, page, 150); + await page.waitForChanges(); + + await expect(slidingItem).toHaveClass(/item-sliding-active-options-start/); + await expect(page.locator('ion-item-options')).toBeVisible(); + }; + + test('should open the start options when side is assigned before connecting', async ({ page }) => { + await openStartOptions(page, false); + }); + + test('should open the start options when side is assigned after connecting', async ({ page }, testInfo) => { + testInfo.annotations.push({ + type: 'issue', + description: 'https://github.com/ionic-team/ionic-framework/issues/31388', + }); + + await openStartOptions(page, true); + }); + }); +}); diff --git a/core/src/components/segment-button/segment-button.tsx b/core/src/components/segment-button/segment-button.tsx index 115040298d3..6601efd5b40 100644 --- a/core/src/components/segment-button/segment-button.tsx +++ b/core/src/components/segment-button/segment-button.tsx @@ -73,14 +73,6 @@ export class SegmentButton implements ComponentInterface, ButtonInterface { addEventListener(segmentEl, 'ionSelect', this.updateState); addEventListener(segmentEl, 'ionStyle', this.updateStyle); } - - // Prevent buttons from being disabled when associated with segment content - if (this.contentId && this.disabled) { - printIonWarning( - `[ion-segment-button] - Segment buttons cannot be disabled when associated with an .` - ); - this.disabled = false; - } } disconnectedCallback() { @@ -100,6 +92,18 @@ export class SegmentButton implements ComponentInterface, ButtonInterface { // Return if there is no contentId defined if (!this.contentId) return; + /** + * Checked here rather than in `connectedCallback` so frameworks that assign element + * props after inserting the element have set `disabled` by now. A disabled ion-segment + * pushes that onto its buttons too, which this guard should not undo. + */ + if (this.disabled && this.segmentEl?.disabled !== true) { + printIonWarning( + `[ion-segment-button] - Segment buttons cannot be disabled when associated with an .` + ); + this.disabled = false; + } + // Attempt to find the Segment Content by its contentId const segmentContent = document.getElementById(this.contentId) as HTMLIonSegmentContentElement | null; diff --git a/core/src/components/segment-view/test/disabled/segment-view.e2e.ts b/core/src/components/segment-view/test/disabled/segment-view.e2e.ts index c7dead8943f..8429f63fbbe 100644 --- a/core/src/components/segment-view/test/disabled/segment-view.e2e.ts +++ b/core/src/components/segment-view/test/disabled/segment-view.e2e.ts @@ -47,3 +47,113 @@ configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => { }); }); }); + +/** + * Frameworks that assign element props after inserting the element have set neither + * `contentId` nor `disabled` while `connectedCallback` runs, so the check that keeps a + * button enabled has to happen later. + * + * The shared harness page is used because it loads the custom elements build, which is + * where that ordering applies. + * + * This behavior does not vary across modes or directions. + */ +configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => { + test.describe(title('segment-view: disabled'), () => { + [false, true].forEach((lateProps) => { + const when = lateProps ? 'after connecting' : 'before connecting'; + + test(`should only re-enable the developer-disabled button when props are assigned ${when}`, async ({ page }) => { + const warnings: string[] = []; + + page.on('console', (msg) => { + if (msg.type() === 'warning') { + warnings.push(msg.text()); + } + }); + + await page.goto('/src/utils/test/late-props', config); + await page.waitForFunction(() => (window as any).harnessReady === true); + + await page.evaluate( + (late: boolean) => + (window as any).mountLateProps( + ['ion-segment', 'ion-segment-button', 'ion-segment-view', 'ion-segment-content', 'ion-label'], + { + tag: 'div', + children: [ + { + // The developer disabled the second button, which has to be forced back on. + tag: 'ion-segment', + props: { value: 'first' }, + children: [ + { + tag: 'ion-segment-button', + props: { value: 'first', contentId: 'first-content' }, + children: [{ tag: 'ion-label', children: ['First'] }], + }, + { + tag: 'ion-segment-button', + props: { value: 'second', contentId: 'second-content', disabled: true }, + children: [{ tag: 'ion-label', children: ['Second'] }], + }, + ], + }, + { + tag: 'ion-segment-view', + children: [ + { tag: 'ion-segment-content', attrs: { id: 'first-content' }, children: ['First'] }, + { tag: 'ion-segment-content', attrs: { id: 'second-content' }, children: ['Second'] }, + ], + }, + { + // This whole segment is disabled, so its buttons stay off. + tag: 'ion-segment', + props: { value: 'third', disabled: true }, + children: [ + { + tag: 'ion-segment-button', + props: { value: 'third', contentId: 'third-content' }, + children: [{ tag: 'ion-label', children: ['Third'] }], + }, + { + tag: 'ion-segment-button', + props: { value: 'fourth', contentId: 'fourth-content' }, + children: [{ tag: 'ion-label', children: ['Fourth'] }], + }, + ], + }, + { + tag: 'ion-segment-view', + children: [ + { tag: 'ion-segment-content', attrs: { id: 'third-content' }, children: ['Third'] }, + { tag: 'ion-segment-content', attrs: { id: 'fourth-content' }, children: ['Fourth'] }, + ], + }, + ], + }, + late + ), + lateProps + ); + await page.waitForChanges(); + + const disabled = await page + .locator('ion-segment') + .evaluateAll((segments: HTMLIonSegmentElement[]) => + segments.map((segment) => + Array.from(segment.querySelectorAll('ion-segment-button')).map((button) => button.disabled) + ) + ); + + expect(disabled).toEqual([ + [false, false], + [true, true], + ]); + expect(warnings.join('\n')).toContain( + '[ion-segment-button] - Segment buttons cannot be disabled when associated with an .' + ); + }); + }); + }); +}); diff --git a/core/src/utils/test/late-props/index.html b/core/src/utils/test/late-props/index.html new file mode 100644 index 00000000000..395328d21f7 --- /dev/null +++ b/core/src/utils/test/late-props/index.html @@ -0,0 +1,48 @@ + + + + + Late Props + + + + + + + + + + + +
+ + + diff --git a/core/src/utils/test/late-props/late-props.js b/core/src/utils/test/late-props/late-props.js new file mode 100644 index 00000000000..7d29334b4cf --- /dev/null +++ b/core/src/utils/test/late-props/late-props.js @@ -0,0 +1,69 @@ +/** + * Test helpers for the custom elements build, where `connectedCallback` runs + * synchronously as the element is inserted. Frameworks that assign element props after + * inserting the element leave a window where a component can't read its own props or + * its children's, and it still has to work. + */ + +import { initialize } from '/components/index.js'; + +/** + * Initializes Ionic in the mode the test asked for. + */ +export const initializeIonic = () => { + initialize({ mode: new URLSearchParams(location.search).get('ionic:mode') ?? 'ios' }); +}; + +/** + * Defines the given tags. Safe to call again for tags that are already defined, so each + * test can ask for whatever it needs. + */ +export const defineTags = async (tags) => { + await Promise.all( + tags.map(async (tag) => { + const mod = await import(`/components/${tag}.js`); + mod.defineCustomElement(); + }) + ); +}; + +/** + * Builds the tree described by `spec` and appends it to `root`. A spec node is + * `{ tag, props, attrs, children }`, where `children` holds specs or strings. The + * `attrs` are always set before the element connects, and the `props` are set before + * connecting when `lateProps` is false, or after the whole tree connects when it is true. + */ +export const mount = (root, spec, lateProps) => { + const pending = []; + + const build = (node) => { + if (typeof node === 'string') { + return document.createTextNode(node); + } + + const el = document.createElement(node.tag); + + if (node.attrs) { + Object.entries(node.attrs).forEach(([key, value]) => el.setAttribute(key, String(value))); + } + + if (node.props) { + if (lateProps) { + pending.push([el, node.props]); + } else { + Object.assign(el, node.props); + } + } + + (node.children || []).forEach((child) => el.appendChild(build(child))); + + return el; + }; + + const tree = build(spec); + + root.appendChild(tree); + + // Descendants before ancestors, matching the order framework effects run in. + pending.reverse().forEach(([el, props]) => Object.assign(el, props)); +};