Repository navigation
fix: finish Sheet exit once the scroll rests at the exit position - #10835
mhasanince wants to merge 2 commits into
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Seems like you need to sign the CLA |
snowystinger
left a comment
snowystinger
left a comment
There was a problem hiding this comment.
On busy CI runners (GitHub Actions, Chrome) that last report sometimes never arrives, so the overlay stays data-exiting over the page and blocks clicks, even though the sheet has already slid out.
What does this mean? Why would it never arrive? shouldn't it be guaranteed even if maybe it's a frame or two slow? It seems like this would be a huge flaw in IntersectionObserver if this is true.
| done(); | ||
| } | ||
| }; | ||
| let done = () => { |
There was a problem hiding this comment.
for easier reading, please move this declaration before where it is used
| expect(triggerOnOpenChange).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('finishes closing once the exit scroll ends, even if the sheet is never reported hidden', async () => { |
There was a problem hiding this comment.
should this test be a browser.test instead?
There was a problem hiding this comment.
Makes sense, it mocks both scrolling and the observer. Moved it to Sheet.browser.test.tsx, which drops the ratio-0 entry from a real IntersectionObserver.
|
Fair question, and I don't have a good answer. I'd expect the same, that the entry is delivered even if it's late. What we saw in Playwright traces from the failing CI runs: the sheet is gone from the screencast within ~150ms, and 5s later |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| } | ||
|
|
||
| it('finishes closing once the exit scroll ends, even if the sheet is never reported hidden', async () => { | ||
| // Drop the observer's report that the sheet has left the viewport. |
There was a problem hiding this comment.
ok yeah, so this is what i was concerned about, if we have to forcibly drop this, then we don't yet have enough information. We need more research done on why it would be dropped in your CI.
There was a problem hiding this comment.
Agreed, without a root cause this isn't worth adding. I'll close this for now and reopen if I find out why it's dropped on our CI.
Closes #10813
Summary
A closing Sheet only finishes its exit when the IntersectionObserver reports the content at ratio 0. On busy CI runners (GitHub Actions, Chrome) that last report sometimes never arrives, so the overlay stays
data-exitingover the page and blocks clicks, even though the sheet has already slid out.Fix
onExitalso resolves once the scroller firesscrollendat the exit position, or right away if it's already there. Thereact-aria-sheet-closeevent still works as before, this is only a fallback. We run the same change as a patch in our app.✅ Pull Request Checklist:
AI use: I wrote this change with Claude Code, then reviewed and tested it myself.
📝 Test Instructions:
Added a browser test in
Sheet.browser.test.tsxthat closes an entered sheet while dropping the observer's ratio-0 entry. It fails onmainand passes here in Chromium, Firefox and WebKit.To try it in a browser, wrap
IntersectionObserverso it drops entries withintersectionRatio === 0, open a Sheet, let it fully enter, then close it. Onmainthe overlay stays in the DOM withdata-exiting; on this branch it's removed.🧢 Your Project:
Kordela
🤖 Generated with Claude Code