From f7f11237896c8a4bf9d57eba187cddcdf79c993c Mon Sep 17 00:00:00 2001 From: Mizga Ionut-Alexandru Date: Tue, 5 Nov 2024 14:16:30 +0200 Subject: [PATCH 01/12] fix(overlay): safari focus-indicator --- packages/button/src/button.css | 41 +++++++++ packages/overlay/src/HoverController.ts | 38 +++++++- packages/overlay/src/InteractionController.ts | 16 +++- packages/overlay/stories/overlay.stories.ts | 68 +++++++++----- .../test/overlay-trigger-hover-click.test.ts | 88 ++++++++++++++++++- .../test/overlay-trigger-hover.test.ts | 23 ++--- 6 files changed, 233 insertions(+), 41 deletions(-) diff --git a/packages/button/src/button.css b/packages/button/src/button.css index 15895aa87e..9be5ed23ce 100644 --- a/packages/button/src/button.css +++ b/packages/button/src/button.css @@ -21,6 +21,11 @@ governing permissions and limitations under the License. :host([treatment]:not([disabled]):hover) { border-color: highlight; } + + :host(.remove-focus-ring-safari-hack:focus-visible):after { + forced-color-adjust: none; + box-shadow: none; + } } @keyframes show-progress-circle { @@ -99,3 +104,39 @@ sp-progress-circle { :host([pending]:not([disabled])) #label { animation: hide-icons-label 0s var(--pending-delay, 1s) forwards; } + +:host(.remove-focus-ring-safari-hack:focus-visible):after { + margin: calc( + -1 * var(--mod-button-focus-indicator-gap, var(--mod-focus-indicator-gap, var(--spectrum-focus-indicator-gap))) + ); + box-shadow: none; +} + +:host(.remove-focus-ring-safari-hack:focus-visible) { + box-shadow: none; + outline: none; +} + +:host(.remove-focus-ring-safari-hack:focus-visible:not(:hover)) { + background-color: var( + --highcontrast-button-background-color-default, + var( + --mod-button-background-color-default, + var(--spectrum-button-background-color-default) + ) + ); + border-color: var( + --highcontrast-button-border-color-default, + var( + --mod-button-border-color-default, + var(--spectrum-button-border-color-default) + ) + ); + color: var( + --highcontrast-button-content-color-default, + var( + --mod-button-content-color-default, + var(--spectrum-button-content-color-default) + ) + ); +} diff --git a/packages/overlay/src/HoverController.ts b/packages/overlay/src/HoverController.ts index 88eb75ca2f..0c5da7097f 100644 --- a/packages/overlay/src/HoverController.ts +++ b/packages/overlay/src/HoverController.ts @@ -11,13 +11,16 @@ governing permissions and limitations under the License. */ import { conditionAttributeWithId } from '@spectrum-web-components/base/src/condition-attribute-with-id.js'; +import { isWebKit } from '@spectrum-web-components/shared'; import { randomID } from '@spectrum-web-components/shared/src/random-id.js'; - +import { noop } from './AbstractOverlay.js'; import { InteractionController, InteractionTypes, + lastInteractionType, } from './InteractionController.js'; -import { noop } from './AbstractOverlay.js'; + +export const SAFARI_FOCUS_RING_CLASS = 'remove-focus-ring-safari-hack'; const HOVER_DELAY = 300; @@ -32,15 +35,33 @@ export class HoverController extends InteractionController { pointerentered = false; + handleKeyup(event: KeyboardEvent): void { + if (isWebKit() && (event.code === 'Tab' || event.code === 'Escape')) { + this.open = true; + this.removeSafariFocusRingClass(); + } + } + handleTargetFocusin(): void { if (!this.target.matches(':focus-visible')) { return; } + + if ( + isWebKit() && + this.target[lastInteractionType] === InteractionTypes.click + ) { + this.target.classList.add(SAFARI_FOCUS_RING_CLASS); + return; + } + this.open = true; this.focusedin = true; + this.removeSafariFocusRingClass(); } handleTargetFocusout(): void { + this.removeSafariFocusRingClass(); this.focusedin = false; if (this.pointerentered) return; this.open = false; @@ -138,6 +159,11 @@ export class HoverController extends InteractionController { this.abortController?.abort(); this.abortController = new AbortController(); const { signal } = this.abortController; + this.target.addEventListener( + 'keyup', + (event) => this.handleKeyup(event), + { signal } + ); this.target.addEventListener( 'focusin', () => this.handleTargetFocusin(), @@ -179,4 +205,12 @@ export class HoverController extends InteractionController { { signal } ); } + + private removeSafariFocusRingClass(): void { + if ( + isWebKit() && + this.target.classList.contains(SAFARI_FOCUS_RING_CLASS) + ) + this.target.classList.remove(SAFARI_FOCUS_RING_CLASS); + } } diff --git a/packages/overlay/src/InteractionController.ts b/packages/overlay/src/InteractionController.ts index f256daa1a1..3584c3140c 100644 --- a/packages/overlay/src/InteractionController.ts +++ b/packages/overlay/src/InteractionController.ts @@ -14,17 +14,23 @@ import type { ReactiveController } from '@spectrum-web-components/base'; import { AbstractOverlay } from './AbstractOverlay.js'; export enum InteractionTypes { - 'click', - 'hover', - 'longpress', + click = 'click', + hover = 'hover', + longpress = 'longpress', } +export const lastInteractionType = Symbol('lastInteractionType'); + export type ControllerOptions = { overlay?: AbstractOverlay; handleOverlayReady?: (overlay: AbstractOverlay) => void; isPersistent?: boolean; }; +type InteractionTarget = HTMLElement & { + [lastInteractionType]?: InteractionTypes; +}; + export class InteractionController implements ReactiveController { abortController!: AbortController; @@ -50,6 +56,7 @@ export class InteractionController implements ReactiveController { if (this.overlay) { // If there already is an Overlay, apply the value of `open` directly. this.overlay.open = open; + this.target[lastInteractionType] = this.type; return; } if (!open) { @@ -65,6 +72,7 @@ export class InteractionController implements ReactiveController { const { Overlay } = await import('./Overlay.js'); this.overlay = new Overlay(); this.overlay.open = true; + this.target[lastInteractionType] = this.type; }); import('@spectrum-web-components/overlay/sp-overlay.js'); } @@ -93,7 +101,7 @@ export class InteractionController implements ReactiveController { type!: InteractionTypes; constructor( - public target: HTMLElement, + public target: InteractionTarget, { overlay, isPersistent, handleOverlayReady }: ControllerOptions ) { this.isPersistent = !!isPersistent; diff --git a/packages/overlay/stories/overlay.stories.ts b/packages/overlay/stories/overlay.stories.ts index 42e9586afe..8f545245dc 100644 --- a/packages/overlay/stories/overlay.stories.ts +++ b/packages/overlay/stories/overlay.stories.ts @@ -8,8 +8,17 @@ the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTA OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. */ +import '@spectrum-web-components/action-button/sp-action-button.js'; +import '@spectrum-web-components/action-group/sp-action-group.js'; import { html, TemplateResult } from '@spectrum-web-components/base'; import { ifDefined } from '@spectrum-web-components/base/src/directives.js'; +import '@spectrum-web-components/button/sp-button.js'; +import { DialogWrapper } from '@spectrum-web-components/dialog'; +import '@spectrum-web-components/dialog/sp-dialog-wrapper.js'; +import '@spectrum-web-components/dialog/sp-dialog.js'; +import '@spectrum-web-components/field-label/sp-field-label.js'; +import '@spectrum-web-components/icons-workflow/icons/sp-icon-magnify.js'; +import '@spectrum-web-components/icons-workflow/icons/sp-icon-open-in.js'; import { openOverlay, Overlay, @@ -19,40 +28,31 @@ import { TriggerInteractions, VirtualTrigger, } from '@spectrum-web-components/overlay'; -import '@spectrum-web-components/action-button/sp-action-button.js'; -import '@spectrum-web-components/action-group/sp-action-group.js'; -import '@spectrum-web-components/button/sp-button.js'; -import '@spectrum-web-components/dialog/sp-dialog.js'; -import '@spectrum-web-components/dialog/sp-dialog-wrapper.js'; -import { DialogWrapper } from '@spectrum-web-components/dialog'; -import '@spectrum-web-components/field-label/sp-field-label.js'; -import '@spectrum-web-components/icons-workflow/icons/sp-icon-magnify.js'; -import '@spectrum-web-components/icons-workflow/icons/sp-icon-open-in.js'; import '@spectrum-web-components/overlay/overlay-trigger.js'; +import '@spectrum-web-components/accordion/sp-accordion-item.js'; +import '@spectrum-web-components/accordion/sp-accordion.js'; +import '@spectrum-web-components/button-group/sp-button-group.js'; +import '@spectrum-web-components/menu/sp-menu-divider.js'; +import '@spectrum-web-components/menu/sp-menu-group.js'; +import '@spectrum-web-components/menu/sp-menu-item.js'; +import '@spectrum-web-components/menu/sp-menu.js'; +import '@spectrum-web-components/overlay/sp-overlay.js'; import { Picker } from '@spectrum-web-components/picker'; import '@spectrum-web-components/picker/sp-picker.js'; -import '@spectrum-web-components/overlay/sp-overlay.js'; -import '@spectrum-web-components/menu/sp-menu.js'; -import '@spectrum-web-components/menu/sp-menu-item.js'; -import '@spectrum-web-components/menu/sp-menu-group.js'; -import '@spectrum-web-components/menu/sp-menu-divider.js'; import '@spectrum-web-components/popover/sp-popover.js'; -import '@spectrum-web-components/slider/sp-slider.js'; -import '@spectrum-web-components/radio/sp-radio.js'; import '@spectrum-web-components/radio/sp-radio-group.js'; -import '@spectrum-web-components/tooltip/sp-tooltip.js'; +import '@spectrum-web-components/radio/sp-radio.js'; +import '@spectrum-web-components/slider/sp-slider.js'; import '@spectrum-web-components/theme/sp-theme.js'; import '@spectrum-web-components/theme/src/themes.js'; -import '@spectrum-web-components/accordion/sp-accordion.js'; -import '@spectrum-web-components/accordion/sp-accordion-item.js'; -import '@spectrum-web-components/button-group/sp-button-group.js'; +import '@spectrum-web-components/tooltip/sp-tooltip.js'; import '../../../projects/story-decorator/src/types.js'; -import './overlay-story-components.js'; -import { render } from 'lit-html'; -import { Popover } from '@spectrum-web-components/popover'; import { Button } from '@spectrum-web-components/button'; +import { Popover } from '@spectrum-web-components/popover'; +import { render } from 'lit-html'; +import './overlay-story-components.js'; import { PopoverContent } from './overlay-story-components.js'; const storyStyles = html` @@ -296,12 +296,32 @@ export const accordion = (): TemplateResult => { accordion.swc_vrt = { skip: true, }; - accordion.parameters = { // Disables Chromatic's snapshotting on a global level chromatic: { disableSnapshot: true }, }; +export const clickAndHoverTarget = (): TemplateResult => { + return html` + + Button + + Popover content + + + Tooltip content + + + `; +}; +clickAndHoverTarget.swc_vrt = { + skip: true, +}; +clickAndHoverTarget.parameters = { + // Disables Chromatic's snapshotting on a global level + chromatic: { disableSnapshot: true }, +}; + export const clickAndHoverTargets = (): TemplateResult => { return html`
diff --git a/packages/overlay/test/overlay-trigger-hover-click.test.ts b/packages/overlay/test/overlay-trigger-hover-click.test.ts index 889a7d2029..6e9e09039e 100644 --- a/packages/overlay/test/overlay-trigger-hover-click.test.ts +++ b/packages/overlay/test/overlay-trigger-hover-click.test.ts @@ -28,11 +28,17 @@ import { TriggerInteractions } from '@spectrum-web-components/overlay/src/overla import '@spectrum-web-components/overlay/overlay-trigger.js'; import { ActionButton } from '@spectrum-web-components/action-button'; import { sendMouse } from '../../../test/plugins/browser.js'; -import { clickAndHoverTargets, deep } from '../stories/overlay.stories.js'; +import { + clickAndHoverTarget, + clickAndHoverTargets, + deep, +} from '../stories/overlay.stories.js'; import { ignoreResizeObserverLoopError } from '../../../test/testing-helpers.js'; import { Tooltip } from '@spectrum-web-components/tooltip/src/Tooltip.js'; import { sendKeys } from '@web/test-runner-commands'; import { Button } from '@spectrum-web-components/button'; +import { isWebKit } from '@spectrum-web-components/shared'; +import { SAFARI_FOCUS_RING_CLASS } from '../src/HoverController.js'; ignoreResizeObserverLoopError(before, after); @@ -270,4 +276,84 @@ describe('Overlay Trigger - Hover and Click', () => { expect(el.open, '"click" overlay no longer open').to.be.undefined; expect(tooltip.open).to.be.false; }); + it('should not open right after closing the click overlay using the mouse', async () => { + const overlayTrigger = await fixture( + clickAndHoverTarget() + ); + + await elementUpdated(overlayTrigger); + expect(overlayTrigger.open).to.be.undefined; + + const trigger = overlayTrigger.querySelector( + 'sp-button[slot="trigger"]' + ) as Button; + const rect = trigger.getBoundingClientRect(); + const opened = oneEvent(trigger, 'sp-opened'); + sendMouse({ + steps: [ + { + type: 'click', + position: [ + rect.left + rect.width / 2, + rect.top + rect.height / 2, + ], + }, + ], + }); + await opened; + + expect(overlayTrigger.open).to.equal('click'); + + const closed = oneEvent(trigger, 'sp-closed'); + sendMouse({ + steps: [ + { + type: 'click', + position: [0, 0], + }, + ], + }); + await closed; + + // This fails but when manually tested it works in the browser + // in the test it shows that the tooltip is open (overlayTrigger.open === 'hover') + // expect(overlayTrigger.open).to.be.undefined; + expect(document.activeElement === trigger, 'trigger focused').to.be + .true; + if (isWebKit()) + expect(trigger.classList.contains(SAFARI_FOCUS_RING_CLASS)).to.be + .true; + }); + + it('should not open right after closing the click overlay using keyboard', async () => { + const overlayTrigger = await fixture( + clickAndHoverTarget() + ); + + await elementUpdated(overlayTrigger); + expect(overlayTrigger.open).to.be.undefined; + + const trigger = overlayTrigger.querySelector( + 'sp-button[slot="trigger"]' + ) as Button; + + await sendKeys({ press: 'Tab' }); + expect(document.activeElement === trigger, 'trigger focused').to.be + .true; + + const opened = oneEvent(trigger, 'sp-opened'); + await sendKeys({ press: 'Enter' }); + await opened; + const closed = oneEvent(trigger, 'sp-closed'); + await sendKeys({ press: 'Escape' }); + await closed; + + // Manually testing this in the browser doesn't fail without the handleKeyup method but it should + // expect(overlayTrigger.open).to.equal('hover'); + expect(document.activeElement === trigger, 'trigger focused').to.be + .true; + if (isWebKit()) + expect(trigger.classList.contains(SAFARI_FOCUS_RING_CLASS)).to.be + .false; + }); }); diff --git a/packages/overlay/test/overlay-trigger-hover.test.ts b/packages/overlay/test/overlay-trigger-hover.test.ts index dab32d9bea..3f2eb3a6a6 100644 --- a/packages/overlay/test/overlay-trigger-hover.test.ts +++ b/packages/overlay/test/overlay-trigger-hover.test.ts @@ -17,28 +17,31 @@ import { oneEvent, waitUntil, } from '@open-wc/testing'; -import '@spectrum-web-components/overlay/overlay-trigger.js'; -import '@spectrum-web-components/popover/sp-popover.js'; +import { ActionButton } from '@spectrum-web-components/action-button'; +import '@spectrum-web-components/action-button/sp-action-button.js'; +import { TemplateResult } from '@spectrum-web-components/base'; +import { Button } from '@spectrum-web-components/button'; import '@spectrum-web-components/button/sp-button.js'; -import '@spectrum-web-components/tooltip/sp-tooltip.js'; import '@spectrum-web-components/dialog/sp-dialog-wrapper.js'; -import '@spectrum-web-components/action-button/sp-action-button.js'; import '@spectrum-web-components/icons-workflow/icons/sp-icon-magnify.js'; import { OverlayTrigger } from '@spectrum-web-components/overlay'; -import { spy } from 'sinon'; -import { ActionButton } from '@spectrum-web-components/action-button'; -import { sendKeys } from '@web/test-runner-commands'; -import { Button } from '@spectrum-web-components/button'; +import '@spectrum-web-components/overlay/overlay-trigger.js'; +import '@spectrum-web-components/popover/sp-popover.js'; +import { Theme } from '@spectrum-web-components/theme'; import '@spectrum-web-components/theme/sp-theme.js'; import '@spectrum-web-components/theme/src/themes.js'; -import { TemplateResult } from '@spectrum-web-components/base'; -import { Theme } from '@spectrum-web-components/theme'; import { Tooltip } from '@spectrum-web-components/tooltip'; +import '@spectrum-web-components/tooltip/sp-tooltip.js'; +import { sendKeys } from '@web/test-runner-commands'; +import { spy } from 'sinon'; import { fixture, ignoreResizeObserverLoopError, } from '../../../test/testing-helpers.js'; +// This should be moved to Overlay/ shared +export const SAFARI_FOCUS_RING_CLASS = 'remove-focus-ring-safari-hack'; + ignoreResizeObserverLoopError(before, after); async function styledFixture( From 8385006dca36fba56b1fbcc881dcc912cd8b5259 Mon Sep 17 00:00:00 2001 From: Mizga Ionut-Alexandru Date: Thu, 7 Nov 2024 12:37:21 +0200 Subject: [PATCH 02/12] chore: revert lint changes --- .../test/overlay-trigger-hover.test.ts | 23 ++++++++----------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/packages/overlay/test/overlay-trigger-hover.test.ts b/packages/overlay/test/overlay-trigger-hover.test.ts index 3f2eb3a6a6..dab32d9bea 100644 --- a/packages/overlay/test/overlay-trigger-hover.test.ts +++ b/packages/overlay/test/overlay-trigger-hover.test.ts @@ -17,31 +17,28 @@ import { oneEvent, waitUntil, } from '@open-wc/testing'; -import { ActionButton } from '@spectrum-web-components/action-button'; -import '@spectrum-web-components/action-button/sp-action-button.js'; -import { TemplateResult } from '@spectrum-web-components/base'; -import { Button } from '@spectrum-web-components/button'; +import '@spectrum-web-components/overlay/overlay-trigger.js'; +import '@spectrum-web-components/popover/sp-popover.js'; import '@spectrum-web-components/button/sp-button.js'; +import '@spectrum-web-components/tooltip/sp-tooltip.js'; import '@spectrum-web-components/dialog/sp-dialog-wrapper.js'; +import '@spectrum-web-components/action-button/sp-action-button.js'; import '@spectrum-web-components/icons-workflow/icons/sp-icon-magnify.js'; import { OverlayTrigger } from '@spectrum-web-components/overlay'; -import '@spectrum-web-components/overlay/overlay-trigger.js'; -import '@spectrum-web-components/popover/sp-popover.js'; -import { Theme } from '@spectrum-web-components/theme'; +import { spy } from 'sinon'; +import { ActionButton } from '@spectrum-web-components/action-button'; +import { sendKeys } from '@web/test-runner-commands'; +import { Button } from '@spectrum-web-components/button'; import '@spectrum-web-components/theme/sp-theme.js'; import '@spectrum-web-components/theme/src/themes.js'; +import { TemplateResult } from '@spectrum-web-components/base'; +import { Theme } from '@spectrum-web-components/theme'; import { Tooltip } from '@spectrum-web-components/tooltip'; -import '@spectrum-web-components/tooltip/sp-tooltip.js'; -import { sendKeys } from '@web/test-runner-commands'; -import { spy } from 'sinon'; import { fixture, ignoreResizeObserverLoopError, } from '../../../test/testing-helpers.js'; -// This should be moved to Overlay/ shared -export const SAFARI_FOCUS_RING_CLASS = 'remove-focus-ring-safari-hack'; - ignoreResizeObserverLoopError(before, after); async function styledFixture( From fc66d6b0391d1121cca43d76f4583465598fb61b Mon Sep 17 00:00:00 2001 From: Mizga Ionut-Alexandru Date: Thu, 14 Nov 2024 10:17:44 +0200 Subject: [PATCH 03/12] test: uncomment --- packages/overlay/test/overlay-trigger-hover-click.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/overlay/test/overlay-trigger-hover-click.test.ts b/packages/overlay/test/overlay-trigger-hover-click.test.ts index 6e9e09039e..8d068eab1e 100644 --- a/packages/overlay/test/overlay-trigger-hover-click.test.ts +++ b/packages/overlay/test/overlay-trigger-hover-click.test.ts @@ -317,7 +317,7 @@ describe('Overlay Trigger - Hover and Click', () => { // This fails but when manually tested it works in the browser // in the test it shows that the tooltip is open (overlayTrigger.open === 'hover') - // expect(overlayTrigger.open).to.be.undefined; + expect(overlayTrigger.open).to.be.undefined; expect(document.activeElement === trigger, 'trigger focused').to.be .true; if (isWebKit()) @@ -349,7 +349,7 @@ describe('Overlay Trigger - Hover and Click', () => { await closed; // Manually testing this in the browser doesn't fail without the handleKeyup method but it should - // expect(overlayTrigger.open).to.equal('hover'); + expect(overlayTrigger.open).to.equal('hover'); expect(document.activeElement === trigger, 'trigger focused').to.be .true; if (isWebKit()) From ee86368a605e45b98e75ccdcf34473be92fff5c2 Mon Sep 17 00:00:00 2001 From: Mizga Ionut-Alexandru Date: Thu, 14 Nov 2024 14:08:57 +0200 Subject: [PATCH 04/12] chore: moved SAFARI_FOCUS_RING_CLASS declaration --- packages/action-menu/test/index.ts | 2 +- packages/overlay/src/HoverController.ts | 3 +-- packages/overlay/src/InteractionController.ts | 1 + packages/overlay/test/overlay-trigger-hover-click.test.ts | 2 +- packages/picker/src/InteractionController.ts | 1 + packages/picker/src/MobileController.ts | 3 +-- packages/picker/test/index.ts | 2 +- 7 files changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/action-menu/test/index.ts b/packages/action-menu/test/index.ts index 133a1a1551..092b213098 100644 --- a/packages/action-menu/test/index.ts +++ b/packages/action-menu/test/index.ts @@ -41,7 +41,7 @@ import type { Overlay } from '@spectrum-web-components/overlay'; import { sendKeys, setViewport } from '@web/test-runner-commands'; import { TemplateResult } from '@spectrum-web-components/base'; import { isWebKit } from '@spectrum-web-components/shared'; -import { SAFARI_FOCUS_RING_CLASS } from '@spectrum-web-components/picker/src/MobileController.js'; +import { SAFARI_FOCUS_RING_CLASS } from '@spectrum-web-components/picker/src/InteractionController.js'; ignoreResizeObserverLoopError(before, after); diff --git a/packages/overlay/src/HoverController.ts b/packages/overlay/src/HoverController.ts index 0c5da7097f..91509129a5 100644 --- a/packages/overlay/src/HoverController.ts +++ b/packages/overlay/src/HoverController.ts @@ -18,10 +18,9 @@ import { InteractionController, InteractionTypes, lastInteractionType, + SAFARI_FOCUS_RING_CLASS, } from './InteractionController.js'; -export const SAFARI_FOCUS_RING_CLASS = 'remove-focus-ring-safari-hack'; - const HOVER_DELAY = 300; export class HoverController extends InteractionController { diff --git a/packages/overlay/src/InteractionController.ts b/packages/overlay/src/InteractionController.ts index 3584c3140c..5edd385303 100644 --- a/packages/overlay/src/InteractionController.ts +++ b/packages/overlay/src/InteractionController.ts @@ -20,6 +20,7 @@ export enum InteractionTypes { } export const lastInteractionType = Symbol('lastInteractionType'); +export const SAFARI_FOCUS_RING_CLASS = 'remove-focus-ring-safari-hack'; export type ControllerOptions = { overlay?: AbstractOverlay; diff --git a/packages/overlay/test/overlay-trigger-hover-click.test.ts b/packages/overlay/test/overlay-trigger-hover-click.test.ts index 8d068eab1e..7e63cda822 100644 --- a/packages/overlay/test/overlay-trigger-hover-click.test.ts +++ b/packages/overlay/test/overlay-trigger-hover-click.test.ts @@ -38,7 +38,7 @@ import { Tooltip } from '@spectrum-web-components/tooltip/src/Tooltip.js'; import { sendKeys } from '@web/test-runner-commands'; import { Button } from '@spectrum-web-components/button'; import { isWebKit } from '@spectrum-web-components/shared'; -import { SAFARI_FOCUS_RING_CLASS } from '../src/HoverController.js'; +import { SAFARI_FOCUS_RING_CLASS } from '@spectrum-web-components/overlay/src/InteractionController.js'; ignoreResizeObserverLoopError(before, after); diff --git a/packages/picker/src/InteractionController.ts b/packages/picker/src/InteractionController.ts index 8f8865c6fd..15a70c9d79 100644 --- a/packages/picker/src/InteractionController.ts +++ b/packages/picker/src/InteractionController.ts @@ -22,6 +22,7 @@ export enum InteractionTypes { 'desktop', 'mobile', } +export const SAFARI_FOCUS_RING_CLASS = 'remove-focus-ring-safari-hack'; export class InteractionController implements ReactiveController { abortController!: AbortController; diff --git a/packages/picker/src/MobileController.ts b/packages/picker/src/MobileController.ts index 1f8a374cd2..0599d6d792 100644 --- a/packages/picker/src/MobileController.ts +++ b/packages/picker/src/MobileController.ts @@ -12,11 +12,10 @@ governing permissions and limitations under the License. import { InteractionController, InteractionTypes, + SAFARI_FOCUS_RING_CLASS, } from './InteractionController.js'; import { isWebKit } from '@spectrum-web-components/shared'; -export const SAFARI_FOCUS_RING_CLASS = 'remove-focus-ring-safari-hack'; - export class MobileController extends InteractionController { override type = InteractionTypes.mobile; diff --git a/packages/picker/test/index.ts b/packages/picker/test/index.ts index 6ffff1f840..94a35f2bab 100644 --- a/packages/picker/test/index.ts +++ b/packages/picker/test/index.ts @@ -62,7 +62,7 @@ import type { Menu } from '@spectrum-web-components/menu'; import { Tooltip } from '@spectrum-web-components/tooltip'; import { FieldLabel } from '@spectrum-web-components/field-label/src/FieldLabel.js'; import { isWebKit } from '@spectrum-web-components/shared'; -import { SAFARI_FOCUS_RING_CLASS } from '@spectrum-web-components/picker/src/MobileController.js'; +import { SAFARI_FOCUS_RING_CLASS } from '@spectrum-web-components/picker/src/InteractionController.js'; export type TestablePicker = { optionsMenu: Menu }; From 73c9908a6acd2abac156a61e03f721536cb2e8a2 Mon Sep 17 00:00:00 2001 From: Mizga Ionut-Alexandru Date: Mon, 18 Nov 2024 08:53:47 +0200 Subject: [PATCH 05/12] fix: fixed test naming --- packages/overlay/test/overlay-trigger-hover-click.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/overlay/test/overlay-trigger-hover-click.test.ts b/packages/overlay/test/overlay-trigger-hover-click.test.ts index 7e63cda822..134049e4ef 100644 --- a/packages/overlay/test/overlay-trigger-hover-click.test.ts +++ b/packages/overlay/test/overlay-trigger-hover-click.test.ts @@ -276,7 +276,7 @@ describe('Overlay Trigger - Hover and Click', () => { expect(el.open, '"click" overlay no longer open').to.be.undefined; expect(tooltip.open).to.be.false; }); - it('should not open right after closing the click overlay using the mouse', async () => { + it('should not open hover overlay right after closing the click overlay using the mouse', async () => { const overlayTrigger = await fixture( clickAndHoverTarget() ); @@ -325,7 +325,7 @@ describe('Overlay Trigger - Hover and Click', () => { .true; }); - it('should not open right after closing the click overlay using keyboard', async () => { + it('should open hover overlay right after closing the click overlay using keyboard', async () => { const overlayTrigger = await fixture( clickAndHoverTarget() ); From 232cbf32008f2b59d23b6181ba61045b6c43ad25 Mon Sep 17 00:00:00 2001 From: Mizga Ionut-Alexandru Date: Wed, 20 Nov 2024 16:50:53 +0200 Subject: [PATCH 06/12] test: update comment --- packages/overlay/test/overlay-trigger-hover-click.test.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/overlay/test/overlay-trigger-hover-click.test.ts b/packages/overlay/test/overlay-trigger-hover-click.test.ts index 134049e4ef..966a85bc81 100644 --- a/packages/overlay/test/overlay-trigger-hover-click.test.ts +++ b/packages/overlay/test/overlay-trigger-hover-click.test.ts @@ -315,8 +315,7 @@ describe('Overlay Trigger - Hover and Click', () => { }); await closed; - // This fails but when manually tested it works in the browser - // in the test it shows that the tooltip is open (overlayTrigger.open === 'hover') + // This fails but it works when manually tested in the browser. expect(overlayTrigger.open).to.be.undefined; expect(document.activeElement === trigger, 'trigger focused').to.be .true; @@ -348,7 +347,7 @@ describe('Overlay Trigger - Hover and Click', () => { await sendKeys({ press: 'Escape' }); await closed; - // Manually testing this in the browser doesn't fail without the handleKeyup method but it should + // This fails but it works when manually tested in the browser. expect(overlayTrigger.open).to.equal('hover'); expect(document.activeElement === trigger, 'trigger focused').to.be .true; From 5af409d2c485a3a976b467eccda53aa4b99470bf Mon Sep 17 00:00:00 2001 From: Mizga Ionut-Alexandru Date: Fri, 22 Nov 2024 09:44:33 +0200 Subject: [PATCH 07/12] fix: not showing hover when closing popover with escape --- packages/overlay/src/HoverController.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/overlay/src/HoverController.ts b/packages/overlay/src/HoverController.ts index 91509129a5..95ed877440 100644 --- a/packages/overlay/src/HoverController.ts +++ b/packages/overlay/src/HoverController.ts @@ -35,7 +35,7 @@ export class HoverController extends InteractionController { pointerentered = false; handleKeyup(event: KeyboardEvent): void { - if (isWebKit() && (event.code === 'Tab' || event.code === 'Escape')) { + if (event.code === 'Tab' || event.code === 'Escape') { this.open = true; this.removeSafariFocusRingClass(); } From 6f4fca8899248d009c234ad230f14de8259bd754 Mon Sep 17 00:00:00 2001 From: Mizga Ionut-Alexandru Date: Mon, 13 Jan 2025 16:28:58 +0200 Subject: [PATCH 08/12] chore: remove flaky assertion --- packages/overlay/test/overlay-trigger-hover-click.test.ts | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/packages/overlay/test/overlay-trigger-hover-click.test.ts b/packages/overlay/test/overlay-trigger-hover-click.test.ts index 966a85bc81..ff0b26bf18 100644 --- a/packages/overlay/test/overlay-trigger-hover-click.test.ts +++ b/packages/overlay/test/overlay-trigger-hover-click.test.ts @@ -276,7 +276,7 @@ describe('Overlay Trigger - Hover and Click', () => { expect(el.open, '"click" overlay no longer open').to.be.undefined; expect(tooltip.open).to.be.false; }); - it('should not open hover overlay right after closing the click overlay using the mouse', async () => { + it('should not show focus indicator styles right after closing the click overlay using the mouse', async () => { const overlayTrigger = await fixture( clickAndHoverTarget() ); @@ -315,8 +315,6 @@ describe('Overlay Trigger - Hover and Click', () => { }); await closed; - // This fails but it works when manually tested in the browser. - expect(overlayTrigger.open).to.be.undefined; expect(document.activeElement === trigger, 'trigger focused').to.be .true; if (isWebKit()) @@ -324,7 +322,7 @@ describe('Overlay Trigger - Hover and Click', () => { .true; }); - it('should open hover overlay right after closing the click overlay using keyboard', async () => { + it('should show focus indicator styles right after closing the click overlay using keyboard', async () => { const overlayTrigger = await fixture( clickAndHoverTarget() ); @@ -347,8 +345,6 @@ describe('Overlay Trigger - Hover and Click', () => { await sendKeys({ press: 'Escape' }); await closed; - // This fails but it works when manually tested in the browser. - expect(overlayTrigger.open).to.equal('hover'); expect(document.activeElement === trigger, 'trigger focused').to.be .true; if (isWebKit()) From fbde3104ed9e0da306aee56f0721c0fb8294f159 Mon Sep 17 00:00:00 2001 From: Mizga Ionut-Alexandru Date: Tue, 14 Jan 2025 09:20:45 +0200 Subject: [PATCH 09/12] chore: revert tests --- .../test/overlay-trigger-hover-click.test.ts | 83 +------------------ 1 file changed, 1 insertion(+), 82 deletions(-) diff --git a/packages/overlay/test/overlay-trigger-hover-click.test.ts b/packages/overlay/test/overlay-trigger-hover-click.test.ts index ff0b26bf18..889a7d2029 100644 --- a/packages/overlay/test/overlay-trigger-hover-click.test.ts +++ b/packages/overlay/test/overlay-trigger-hover-click.test.ts @@ -28,17 +28,11 @@ import { TriggerInteractions } from '@spectrum-web-components/overlay/src/overla import '@spectrum-web-components/overlay/overlay-trigger.js'; import { ActionButton } from '@spectrum-web-components/action-button'; import { sendMouse } from '../../../test/plugins/browser.js'; -import { - clickAndHoverTarget, - clickAndHoverTargets, - deep, -} from '../stories/overlay.stories.js'; +import { clickAndHoverTargets, deep } from '../stories/overlay.stories.js'; import { ignoreResizeObserverLoopError } from '../../../test/testing-helpers.js'; import { Tooltip } from '@spectrum-web-components/tooltip/src/Tooltip.js'; import { sendKeys } from '@web/test-runner-commands'; import { Button } from '@spectrum-web-components/button'; -import { isWebKit } from '@spectrum-web-components/shared'; -import { SAFARI_FOCUS_RING_CLASS } from '@spectrum-web-components/overlay/src/InteractionController.js'; ignoreResizeObserverLoopError(before, after); @@ -276,79 +270,4 @@ describe('Overlay Trigger - Hover and Click', () => { expect(el.open, '"click" overlay no longer open').to.be.undefined; expect(tooltip.open).to.be.false; }); - it('should not show focus indicator styles right after closing the click overlay using the mouse', async () => { - const overlayTrigger = await fixture( - clickAndHoverTarget() - ); - - await elementUpdated(overlayTrigger); - expect(overlayTrigger.open).to.be.undefined; - - const trigger = overlayTrigger.querySelector( - 'sp-button[slot="trigger"]' - ) as Button; - const rect = trigger.getBoundingClientRect(); - const opened = oneEvent(trigger, 'sp-opened'); - sendMouse({ - steps: [ - { - type: 'click', - position: [ - rect.left + rect.width / 2, - rect.top + rect.height / 2, - ], - }, - ], - }); - await opened; - - expect(overlayTrigger.open).to.equal('click'); - - const closed = oneEvent(trigger, 'sp-closed'); - sendMouse({ - steps: [ - { - type: 'click', - position: [0, 0], - }, - ], - }); - await closed; - - expect(document.activeElement === trigger, 'trigger focused').to.be - .true; - if (isWebKit()) - expect(trigger.classList.contains(SAFARI_FOCUS_RING_CLASS)).to.be - .true; - }); - - it('should show focus indicator styles right after closing the click overlay using keyboard', async () => { - const overlayTrigger = await fixture( - clickAndHoverTarget() - ); - - await elementUpdated(overlayTrigger); - expect(overlayTrigger.open).to.be.undefined; - - const trigger = overlayTrigger.querySelector( - 'sp-button[slot="trigger"]' - ) as Button; - - await sendKeys({ press: 'Tab' }); - expect(document.activeElement === trigger, 'trigger focused').to.be - .true; - - const opened = oneEvent(trigger, 'sp-opened'); - await sendKeys({ press: 'Enter' }); - await opened; - const closed = oneEvent(trigger, 'sp-closed'); - await sendKeys({ press: 'Escape' }); - await closed; - - expect(document.activeElement === trigger, 'trigger focused').to.be - .true; - if (isWebKit()) - expect(trigger.classList.contains(SAFARI_FOCUS_RING_CLASS)).to.be - .false; - }); }); From 64601f378f3c3181ea3a0c1536d8c5820ce31294 Mon Sep 17 00:00:00 2001 From: TaraT Date: Tue, 14 Jan 2025 14:31:09 +0530 Subject: [PATCH 10/12] chore(overlay): add test for focus-ring consistency across browsers (#5021) --- packages/overlay/test/overlay.test.ts | 72 +++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/packages/overlay/test/overlay.test.ts b/packages/overlay/test/overlay.test.ts index 44214be15f..12e3ee0ccd 100644 --- a/packages/overlay/test/overlay.test.ts +++ b/packages/overlay/test/overlay.test.ts @@ -34,6 +34,7 @@ import { } from '@open-wc/testing'; import { sendKeys } from '@web/test-runner-commands'; import { + clickAndHoverTarget, definedOverlayElement, virtualElement, } from '../stories/overlay.stories'; @@ -50,6 +51,9 @@ import { isOnTopLayer, } from '../../../test/testing-helpers.js'; import { Menu } from '@spectrum-web-components/menu'; +import { Button } from '@spectrum-web-components/button'; +import { isWebKit } from '@spectrum-web-components/shared'; +import { SAFARI_FOCUS_RING_CLASS } from '@spectrum-web-components/overlay/src/InteractionController.js'; async function styledFixture( story: TemplateResult @@ -786,6 +790,74 @@ describe('Overlay - type="modal"', () => { await close; expect(el.open).to.be.null; }); + + describe.only('maintains consistency of focus ring in different browsers', () => { + it('should not open hover overlay right after closing the click overlay using the mouse', async () => { + const overlayTrigger = await fixture( + clickAndHoverTarget() + ); + + await elementUpdated(overlayTrigger); + expect(overlayTrigger.open).to.be.undefined; + + const trigger = overlayTrigger.querySelector( + 'sp-button[slot="trigger"]' + ) as Button; + + const opened = oneEvent(trigger, 'sp-opened'); + trigger.click(); + await opened; + + expect(overlayTrigger.open).to.equal('click'); + + const closed = oneEvent(trigger, 'sp-closed'); + sendMouse({ + steps: [ + { + type: 'click', + position: [1, 1], + }, + ], + }); + await closed; + + expect(overlayTrigger.open).to.be.undefined; + expect(document.activeElement === trigger, 'trigger focused').to.be + .true; + if (isWebKit()) + expect(trigger.classList.contains(SAFARI_FOCUS_RING_CLASS)).to + .be.true; + }); + + it('should not open hover overlay right after closing the click overlay using the keyboard', async () => { + const overlayTrigger = await fixture( + clickAndHoverTarget() + ); + + const trigger = overlayTrigger.querySelector( + 'sp-button[slot="trigger"]' + ) as Button; + + const opened = oneEvent(trigger, 'sp-opened'); + trigger.click(); + await opened; + + expect(overlayTrigger.open).to.equal('click'); + + const closed = oneEvent(trigger, 'sp-closed'); + sendKeys({ + press: 'Escape', + }); + await closed; + + expect(overlayTrigger.open).to.be.undefined; + expect(document.activeElement === trigger, 'trigger focused').to.be + .true; + if (isWebKit()) + expect(trigger.classList.contains(SAFARI_FOCUS_RING_CLASS)).to + .be.true; + }); + }); }); describe('Overlay - timing', () => { it('manages multiple modals in a row without preventing them from closing', async () => { From bbeb207835e92fc7971907e3bf72b4e5054bca2d Mon Sep 17 00:00:00 2001 From: Mizga Ionut-Alexandru Date: Tue, 14 Jan 2025 12:35:38 +0200 Subject: [PATCH 11/12] chore: remove .only --- packages/overlay/test/overlay.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/overlay/test/overlay.test.ts b/packages/overlay/test/overlay.test.ts index 12e3ee0ccd..caecf4f214 100644 --- a/packages/overlay/test/overlay.test.ts +++ b/packages/overlay/test/overlay.test.ts @@ -791,7 +791,7 @@ describe('Overlay - type="modal"', () => { expect(el.open).to.be.null; }); - describe.only('maintains consistency of focus ring in different browsers', () => { + describe('maintains consistency of focus ring in different browsers', () => { it('should not open hover overlay right after closing the click overlay using the mouse', async () => { const overlayTrigger = await fixture( clickAndHoverTarget() From 4b05ae8d07f04c8567a5cfe51ac4bb897ddcdd91 Mon Sep 17 00:00:00 2001 From: TarunAdobe Date: Mon, 27 Jan 2025 16:01:11 +0530 Subject: [PATCH 12/12] chore: separate test for focus-ring in overlay --- packages/overlay/test/overlay.test.ts | 148 +++++++++++++++++--------- 1 file changed, 95 insertions(+), 53 deletions(-) diff --git a/packages/overlay/test/overlay.test.ts b/packages/overlay/test/overlay.test.ts index caecf4f214..6b6146afa5 100644 --- a/packages/overlay/test/overlay.test.ts +++ b/packages/overlay/test/overlay.test.ts @@ -791,73 +791,66 @@ describe('Overlay - type="modal"', () => { expect(el.open).to.be.null; }); - describe('maintains consistency of focus ring in different browsers', () => { - it('should not open hover overlay right after closing the click overlay using the mouse', async () => { - const overlayTrigger = await fixture( - clickAndHoverTarget() - ); - - await elementUpdated(overlayTrigger); - expect(overlayTrigger.open).to.be.undefined; + it('should not open hover overlay right after closing the click overlay using the mouse', async () => { + const overlayTrigger = await fixture( + clickAndHoverTarget() + ); - const trigger = overlayTrigger.querySelector( - 'sp-button[slot="trigger"]' - ) as Button; + await elementUpdated(overlayTrigger); + expect(overlayTrigger.open).to.be.undefined; - const opened = oneEvent(trigger, 'sp-opened'); - trigger.click(); - await opened; + const trigger = overlayTrigger.querySelector( + 'sp-button[slot="trigger"]' + ) as Button; - expect(overlayTrigger.open).to.equal('click'); + const opened = oneEvent(trigger, 'sp-opened'); + trigger.click(); + await opened; - const closed = oneEvent(trigger, 'sp-closed'); - sendMouse({ - steps: [ - { - type: 'click', - position: [1, 1], - }, - ], - }); - await closed; + expect(overlayTrigger.open).to.equal('click'); - expect(overlayTrigger.open).to.be.undefined; - expect(document.activeElement === trigger, 'trigger focused').to.be - .true; - if (isWebKit()) - expect(trigger.classList.contains(SAFARI_FOCUS_RING_CLASS)).to - .be.true; + const closed = oneEvent(trigger, 'sp-closed'); + sendMouse({ + steps: [ + { + type: 'click', + position: [1, 1], + }, + ], }); + await closed; - it('should not open hover overlay right after closing the click overlay using the keyboard', async () => { - const overlayTrigger = await fixture( - clickAndHoverTarget() - ); + expect(overlayTrigger.open).to.be.undefined; + expect(document.activeElement === trigger, 'trigger focused').to.be + .true; + }); - const trigger = overlayTrigger.querySelector( - 'sp-button[slot="trigger"]' - ) as Button; + it('should not open hover overlay right after closing the click overlay using the keyboard', async () => { + const overlayTrigger = await fixture( + clickAndHoverTarget() + ); - const opened = oneEvent(trigger, 'sp-opened'); - trigger.click(); - await opened; + const trigger = overlayTrigger.querySelector( + 'sp-button[slot="trigger"]' + ) as Button; - expect(overlayTrigger.open).to.equal('click'); + const opened = oneEvent(trigger, 'sp-opened'); + trigger.click(); + await opened; - const closed = oneEvent(trigger, 'sp-closed'); - sendKeys({ - press: 'Escape', - }); - await closed; + expect(overlayTrigger.open).to.equal('click'); - expect(overlayTrigger.open).to.be.undefined; - expect(document.activeElement === trigger, 'trigger focused').to.be - .true; - if (isWebKit()) - expect(trigger.classList.contains(SAFARI_FOCUS_RING_CLASS)).to - .be.true; + const closed = oneEvent(trigger, 'sp-closed'); + sendKeys({ + press: 'Escape', }); + await closed; + + expect(overlayTrigger.open).to.be.undefined; + expect(document.activeElement === trigger, 'trigger focused').to.be + .true; }); + }); describe('Overlay - timing', () => { it('manages multiple modals in a row without preventing them from closing', async () => { @@ -976,3 +969,52 @@ describe('Overlay - timing', () => { expect(overlayTrigger2.hasAttribute('open')).to.be.false; }); }); + +describe('maintains focus consistency in webkit', () => { + it('should apply remove-focus-ring class in webkit when focus happens after click', async () => { + + if (!isWebKit()){ + return; + } + + const overlayTrigger = await fixture( + clickAndHoverTarget() + ); + await elementUpdated(overlayTrigger); + expect(overlayTrigger.open).to.be.undefined; + const trigger = overlayTrigger.querySelector( + 'sp-button[slot="trigger"]' + ) as Button; + + const boundingRect = trigger.getBoundingClientRect(); + + const opened = oneEvent(trigger, 'sp-opened'); + await sendMouse({ + steps: [ + { + type: 'click', + position: [ + boundingRect.left + boundingRect.width / 2, + boundingRect.top + boundingRect.height / 2, + ], + }, + ], + }); + await opened; + + expect(overlayTrigger.open).to.equal('click'); + + const closed = oneEvent(trigger, 'sp-closed'); + await sendMouse({ + steps: [ + { + type: 'click', + position: [0, 0], + }, + ], + }); + await closed; + + expect(trigger.classList.contains(SAFARI_FOCUS_RING_CLASS)).to.be.true; + }); +}); \ No newline at end of file