Skip to content

Conversation

@chirokas
Copy link
Contributor

Closes #9469

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

@chirokas chirokas marked this pull request as ready for review January 18, 2026 16:28
Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, can you provide a summary of the changes you made and how it solves the problem. I'm having a little trouble understanding how this will always have something to close.

Are we able to write a unit test for this? That could help as well.

</DatePicker>
);

export const DateRangePickerExample: DateRangePickerStory = (args) => (
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1

Copy link
Member

Choose a reason for hiding this comment

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

Ah, usually I split them into two functions then, one that's the story and the other is the shareable implementation

};

let onInteractOutsideStart = (e: PointerEvent) => {
const topMostOverlay = (lastVisibleOverlay.current = visibleOverlays[visibleOverlays.length - 1]);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const topMostOverlay = (lastVisibleOverlay.current = visibleOverlays[visibleOverlays.length - 1]);
const topMostOverlay = visibleOverlays[visibleOverlays.length - 1]
lastVisibleOverlay.current = topMostOverlay;

Makes it easier to read that there are two assignments

@chirokas
Copy link
Contributor Author

@snowystinger Do you remember issue #7887? I think it has the same root cause as this one.

The problem is that useOverlay removes the topmost overlay from visibleOverlays before calling onInteractOutside. This happens when we close an overlay after selecting something. However, this causes the first overlay to trigger onClose unexpectedly.

A simple fix would be to only call onClose when the topmost overlay at onInteractOutside is the same as at onInteractOutsideStart.

Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

@snowystinger Do you remember issue #7887? I think it has the same root cause as this one.

Great call out, I'd forgotten about that one. Thanks.

Also, thanks for writing a test for it and including some more info in the comment. I think the fix makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DateRangePicker inside dismissible Modal closes Modal unexpectedly

2 participants