From 5e5efaf36e525f7b5f42df538d248fa13bce723c Mon Sep 17 00:00:00 2001 From: ShaneK Date: Thu, 12 Jun 2025 06:46:48 -0700 Subject: [PATCH 1/3] fix(range): improve focus and blur handling for dual knobs --- core/src/components/range/range.tsx | 69 +++++- .../components/range/test/basic/index.html | 4 + .../components/range/test/basic/range.spec.ts | 199 ++++++++++++++++++ 3 files changed, 271 insertions(+), 1 deletion(-) create mode 100644 core/src/components/range/test/basic/range.spec.ts diff --git a/core/src/components/range/range.tsx b/core/src/components/range/range.tsx index eac3dd3ebcd..d39e9732e82 100644 --- a/core/src/components/range/range.tsx +++ b/core/src/components/range/range.tsx @@ -639,6 +639,51 @@ export class Range implements ComponentInterface { } }; + private onKnobFocus = (knob: KnobName) => { + if (!this.hasFocus) { + this.hasFocus = true; + this.ionFocus.emit(); + } + + // Manually manage ion-focused class for dual knobs + if (this.dualKnobs && this.el.shadowRoot) { + const knobA = this.el.shadowRoot.querySelector('.range-knob-a'); + const knobB = this.el.shadowRoot.querySelector('.range-knob-b'); + + // Remove ion-focused from both knobs first + knobA?.classList.remove('ion-focused'); + knobB?.classList.remove('ion-focused'); + + // Add ion-focused only to the focused knob + const focusedKnobEl = knob === 'A' ? knobA : knobB; + focusedKnobEl?.classList.add('ion-focused'); + } + }; + + private onKnobBlur = () => { + // Check if focus is moving to another knob within the same range + // by delaying the reset to allow the new focus to register + setTimeout(() => { + const activeElement = this.el.shadowRoot?.activeElement; + const isStillFocusedOnKnob = activeElement && activeElement.classList.contains('range-knob-handle'); + + if (!isStillFocusedOnKnob) { + if (this.hasFocus) { + this.hasFocus = false; + this.ionBlur.emit(); + } + + // Remove ion-focused from both knobs when focus leaves the range + if (this.dualKnobs && this.el.shadowRoot) { + const knobA = this.el.shadowRoot.querySelector('.range-knob-a'); + const knobB = this.el.shadowRoot.querySelector('.range-knob-b'); + knobA?.classList.remove('ion-focused'); + knobB?.classList.remove('ion-focused'); + } + } + }, 0); + }; + /** * Returns true if content was passed to the "start" slot */ @@ -813,6 +858,8 @@ export class Range implements ComponentInterface { min, max, inheritedAttributes, + onKnobFocus: this.onKnobFocus, + onKnobBlur: this.onKnobBlur, })} {this.dualKnobs && @@ -828,6 +875,8 @@ export class Range implements ComponentInterface { min, max, inheritedAttributes, + onKnobFocus: this.onKnobFocus, + onKnobBlur: this.onKnobBlur, })} ); @@ -908,11 +957,27 @@ interface RangeKnob { pinFormatter: PinFormatter; inheritedAttributes: Attributes; handleKeyboard: (name: KnobName, isIncrease: boolean) => void; + onKnobFocus: (knob: KnobName) => void; + onKnobBlur: () => void; } const renderKnob = ( rtl: boolean, - { knob, value, ratio, min, max, disabled, pressed, pin, handleKeyboard, pinFormatter, inheritedAttributes }: RangeKnob + { + knob, + value, + ratio, + min, + max, + disabled, + pressed, + pin, + handleKeyboard, + pinFormatter, + inheritedAttributes, + onKnobFocus, + onKnobBlur, + }: RangeKnob ) => { const start = rtl ? 'right' : 'left'; @@ -941,6 +1006,8 @@ const renderKnob = ( ev.stopPropagation(); } }} + onFocus={() => onKnobFocus(knob)} + onBlur={onKnobBlur} class={{ 'range-knob-handle': true, 'range-knob-a': knob === 'A', diff --git a/core/src/components/range/test/basic/index.html b/core/src/components/range/test/basic/index.html index 50a7ad05197..98370423f06 100644 --- a/core/src/components/range/test/basic/index.html +++ b/core/src/components/range/test/basic/index.html @@ -80,6 +80,10 @@

Pin

lower: '10', upper: '90', }; + + dualKnobs.addEventListener('ionFocus', () => { + console.log('Dual Knob ionFocus', dualKnobs.value); + }); diff --git a/core/src/components/range/test/basic/range.spec.ts b/core/src/components/range/test/basic/range.spec.ts new file mode 100644 index 00000000000..9d91c66bbec --- /dev/null +++ b/core/src/components/range/test/basic/range.spec.ts @@ -0,0 +1,199 @@ +import { newSpecPage } from '@stencil/core/testing'; + +import { Range } from '../../range'; + +describe('range: dual knobs focus management', () => { + it('should properly manage initial focus with dual knobs', async () => { + const page = await newSpecPage({ + components: [Range], + html: ` + + + `, + }); + + const range = page.body.querySelector('ion-range'); + expect(range).not.toBeNull(); + + await page.waitForChanges(); + + // Get the knob elements + const knobA = range!.shadowRoot!.querySelector('.range-knob-a') as HTMLElement; + const knobB = range!.shadowRoot!.querySelector('.range-knob-b') as HTMLElement; + + expect(knobA).not.toBeNull(); + expect(knobB).not.toBeNull(); + + // Initially, neither knob should have the ion-focused class + expect(knobA.classList.contains('ion-focused')).toBe(false); + expect(knobB.classList.contains('ion-focused')).toBe(false); + }); + + it('should show focus on the correct knob when focused via keyboard navigation', async () => { + const page = await newSpecPage({ + components: [Range], + html: ` + + + `, + }); + + const range = page.body.querySelector('ion-range'); + await page.waitForChanges(); + + const knobA = range!.shadowRoot!.querySelector('.range-knob-a') as HTMLElement; + const knobB = range!.shadowRoot!.querySelector('.range-knob-b') as HTMLElement; + + // Focus knob A + knobA.dispatchEvent(new Event('focus')); + await page.waitForChanges(); + + // Only knob A should have the ion-focused class + expect(knobA.classList.contains('ion-focused')).toBe(true); + expect(knobB.classList.contains('ion-focused')).toBe(false); + + // Focus knob B + knobB.dispatchEvent(new Event('focus')); + await page.waitForChanges(); + + // Only knob B should have the ion-focused class + expect(knobA.classList.contains('ion-focused')).toBe(false); + expect(knobB.classList.contains('ion-focused')).toBe(true); + }); + + it('should remove focus from all knobs when focus leaves the range', async () => { + const page = await newSpecPage({ + components: [Range], + html: ` + + + `, + }); + + const range = page.body.querySelector('ion-range'); + await page.waitForChanges(); + + const knobA = range!.shadowRoot!.querySelector('.range-knob-a') as HTMLElement; + const knobB = range!.shadowRoot!.querySelector('.range-knob-b') as HTMLElement; + + // Focus knob A + knobA.dispatchEvent(new Event('focus')); + await page.waitForChanges(); + + expect(knobA.classList.contains('ion-focused')).toBe(true); + + // Blur the knob (focus leaves the range) + knobA.dispatchEvent(new Event('blur')); + await page.waitForChanges(); + + // Wait for the timeout in onKnobBlur to complete + await new Promise((resolve) => setTimeout(resolve, 10)); + await page.waitForChanges(); + + // Neither knob should have the ion-focused class + expect(knobA.classList.contains('ion-focused')).toBe(false); + expect(knobB.classList.contains('ion-focused')).toBe(false); + }); + + it('should emit ionFocus when any knob receives focus', async () => { + const page = await newSpecPage({ + components: [Range], + html: ` + + + `, + }); + + const range = page.body.querySelector('ion-range')!; + await page.waitForChanges(); + + let focusEventFired = false; + range.addEventListener('ionFocus', () => { + focusEventFired = true; + }); + + const knobA = range.shadowRoot!.querySelector('.range-knob-a') as HTMLElement; + + // Focus knob A + knobA.dispatchEvent(new Event('focus')); + await page.waitForChanges(); + + expect(focusEventFired).toBe(true); + }); + + it('should emit ionBlur when focus leaves the range completely', async () => { + const page = await newSpecPage({ + components: [Range], + html: ` + + + `, + }); + + const range = page.body.querySelector('ion-range')!; + await page.waitForChanges(); + + let blurEventFired = false; + range.addEventListener('ionBlur', () => { + blurEventFired = true; + }); + + const knobA = range.shadowRoot!.querySelector('.range-knob-a') as HTMLElement; + + // Focus and then blur knob A + knobA.dispatchEvent(new Event('focus')); + await page.waitForChanges(); + + knobA.dispatchEvent(new Event('blur')); + await page.waitForChanges(); + + // Wait for the timeout in onKnobBlur to complete + await new Promise((resolve) => setTimeout(resolve, 10)); + await page.waitForChanges(); + + expect(blurEventFired).toBe(true); + }); + + it('should correctly handle Tab navigation between knobs', async () => { + const page = await newSpecPage({ + components: [Range], + html: ` + + + `, + }); + + const range = page.body.querySelector('ion-range')!; + await page.waitForChanges(); + + const knobA = range.shadowRoot!.querySelector('.range-knob-a') as HTMLElement; + const knobB = range.shadowRoot!.querySelector('.range-knob-b') as HTMLElement; + + // Simulate Tab to first knob + knobA.dispatchEvent(new Event('focus')); + await page.waitForChanges(); + + // First knob should be focused + expect(knobA.classList.contains('ion-focused')).toBe(true); + expect(knobB.classList.contains('ion-focused')).toBe(false); + + // Simulate Tab to second knob (blur first, focus second) + knobA.dispatchEvent(new Event('blur')); + knobB.dispatchEvent(new Event('focus')); + await page.waitForChanges(); + + // Second knob should be focused, first should not + expect(knobA.classList.contains('ion-focused')).toBe(false); + expect(knobB.classList.contains('ion-focused')).toBe(true); + + // Verify Arrow key navigation still works on focused knob + + // Simulate Arrow Right key press on knob B + const keyEvent = new KeyboardEvent('keydown', { key: 'ArrowRight' }); + knobB.dispatchEvent(keyEvent); + await page.waitForChanges(); + + // The knob that visually appears focused should be the one that responds to keyboard input + expect(knobB.classList.contains('ion-focused')).toBe(true); + }); +}); From 40db33569c5d38c0c81b6233163e25b7ae2155ee Mon Sep 17 00:00:00 2001 From: ShaneK Date: Thu, 12 Jun 2025 12:53:52 -0700 Subject: [PATCH 2/3] test(range): emit ionFocus only once per focus event --- core/src/components/range/test/basic/range.spec.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/core/src/components/range/test/basic/range.spec.ts b/core/src/components/range/test/basic/range.spec.ts index 9d91c66bbec..c83f3fbca2a 100644 --- a/core/src/components/range/test/basic/range.spec.ts +++ b/core/src/components/range/test/basic/range.spec.ts @@ -95,7 +95,7 @@ describe('range: dual knobs focus management', () => { expect(knobB.classList.contains('ion-focused')).toBe(false); }); - it('should emit ionFocus when any knob receives focus', async () => { + it('should emit ionFocus when any knob receives focus but only once until blur', async () => { const page = await newSpecPage({ components: [Range], html: ` @@ -107,18 +107,20 @@ describe('range: dual knobs focus management', () => { const range = page.body.querySelector('ion-range')!; await page.waitForChanges(); - let focusEventFired = false; + let focusEventFiredCount = 0; range.addEventListener('ionFocus', () => { - focusEventFired = true; + focusEventFiredCount++; }); const knobA = range.shadowRoot!.querySelector('.range-knob-a') as HTMLElement; + const knobB = range.shadowRoot!.querySelector('.range-knob-b') as HTMLElement; // Focus knob A knobA.dispatchEvent(new Event('focus')); + knobB.dispatchEvent(new Event('focus')); await page.waitForChanges(); - expect(focusEventFired).toBe(true); + expect(focusEventFiredCount).toBe(1); }); it('should emit ionBlur when focus leaves the range completely', async () => { From 3f514ff54c9780a5476e21f525d57df3adfd61fe Mon Sep 17 00:00:00 2001 From: ShaneK Date: Fri, 13 Jun 2025 09:24:18 -0700 Subject: [PATCH 3/3] test(range): improving test of dual knob range slider for actual usage test --- .../components/range/test/basic/range.spec.ts | 65 +++++++++++++++---- 1 file changed, 54 insertions(+), 11 deletions(-) diff --git a/core/src/components/range/test/basic/range.spec.ts b/core/src/components/range/test/basic/range.spec.ts index c83f3fbca2a..ab53ee05dc2 100644 --- a/core/src/components/range/test/basic/range.spec.ts +++ b/core/src/components/range/test/basic/range.spec.ts @@ -156,46 +156,89 @@ describe('range: dual knobs focus management', () => { expect(blurEventFired).toBe(true); }); - it('should correctly handle Tab navigation between knobs', async () => { + it('should correctly handle Tab navigation between knobs using KeyboardEvent', async () => { + // Using KeyboardEvent to simulate Tab key is more realistic than just firing focus events + // because it tests the actual keyboard navigation behavior users would experience const page = await newSpecPage({ components: [Range], html: ` + + `, }); const range = page.body.querySelector('ion-range')!; + const beforeButton = page.body.querySelector('#before') as HTMLElement; await page.waitForChanges(); const knobA = range.shadowRoot!.querySelector('.range-knob-a') as HTMLElement; const knobB = range.shadowRoot!.querySelector('.range-knob-b') as HTMLElement; - // Simulate Tab to first knob - knobA.dispatchEvent(new Event('focus')); + // Start with focus on element before the range + beforeButton.focus(); + + // Simulate Tab key press - this would move focus to first knob + let tabEvent = new KeyboardEvent('keydown', { + key: 'Tab', + code: 'Tab', + bubbles: true, + cancelable: true, + }); + + beforeButton.dispatchEvent(tabEvent); + knobA.focus(); // Browser would focus next tabindex element await page.waitForChanges(); // First knob should be focused expect(knobA.classList.contains('ion-focused')).toBe(true); expect(knobB.classList.contains('ion-focused')).toBe(false); - // Simulate Tab to second knob (blur first, focus second) - knobA.dispatchEvent(new Event('blur')); - knobB.dispatchEvent(new Event('focus')); + // Simulate another Tab key press - this would move focus to second knob + tabEvent = new KeyboardEvent('keydown', { + key: 'Tab', + code: 'Tab', + bubbles: true, + cancelable: true, + }); + + knobA.dispatchEvent(tabEvent); + knobB.focus(); // Browser would focus next tabindex element await page.waitForChanges(); // Second knob should be focused, first should not expect(knobA.classList.contains('ion-focused')).toBe(false); expect(knobB.classList.contains('ion-focused')).toBe(true); - // Verify Arrow key navigation still works on focused knob + // Simulate Shift+Tab (reverse tab) - should go back to first knob + const shiftTabEvent = new KeyboardEvent('keydown', { + key: 'Tab', + code: 'Tab', + shiftKey: true, + bubbles: true, + cancelable: true, + }); - // Simulate Arrow Right key press on knob B - const keyEvent = new KeyboardEvent('keydown', { key: 'ArrowRight' }); - knobB.dispatchEvent(keyEvent); + knobB.dispatchEvent(shiftTabEvent); + knobA.focus(); // Browser would focus previous tabindex element + await page.waitForChanges(); + + // First knob should be focused again + expect(knobA.classList.contains('ion-focused')).toBe(true); + expect(knobB.classList.contains('ion-focused')).toBe(false); + + // Verify Arrow key navigation still works on focused knob + const arrowEvent = new KeyboardEvent('keydown', { + key: 'ArrowRight', + code: 'ArrowRight', + bubbles: true, + cancelable: true, + }); + knobA.dispatchEvent(arrowEvent); await page.waitForChanges(); // The knob that visually appears focused should be the one that responds to keyboard input - expect(knobB.classList.contains('ion-focused')).toBe(true); + expect(knobA.classList.contains('ion-focused')).toBe(true); }); });