+
Skip to content
This repository was archived by the owner on Oct 14, 2025. It is now read-only.

Add Menu target click() support #986

Merged
merged 2 commits into from
Jul 22, 2025
Merged

Add Menu target click() support #986

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
54d363d
Add Menu target `click()` support
clintcs Jul 21, 2025
0f716fd
Apply suggestions from code review
clintcs Jul 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/good-pianos-heal.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@crowdstrike/glide-core': minor
---

Menu can no longer be opened by setting its `open` attribute via the "click" handler of an element outside Menu.
If you need to open this way, you can call `click()` on Menu's target in your handler.
This change was made to support the ability to open sub-Menu targets programmatically by calling `click()`.
Copy link
Collaborator Author

@clintcs clintcs Jul 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It was nice to be able to support setting open this way. But supporting it precluded consumers being able to call click() on sub-Menu targets.

And there's an alternative. But there's no alternative to not being able to call click() on a sub-Menu target. So, overall, a net improvement for consumers.

5 changes: 5 additions & 0 deletions .changeset/swift-ads-sell.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@crowdstrike/glide-core': patch
---

Sub-Menus of Menu can now be opened by programmatically calling `click()` on sub-Menu targets.
229 changes: 97 additions & 132 deletions src/menu.test.interactions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,13 +228,24 @@ it('remains open when its sub-Menus are opened via click', async () => {
),
);

// Replaces the usual `await aTimeout(0)`. It's not clear why. But opening Menus
// in tests sometimes takes more than a tick when sub-Menus are present.
await waitUntil(() => {
return defaultSlots[0]?.checkVisibility();
});
await aTimeout(0); // Wait for Floating UI

await click(targets[1]);
// A programmatic click instead of an actual one because `sendMouse()` (via
// `mouse.ts`) turned out to be flaky in CI.
//
// After a bunch of experimentation and source code digging, the ultimate cause of
// the `sendMouse()` flakiness isn't clear. The immediate cause seems to be
// that it clicks an Option or an element outside Menu instead of the target,
// causing Menu to close. Though it only happens when nested popovers are present
// via sub-Menus.
//
// `sendMouse()` is just a thin abstraction over Playwright's equivalent API. So
// the ultimate cause is likely Playwright or Chromium's DevTools Protocol, which
// Playwright relies on. It's that or I'm missing something obvious. Either way,
// it was time to move on.
targets[1]?.click();
Copy link
Collaborator Author

@clintcs clintcs Jul 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

As a bonus, we can now call click() in tests—which I believe will resolve most if not all of the flakiness we've seen.

Copy link
Collaborator Author

@clintcs clintcs Jul 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

That said, there still appears to be systemic issues causing sporadic flakiness of various tests, and we've been seeing for some time. The cause isn't clear to me.

I had hoped that Playwright's support for component testing would be stable by now. When it finally is (and when they support web components with it), we'll move to it and hopefully things will stabilize. Until then, there are a couple things we can try:

Chromium's new headless mode isn't quite there yet in terms of quality. Either that or it doesn't work well with Web Test Runner. But we can break out the tests out now—which probably isn't a bad idea anyway because doing so could cut our build times in half. So I'll do that soon.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks!


await aTimeout(0); // Wait for the timeout in `#onTargetSlotClick()`

expect(hosts[0]?.open).to.be.true;
expect(targets[0]?.ariaExpanded).to.equal('true');
Expand All @@ -246,13 +257,8 @@ it('remains open when its sub-Menus are opened via click', async () => {
?.getAttribute('aria-activedescendant'),
).to.equal(options[0]?.id);

// Replaces the usual `await aTimeout(0)`. It's not clear why. But opening Menus
// in tests sometimes takes more than a tick when sub-Menus are present.
await waitUntil(() => {
return defaultSlots[1]?.checkVisibility();
});

await click(targets[2]);
targets[2]?.click();
await aTimeout(0); // Wait for the timeout in `#onTargetSlotClick()`

expect(hosts[0]?.open).to.be.true;
expect(hosts[1]?.open).to.be.true;
Expand Down Expand Up @@ -1334,47 +1340,6 @@ it('does not scroll the page when already open and various keys are pressed and
expect(event.defaultPrevented).to.be.true;
});

