-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix IntersectionObserver not re-firing in Chrome after Reorder #3724
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| import { useEffect, useRef, useState } from "react" | ||
| import { Reorder } from "framer-motion" | ||
|
|
||
| interface IntersectionLog { | ||
| id: string | ||
| isIntersecting: boolean | ||
| time: number | ||
| } | ||
|
|
||
| export const App = () => { | ||
| const [items, setItems] = useState([ | ||
| "Test 1", | ||
| "Test 2", | ||
| "Test 3", | ||
| "Test 4", | ||
| "Test 5", | ||
| ]) | ||
| const [logs, setLogs] = useState<IntersectionLog[]>([]) | ||
| const containerRef = useRef<HTMLDivElement>(null) | ||
|
|
||
| useEffect(() => { | ||
| const root = containerRef.current | ||
| if (!root) return | ||
|
|
||
| const observer = new IntersectionObserver( | ||
| (entries) => { | ||
| const time = performance.now() | ||
| setLogs((prev) => [ | ||
| ...prev, | ||
| ...entries.map((entry) => ({ | ||
| id: (entry.target as HTMLElement).id, | ||
| isIntersecting: entry.isIntersecting, | ||
| time, | ||
| })), | ||
| ]) | ||
| }, | ||
| { root, threshold: 0 } | ||
| ) | ||
|
|
||
| const targets = root.querySelectorAll<HTMLElement>("[data-observed]") | ||
| targets.forEach((el) => observer.observe(el)) | ||
|
|
||
| return () => observer.disconnect() | ||
| }, []) | ||
|
|
||
| return ( | ||
| <div> | ||
| <div | ||
| ref={containerRef} | ||
| style={{ | ||
| height: 400, | ||
| overflow: "auto", | ||
| border: "1px solid #333", | ||
| }} | ||
| > | ||
| <Reorder.Group | ||
| axis="y" | ||
| values={items} | ||
| onReorder={setItems} | ||
| style={{ | ||
| listStyle: "none", | ||
| padding: 0, | ||
| margin: 0, | ||
| }} | ||
| > | ||
| {items.map((item) => ( | ||
| <Reorder.Item | ||
| key={item} | ||
| value={item} | ||
| id={item.replace(/\s/g, "-")} | ||
| data-observed | ||
| style={{ | ||
| height: 80, | ||
| padding: 10, | ||
| boxSizing: "border-box", | ||
| background: "#eef", | ||
| borderBottom: "1px solid #99c", | ||
| cursor: "grab", | ||
| }} | ||
| > | ||
| {item} | ||
| </Reorder.Item> | ||
| ))} | ||
| </Reorder.Group> | ||
| </div> | ||
| <div | ||
| id="log-state" | ||
| data-count={logs.length} | ||
| data-log={JSON.stringify(logs)} | ||
| /> | ||
| </div> | ||
| ) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| interface IntersectionLog { | ||
| id: string | ||
| isIntersecting: boolean | ||
| time: number | ||
| } | ||
|
|
||
| function readLogs(win: Window): IntersectionLog[] { | ||
| const el = win.document.getElementById("log-state") | ||
| if (!el) return [] | ||
| const raw = el.getAttribute("data-log") | ||
| return raw ? JSON.parse(raw) : [] | ||
| } | ||
|
|
||
| /** | ||
| * Regression test for https://github.com/motiondivision/motion/issues/2679 | ||
| * | ||
| * After dragging a Reorder.Item out of view and back in, the | ||
| * IntersectionObserver should fire callbacks reflecting the final visibility | ||
| * state — never leaving an item stuck reporting isIntersecting: false when it | ||
| * is actually visible. | ||
| * | ||
| * The underlying bug is a Chrome IntersectionObserver implementation quirk | ||
| * (it does not reproduce in Electron/Cypress), so this test documents the | ||
| * expected behaviour rather than reliably failing on the unfixed codebase. | ||
| */ | ||
| describe("Reorder + IntersectionObserver", () => { | ||
| it("Items report isIntersecting: true once visible after a reorder", () => { | ||
| cy.visit("?test=reorder-intersection-observer") | ||
| .wait(200) | ||
| // Drag Test-1 down past Test-2 to trigger a reorder + layout | ||
| // animation, then back to settle. | ||
| .get("#Test-1") | ||
| .trigger("pointerdown", 50, 20, { force: true }) | ||
| .wait(50) | ||
| .trigger("pointermove", 50, 40, { force: true }) | ||
| .wait(50) | ||
| .trigger("pointermove", 50, 100, { force: true }) | ||
| .wait(100) | ||
| .trigger("pointerup", 50, 100, { force: true }) | ||
| .wait(500) | ||
| .window() | ||
| .then((win) => { | ||
| const logs = readLogs(win) | ||
| // Every observed item must end its log with | ||
| // isIntersecting: true — none should be stuck "not visible". | ||
| const ids = new Set(logs.map((l) => l.id)) | ||
| ids.forEach((id) => { | ||
| const final = [...logs] | ||
| .reverse() | ||
| .find((entry) => entry.id === id) | ||
| expect(final, `final state for ${id}`).to.exist | ||
| expect( | ||
| final!.isIntersecting, | ||
| `${id} final isIntersecting` | ||
| ).to.equal(true) | ||
| }) | ||
| }) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1997,6 +1997,13 @@ export function createProjectionNode<I>({ | |||||||||||||||
| ? transformTemplate({}, "") | ||||||||||||||||
| : "none" | ||||||||||||||||
| this.hasProjected = false | ||||||||||||||||
|
|
||||||||||||||||
| // Flush layout so Chrome's IntersectionObserver re-evaluates | ||||||||||||||||
| // after the projection transform is removed (see #2679). | ||||||||||||||||
| if (this.instance) { | ||||||||||||||||
| void (this.instance as unknown as HTMLElement) | ||||||||||||||||
| .offsetWidth | ||||||||||||||||
| } | ||||||||||||||||
|
Comment on lines
+2003
to
+2006
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| return | ||||||||||||||||
|
|
||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
time.now()frommotion-dom/src/frameloop/sync-time.tsinstead ofperformance.now(). While this is a dev test file and the timestamp is used only for logging, keeping it consistent with the rest of the codebase avoids ambiguity about which clock source to use.Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!