Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions .changeset/fix-picker-overlay-scroll.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
'@spectrum-web-components/overlay': patch
'@spectrum-web-components/picker': patch
---

**Fixed** issue where picker menus inside overlays could not scroll to the bottom after selecting an item and reopening. The problem was caused by the overlay's placement calculation happening before the menu fully rendered, resulting in incorrect height measurements.

This fix ensures picker menus maintain proper scrollable height when reopened, regardless of the selected item's position.
20 changes: 10 additions & 10 deletions 1st-gen/packages/menu/src/Menu.ts
Copy link
Contributor

@caseyisonit caseyisonit Nov 13, 2025

Choose a reason for hiding this comment

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

Can you explain what the double bangs were doing and why removing them fixes this?

Copy link
Contributor Author

@Rajdeepc Rajdeepc Nov 14, 2025

Choose a reason for hiding this comment

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

This is not the fix. Apologies if this has surfaced up as the fix scenario. They're functionally the same in this context. The !! pattern is useful when you need to store or return an explicit boolean, here its not the case. No impact on the current logic or any regressions.

Original file line number Diff line number Diff line change
Expand Up @@ -103,30 +103,30 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) {

/**
* Minimum vertical movement (in pixels) required to trigger scrolling detection.
*
*
* This threshold is consistent with other components in the design system:
* - Card component uses 10px for click vs. drag detection
* - Menu component uses 10px for scroll vs. selection detection
*
*
* The 10px threshold is carefully chosen to:
* - Allow for natural finger tremor and accidental touches
* - Distinguish between intentional scroll gestures and taps
* - Provide consistent behavior across the platform
*
*
* @see {@link packages/card/src/Card.ts} for similar threshold usage
*/
private scrollThreshold = 10; // pixels

/**
* Maximum time (in milliseconds) for a movement to be considered scrolling.
*
*
* This threshold is consistent with other timing values in the design system:
* - Longpress duration: 300ms (ActionButton, LongpressController)
* - Scroll detection: 300ms (Menu component)
*
*
* Quick movements within this timeframe are likely intentional scrolls,
* while slower movements are more likely taps or selections.
*
*
* @see {@link packages/action-button/src/ActionButton.ts} for longpress duration
* @see {@link packages/overlay/src/LongpressController.ts} for longpress duration
*/
Expand Down Expand Up @@ -576,7 +576,7 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) {
* Resets the scrolling state after a short delay (100ms) to allow for
* any final touch events to be processed. This delay prevents immediate
* state changes that could interfere with the selection logic.
*
*
* The 100ms delay is consistent with the design system's approach to
* touch event handling and ensures that any final touch events or
* gesture recognition can complete before the scrolling state is reset.
Expand Down Expand Up @@ -720,7 +720,7 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) {
): MenuItem {
const diff = before ? -1 : 1;
const elements = this.rovingTabindexController?.elements || [];
const index = !!menuItem ? elements.indexOf(menuItem) : -1;
const index = menuItem ? elements.indexOf(menuItem) : -1;
let newIndex = Math.min(Math.max(0, index + diff), elements.length - 1);
while (
!this.isFocusableElement(elements[newIndex]) &&
Expand All @@ -729,7 +729,7 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) {
) {
newIndex += diff;
}
return !!this.isFocusableElement(elements[newIndex])
return this.isFocusableElement(elements[newIndex])
? (elements[newIndex] as MenuItem)
: menuItem || elements[0];
}
Expand Down Expand Up @@ -1010,7 +1010,7 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) {
}
});
}
if (!!this._updateFocus) {
if (this._updateFocus) {
this.rovingTabindexController?.focusOnItem(this._updateFocus);
this._updateFocus = undefined;
}
Expand Down
23 changes: 23 additions & 0 deletions 1st-gen/packages/overlay/src/PlacementController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
shift,
size,
} from '@floating-ui/dom';
import { isWebKit } from '@spectrum-web-components/shared';
import type { VirtualTrigger } from './VirtualTrigger.js';
import type { OpenableElement } from './overlay-types.js';
import type { Overlay } from './Overlay.js';
Expand Down Expand Up @@ -278,6 +279,28 @@ export class PlacementController implements ReactiveController {
// Wait for document fonts to be ready before computing placement.
await (document.fonts ? document.fonts.ready : Promise.resolve());

// Safari/iOS-specific fix: Add small delay for picker menus to allow scrollIntoView to complete
// Check if this is a submenu overlay (slot="submenu")
// Submenus need immediate positioning for hover responsiveness
const isSubmenu = Array.from(this.host.elements).some(
(el) => el.getAttribute?.('slot') === 'submenu'
);

// Safari-specific timing fix covered by cross-browser integration tests
/* c8 ignore start */
if (isWebKit() && !isSubmenu) {
const hasMenu = Array.from(this.host.elements).some(
(el) =>
el.tagName === 'SP-MENU' || el.querySelector?.('sp-menu')
);

if (hasMenu) {
// Wait 1 frame for Safari layout to settle after scrollIntoView
await new Promise((resolve) => requestAnimationFrame(resolve));
}
}
/* c8 ignore stop */

// Determine the flip middleware based on the type of trigger element.
const flipMiddleware = !(options.trigger instanceof HTMLElement)
? flip({
Expand Down
5 changes: 4 additions & 1 deletion 1st-gen/packages/overlay/test/overlay.test.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

we should not need to be mocking user agents in these tests, we already run them for safari, chrome, and firefox. we should be using our browser checks isWebkit() for specific browser needs in tests. this potentially is introducing an internal tool spec that you would have to be aware of to change as the browsers update

Copy link
Contributor

@caseyisonit caseyisonit Nov 13, 2025

Choose a reason for hiding this comment

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

There's also a lot of repetitive code in each test that could be moved to the beforeEach. Actually looking at this closer, these all look identical in test assertions. this functionality should already be covered in other menu tests. can you explain the purpose and need of these tests so i can better understand?

Copy link
Contributor Author

@Rajdeepc Rajdeepc Nov 14, 2025

Choose a reason for hiding this comment

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

You're correct on both fronts. These tests are an anti-pattern. I added these initially to achieve branch coverage on the Safari-specific conditional path in PlacementController.computePlacement(). If this optimisation regresses, our existing overlay positioning assertions will fail on actual WebKit, which is the signal that matters. I agree that this is giving false confidence and I just checked yes we already have a validation strategy. I have excluded the entire Safari specific block from the coverage reporting.

Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ import {
import { render, TemplateResult } from '@spectrum-web-components/base';
import { Button } from '@spectrum-web-components/button';
import { Menu } from '@spectrum-web-components/menu';
import '@spectrum-web-components/menu/sp-menu.js';
import '@spectrum-web-components/menu/sp-menu-item.js';
import { Theme } from '@spectrum-web-components/theme';
import '@spectrum-web-components/theme/sp-theme.js';
import '@spectrum-web-components/theme/src/themes.js';
Expand All @@ -54,7 +56,7 @@ import {
clickAndHoverTarget,
definedOverlayElement,
virtualElement,
} from '../stories/overlay.stories';
} from '../stories/overlay.stories.js';
// import { isWebKit } from '@spectrum-web-components/shared';

async function styledFixture<T extends Element>(
Expand Down Expand Up @@ -476,6 +478,7 @@ describe('Overlays', () => {

const initial = el.getBoundingClientRect();
trigger.updateBoundingClientRect(500, 500);
// Wait for placement computation to complete
await nextFrame();
await nextFrame();
const final = el.getBoundingClientRect();
Expand Down
2 changes: 1 addition & 1 deletion 1st-gen/packages/picker/src/Picker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1000,7 +1000,7 @@ export class Picker extends PickerBase {
);
if (!this.value || nextItem !== this.selectedItem) {
// updates picker text but does not fire change event until action is completed
if (!!nextItem) this.setValueFromItem(nextItem as MenuItem);
if (nextItem) this.setValueFromItem(nextItem as MenuItem);
}
};
}
Loading
Loading