// See the comment in `connectedCallback()` for an explanation.
it('opens when opened programmatically via the click handler of another element', async () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Menu can no longer be opened by setting its open attribute via the "click" handler of an element outside Menu. If you need to open Menu via the "click" handler of an element outside Menu, you can call click() on Menu's target. This change was made to support opening sub-Menu targets programmatically via click().

const container = document.createElement('div');

const host = await fixture<Menu>(
html`<glide-core-menu>
<button slot="target">Target</button>

<glide-core-options>
<glide-core-option label="Label"></glide-core-option>
</glide-core-options>
</glide-core-menu>`,
{ parentNode: container },
);

const target = host.querySelector('button');
const options = host.querySelectorAll('glide-core-option');

const defaultSlot = host.shadowRoot?.querySelector<HTMLSlotElement>(
'[data-test="default-slot"]',
);

const anotherElement = document.createElement('button');

anotherElement.addEventListener('click', () => (host.open = true));
anotherElement.textContent = 'Another element';

container.append(anotherElement);
await click(anotherElement);

expect(host.open).to.be.true;
expect(target?.ariaExpanded).to.equal('true');
expect(defaultSlot?.checkVisibility()).to.be.true;

expect(
host
.querySelector('glide-core-options')
?.getAttribute('aria-activedescendant'),
).to.equal(options[0]?.id);
});

