fix: add delay to overlay-trigger #3748

AndreiBaicu26 · 2023-10-26T10:44:02Z

This PR adds the delay capability back to overlay-trigger

Motivation and context

Adding back the capability of delayed overlays.

How has this been tested?

Test case 1
Inside storybook created a button with a delayed tooltip through overlay-trigger.
Test case 2
Called the Overlay.open() method to test backwards compatibility.

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
If my change required a change to the documentation, I have updated the documentation in this pull request.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.
I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

github-actions · 2023-10-26T10:47:41Z

Branch Preview

Visual regression test results

When a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:

github-actions · 2023-10-26T11:19:54Z

Tachometer results

Chrome

action-bar permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	486 kB	149.62ms - 155.59ms	-	unsure 🔍 -2% - +3% -3.43ms - +4.75ms
branch	473 kB	149.15ms - 154.74ms	unsure 🔍 -3% - +2% -4.75ms - +3.43ms	-

action-menu permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	639 kB	305.54ms - 312.56ms	-	unsure 🔍 -2% - +2% -5.55ms - +4.97ms
branch	626 kB	305.43ms - 313.25ms	unsure 🔍 -2% - +2% -4.97ms - +5.55ms	-

menu permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	461 kB	426.39ms - 438.80ms	-	slower ❌ 1% - 5% 4.39ms - 22.00ms
branch	448 kB	413.15ms - 425.65ms	faster ✔ 1% - 5% 4.39ms - 22.00ms	-

overlay permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	487 kB	97.34ms - 101.76ms	-	unsure 🔍 -4% - +2% -3.53ms - +2.48ms
branch	489 kB	98.04ms - 102.11ms	unsure 🔍 -3% - +4% -2.48ms - +3.53ms	-

picker permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	505 kB	1023.34ms - 1049.08ms	-	unsure 🔍 -2% - +2% -17.42ms - +17.23ms
branch	492 kB	1024.71ms - 1047.90ms	unsure 🔍 -2% - +2% -17.23ms - +17.42ms	-

popover permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	383 kB	38.74ms - 40.61ms	-	unsure 🔍 -3% - +3% -1.26ms - +1.17ms
branch	369 kB	38.94ms - 40.50ms	unsure 🔍 -3% - +3% -1.17ms - +1.26ms	-

split-button permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	710 kB	1907.96ms - 1912.00ms	-	unsure 🔍 -0% - +0% -2.22ms - +3.90ms
branch	698 kB	1906.84ms - 1911.44ms	unsure 🔍 -0% - +0% -3.90ms - +2.22ms	-

tooltip permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	555 kB	83.62ms - 86.35ms	-	slower ❌ 1% - 6% 0.89ms - 4.53ms
branch	563 kB	81.06ms - 83.48ms	faster ✔ 1% - 5% 0.89ms - 4.53ms	-

Firefox

action-bar permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	486 kB	292.81ms - 301.19ms	-	faster ✔ 5% - 9% 15.63ms - 28.17ms
branch	473 kB	314.24ms - 323.56ms	slower ❌ 5% - 10% 15.63ms - 28.17ms	-

action-menu permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	639 kB	425.33ms - 440.95ms	-	unsure 🔍 -2% - +2% -10.08ms - +9.48ms
branch	626 kB	427.56ms - 439.32ms	unsure 🔍 -2% - +2% -9.48ms - +10.08ms	-

menu permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	461 kB	627.53ms - 636.31ms	-	faster ✔ 0% - 2% 0.32ms - 14.80ms
branch	448 kB	633.72ms - 645.24ms	slower ❌ 0% - 2% 0.32ms - 14.80ms	-

overlay permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	579 kB	186.83ms - 192.61ms	-	faster ✔ 18% - 24% 42.11ms - 58.29ms
branch	566 kB	232.37ms - 247.47ms	slower ❌ 22% - 31% 42.11ms - 58.29ms	-

picker permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	505 kB	1108.12ms - 1123.92ms	-	unsure 🔍 -2% - +1% -17.79ms - +7.95ms
branch	492 kB	1110.78ms - 1131.10ms	unsure 🔍 -1% - +2% -7.95ms - +17.79ms	-

popover permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	383 kB	109.93ms - 124.75ms	-	faster ✔ 9% - 23% 11.13ms - 33.07ms
branch	369 kB	131.35ms - 147.53ms	slower ❌ 9% - 29% 11.13ms - 33.07ms	-

split-button permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	710 kB	1614.72ms - 1621.60ms	-	unsure 🔍 -0% - +0% -4.66ms - +5.58ms
branch	698 kB	1613.90ms - 1621.50ms	unsure 🔍 -0% - +0% -5.58ms - +4.66ms	-

tooltip permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	644 kB	163.35ms - 166.65ms	-	unsure 🔍 -1% - +1% -2.16ms - +1.92ms
branch	631 kB	163.92ms - 166.32ms	unsure 🔍 -1% - +1% -1.92ms - +2.16ms	-

Westbrook · 2023-10-26T12:17:44Z

I think we could centralize this change by updating https://github.com/adobe/spectrum-web-components/blob/main/packages/overlay/src/Overlay.ts#L97-L98 to be a getter/setter and have the getter check the elements array for hasAttribute('delayed') and surface this capability for all Overlays, not just those that are powered by <overlay-trigger>. Thoughts?

AndreiBaicu26 · 2023-10-26T13:50:43Z

@Westbrook Something like the new refactoring ? 👀

packages/overlay/src/Overlay.ts

Westbrook · 2023-10-27T18:06:51Z

packages/overlay/src/Overlay.ts

+            return true;
+        }
+
+        return this._delayed;


Is this all enough the same as return this.elements.at(-1)?.hasAttribute('delayed') ?? this._delayed;?

Almost, changed to this.elements.at(-1)?.hasAttribute('delayed') || this._delayed;

Westbrook · 2023-10-30T13:04:23Z

It would be nice if we could add a test for this. Maybe there's room to add a little time checking between the hover and open events in https://github.com/adobe/spectrum-web-components/blob/main/packages/overlay/test/overlay-trigger-hover-click.test.ts#L92-L111 or so. Probably can go without blocking on this, but maybe you've got an idea on testing this other wise?

After that, I think we're just missing some changes from main and this will be good to go. Can you catch this up to that branch?

AndreiBaicu26 · 2023-10-31T13:45:12Z

@Westbrook Rebased with main + added a test for delay on click ✅

Westbrook

Looking good! Tiny refactor in the test for simplicity, and then let's ship it!

Westbrook · 2023-10-31T21:07:42Z

packages/overlay/test/overlay-trigger-click.test.ts

+        el.setAttribute('open', 'click');
+        await waitUntil(
+            () => openedSpy.calledOnce,
+            'hover content projected to overlay',
+            { timeout: 8000 }
+        );


Suggested change

el.setAttribute('open', 'click');

await waitUntil(

() => openedSpy.calledOnce,

'hover content projected to overlay',

{ timeout: 8000 }

);

const start = performance.now();

const opened = oneEvent(el, 'sp-opened');

el.setAttribute('open', 'click');

await opened;

If you structure the test like this you can get away without either of the spies and can be sure to only test the open workflow, rather than any delay that might come from the generation of the fixture.

Westbrook

LGTM! Thanks for bringing this to our attention and finding such a clean fix. 🙇🏼

Westbrook reviewed Oct 27, 2023

View reviewed changes

AndreiBaicu26 marked this pull request as ready for review October 30, 2023 11:58

AndreiBaicu26 added 5 commits October 31, 2023 15:43

fix: add delay to overlay-trigger

d376411

refactor: add delay logic to overlay.ts

7915d06

refactor: add delay private property

60bc5f5

refactor: update delay condition

b749417

test: add test for delay capability

b2b11d1

AndreiBaicu26 force-pushed the abaicu/add-delay branch from 70d1038 to b2b11d1 Compare October 31, 2023 13:43

Westbrook reviewed Oct 31, 2023

View reviewed changes

refactor: tests

32448d2

Westbrook approved these changes Nov 1, 2023

View reviewed changes

Westbrook merged commit 5c4f1f6 into main Nov 1, 2023
7 of 8 checks passed

Westbrook deleted the abaicu/add-delay branch November 1, 2023 12:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add delay to overlay-trigger #3748

fix: add delay to overlay-trigger #3748

AndreiBaicu26 commented Oct 26, 2023 •

edited

Loading

github-actions bot commented Oct 26, 2023

github-actions bot commented Oct 26, 2023 •

edited

Loading

action-bar permalink

action-menu permalink

menu permalink

overlay permalink

picker permalink

popover permalink

split-button permalink

tooltip permalink

action-bar permalink

action-menu permalink

menu permalink

overlay permalink

picker permalink

popover permalink

split-button permalink

tooltip permalink

Westbrook commented Oct 26, 2023

AndreiBaicu26 commented Oct 26, 2023 •

edited

Loading

Westbrook Oct 27, 2023

AndreiBaicu26 Oct 30, 2023

Westbrook commented Oct 30, 2023

AndreiBaicu26 commented Oct 31, 2023

Westbrook left a comment

Westbrook Oct 31, 2023

Westbrook left a comment

fix: add delay to overlay-trigger #3748

fix: add delay to overlay-trigger #3748

Conversation

AndreiBaicu26 commented Oct 26, 2023 • edited Loading

Motivation and context

How has this been tested?

Screenshots (if appropriate)

Types of changes

Checklist

Best practices

github-actions bot commented Oct 26, 2023

Branch Preview

github-actions bot commented Oct 26, 2023 • edited Loading

Tachometer results

action-bar permalink

action-menu permalink

menu permalink

overlay permalink

picker permalink

popover permalink

split-button permalink

tooltip permalink

action-bar permalink

action-menu permalink

menu permalink

overlay permalink

picker permalink

popover permalink

split-button permalink

tooltip permalink

Westbrook commented Oct 26, 2023

AndreiBaicu26 commented Oct 26, 2023 • edited Loading

Westbrook Oct 27, 2023

Choose a reason for hiding this comment

AndreiBaicu26 Oct 30, 2023

Choose a reason for hiding this comment

Westbrook commented Oct 30, 2023

AndreiBaicu26 commented Oct 31, 2023

Westbrook left a comment

Choose a reason for hiding this comment

Westbrook Oct 31, 2023

Choose a reason for hiding this comment

Westbrook left a comment

Choose a reason for hiding this comment

AndreiBaicu26 commented Oct 26, 2023 •

edited

Loading

github-actions bot commented Oct 26, 2023 •

edited

Loading

AndreiBaicu26 commented Oct 26, 2023 •

edited

Loading