it('opens its sub-Menus when its sub-Menu targets are clicked', async () => {
const host = await fixture<Menu>(
html`<glide-core-menu open>
Expand Down Expand Up @@ -1412,19 +1377,24 @@ it('opens its sub-Menus when its sub-Menu targets are clicked', async () => {
),
);

// Replaces the usual `await aTimeout(0)`. It's not clear why. But opening Menus
// in tests sometimes takes more than a tick when sub-Menus are present.
await waitUntil(() => {
return defaultSlots[0]?.checkVisibility();
});

await click(targets[1]);
await aTimeout(0); // Wait for Floating UI

// It's not clear why. But opening and positioning Menus in tests, particularly in
// CI, sometimes takes more than a tick when sub-Menus are present. 50 milliseconds
// is unfortunate. But it gives the browser and whatever is going on with the test
// runner to get sorted out.
await aTimeout(50);
// A programmatic click instead of an actual one because `sendMouse()` (via
// `mouse.ts`) turned out to be flaky in CI.
//
// After a bunch of experimentation and source code digging, the ultimate cause of
// the `sendMouse()` flakiness isn't clear. The immediate cause seems to be
// that it clicks an Option or an element outside Menu instead of the target,
// causing Menu to close. Though it only happens when nested popovers are present
// via sub-Menus.
//
// `sendMouse()` is just a thin abstraction over Playwright's equivalent API. So
// the ultimate cause is likely Playwright or Chromium's DevTools Protocol, which
// Playwright relies on. It's that or I'm missing something obvious. Either way,
// it was time to move on.
targets[1]?.click();

await aTimeout(0); // Wait for the timeout in `#onTargetSlotClick()`

expect(hosts[0]?.open).to.be.true;
expect(hosts[1]?.open).to.be.true;
Expand Down Expand Up @@ -1460,13 +1430,9 @@ it('opens its sub-Menus when its sub-Menu targets are clicked', async () => {
?.getAttribute('aria-activedescendant'),
).to.be.empty.string;

await click(targets[2]);

// It's not clear why. But opening and positioning Menus in tests, particularly in
// CI, sometimes takes more than a tick when sub-Menus are present. 50 milliseconds
// is unfortunate. But it gives the browser and whatever is going on with the test
// runner to get sorted out.
await aTimeout(50);
await aTimeout(0); // Wait for Floating UI
targets[2]?.click();
await aTimeout(0); // Wait for the timeout in `#onTargetSlotClick()`

expect(hosts[0]?.open).to.be.true;
expect(hosts[1]?.open).to.be.true;
Expand Down Expand Up @@ -1672,13 +1638,24 @@ it('closes its sub-Menus when their targets are clicked', async () => {
),
);

// It's not clear why. But opening and positioning Menus in tests, particularly in
// CI, sometimes takes more than a tick when sub-Menus are present. 50 milliseconds
// is unfortunate. But it gives the browser and whatever is going on with the test
// runner to get sorted out.
await aTimeout(50);
await aTimeout(0); // Wait for Floating UI

await click(targets[2]);
// A programmatic click instead of an actual one because `sendMouse()` (via
// `mouse.ts`) turned out to be flaky in CI.
//
// After a bunch of experimentation and source code digging, the ultimate cause of
// the `sendMouse()` flakiness isn't clear. The immediate cause seems to be
// that it clicks an Option or an element outside Menu instead of the target,
// causing Menu to close. Though it only happens when nested popovers are present
// via sub-Menus.
//
// `sendMouse()` is just a thin abstraction over Playwright's equivalent API. So
// the ultimate cause is likely Playwright or Chromium's DevTools Protocol, which
// Playwright relies on. It's that or I'm missing something obvious. Either way,
// it was time to move on.
targets[2]?.click();

await aTimeout(0); // Wait for the timeout in `#onTargetSlotClick()`

expect(hosts[0]?.open).to.be.true;
expect(hosts[1]?.open).to.be.true;
Expand Down Expand Up @@ -1714,7 +1691,9 @@ it('closes its sub-Menus when their targets are clicked', async () => {
?.getAttribute('aria-activedescendant'),
).to.be.empty.string;

await click(targets[1]);
await aTimeout(0); // Wait for Floating UI
targets[1]?.click();
await aTimeout(0); // Wait for the timeout in `#onTargetSlotClick()`

expect(hosts[0]?.open).to.be.true;
expect(hosts[1]?.open).to.be.false;
Expand Down Expand Up @@ -2927,21 +2906,26 @@ it('activates the first Option(s) of its sub-Menus they are opened via click', a
const targets = host.querySelectorAll('button');
const options = host.querySelectorAll('glide-core-option');

// It's not clear why. But opening and positioning Menus in tests, particularly in
// CI, sometimes takes more than a tick when sub-Menus are present. 50 milliseconds
// is unfortunate. But it gives the browser and whatever is going on with the test
// runner to get sorted out.
await aTimeout(50);

await click(targets[1]);
await aTimeout(0); // Wait for Floating UI

// It's not clear why. But opening and positioning Menus in tests, particularly in
// CI, sometimes takes more than a tick when sub-Menus are present. 50 milliseconds
// is unfortunate. But it gives the browser and whatever is going on with the test
// runner to get sorted out.
await aTimeout(50);
// A programmatic click instead of an actual one because `sendMouse()` (via
// `mouse.ts`) turned out to be flaky in CI.
//
// After a bunch of experimentation and source code digging, the ultimate cause of
// the `sendMouse()` flakiness isn't clear. The immediate cause seems to be
// that it clicks an Option or an element outside Menu instead of the target,
// causing Menu to close. Though it only happens when nested popovers are present
// via sub-Menus.
//
// `sendMouse()` is just a thin abstraction over Playwright's equivalent API. So
// the ultimate cause is likely Playwright or Chromium's DevTools Protocol, which
// Playwright relies on. It's that or I'm missing something obvious. Either way,
// it was time to move on.
targets[1]?.click();

await click(targets[2]);
await aTimeout(0); // Wait for Floating UI
targets[2]?.click();
await aTimeout(0); // Wait for the timeout in `#onTargetSlotClick()`

expect(options[0]?.privateActive).to.be.true;
expect(options[1]?.privateActive).to.be.true;
Expand Down Expand Up @@ -3056,7 +3040,7 @@ it('does not open the tooltip of a super-Menu Option when one of its sub-Menu Op
}
}

// Replaces the usual `await aTimeout(0)`. It's not clear why. But opening Menus
// Replaces the usual `await aTimeout(0)`. It's not clear why. But Menus opening
// in tests sometimes takes more than a tick when sub-Menus are present.
await waitUntil(() => {
return (
Expand All @@ -3069,10 +3053,8 @@ it('does not open the tooltip of a super-Menu Option when one of its sub-Menu Op
// For whatever reason, there's a significant delay between hovering and
// the subsequent "mouseover" event. The more tests, the greater the delay.
await waitUntil(() => {
return tooltips[1]?.open;
return !tooltips[0]?.open && tooltips[1]?.open;
});

expect(tooltips[0]?.open).to.be.false;
});

it('retains its active Option when a sub-Menu Option is hovered', async () => {
Expand Down Expand Up @@ -3353,34 +3335,24 @@ it('activates the next sub-Menu Option on ArrowDown after an Option of another M
<button slot="target">Target</button>

<glide-core-options>
<glide-core-option label="One ${'x'.repeat(500)}">
<glide-core-option label="One">
<glide-core-menu slot="submenu" open>
<button slot="target">Target</button>

<glide-core-options>
<glide-core-option
label="Two ${'x'.repeat(500)}"
></glide-core-option>

<glide-core-option
label="Three ${'x'.repeat(500)}"
></glide-core-option>
<glide-core-option label="Two"></glide-core-option>
<glide-core-option label="Three"></glide-core-option>
</glide-core-options>
</glide-core-menu>
</glide-core-option>

<glide-core-option label="Four ${'x'.repeat(500)}">
<glide-core-option label="Four">
<glide-core-menu slot="submenu" open>
<button slot="target">Target</button>

<glide-core-options>
<glide-core-option
label="Five ${'x'.repeat(500)}"
></glide-core-option>

<glide-core-option
label="Six ${'x'.repeat(500)}"
></glide-core-option>
<glide-core-option label="Five"></glide-core-option>
<glide-core-option label="Six"></glide-core-option>
</glide-core-options>
</glide-core-menu>
</glide-core-option>
Expand All @@ -3391,20 +3363,20 @@ it('activates the next sub-Menu Option on ArrowDown after an Option of another M
const hosts = [host, ...host.querySelectorAll('glide-core-menu')];
const options = host.querySelectorAll('glide-core-option');

const tooltips = [...options]
.map((option) => option.shadowRoot?.querySelector('[data-test="tooltip"]'))
.filter((element): element is Tooltip => element instanceof Tooltip);

for (const tooltip of tooltips) {
const element = tooltip?.shadowRoot?.querySelector('[data-test="tooltip"]');
const defaultSlots = hosts.map((host) =>
host.shadowRoot?.querySelector<HTMLSlotElement>(
'[data-test="default-slot"]',
),
);

if (element instanceof HTMLElement) {
element.dataset.closeDelay = '0';
element.dataset.openDelay = '0';
}
}
// Replaces the usual `await aTimeout(0)`. It's not clear why. But Menus opening
// in tests sometimes takes more than a tick when sub-Menus are present.
await waitUntil(() => {
return (
defaultSlots[0]?.checkVisibility() && defaultSlots[1]?.checkVisibility()
);
});

await aTimeout(0); // Wait for Floating UI
await hover(options[3]); // Four
await sendKeys({ press: 'Tab' });
await sendKeys({ press: 'ArrowDown' }); // Three
Expand All @@ -3416,13 +3388,6 @@ it('activates the next sub-Menu Option on ArrowDown after an Option of another M
expect(options[4]?.privateActive).to.be.false;
expect(options[5]?.privateActive).to.be.false;

expect(tooltips[0]?.open).to.be.false;
expect(tooltips[1]?.open).to.be.false;
expect(tooltips[2]?.open).to.be.true;
expect(tooltips[3]?.open).to.be.true;
expect(tooltips[4]?.open).to.be.false;
expect(tooltips[5]?.open).to.be.false;
Copy link
Collaborator Author

@clintcs clintcs Jul 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Flaky and nice but not necessary to assert in this test.


expect(
hosts[0]
?.querySelector('glide-core-options')
Expand Down
Loading
Loading
点击 这是indexloc提供的php浏览器服务，不要输入任何密码和下载