-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix useScroll target not accounting for CSS translate #3655
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,42 @@ | ||
| import { motion, useScroll, useTransform } from "framer-motion" | ||
| import * as React from "react" | ||
| import { useRef } from "react" | ||
|
|
||
| /** | ||
| * Regression test for #2914: useScroll target should account for CSS translate. | ||
| * | ||
| * The target div has transform: translateY(500px), pushing it 500px lower visually. | ||
| * Without the fix, useScroll ignores the translate and reports incorrect progress. | ||
| */ | ||
| export const App = () => { | ||
| const targetRef = useRef<HTMLDivElement>(null) | ||
| const { scrollYProgress } = useScroll({ | ||
| target: targetRef, | ||
| offset: ["start end", "end start"], | ||
| }) | ||
|
|
||
| // Drive opacity from scroll progress so Cypress can read computed style | ||
| const opacity = useTransform(scrollYProgress, [0, 1], [1, 0]) | ||
|
|
||
| return ( | ||
| <div> | ||
| <div style={{ height: 1000 }} /> | ||
| <div | ||
| ref={targetRef} | ||
| id="target" | ||
| style={{ | ||
| height: 200, | ||
| width: 200, | ||
| background: "red", | ||
| transform: "translateY(500px)", | ||
| }} | ||
| > | ||
| <motion.div | ||
| id="indicator" | ||
| style={{ width: 50, height: 50, background: "blue", opacity }} | ||
| /> | ||
| </div> | ||
| <div style={{ height: 3000 }} /> | ||
| </div> | ||
| ) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| describe("useScroll target accounts for CSS translate (#2914)", () => { | ||
| it("scroll progress reflects CSS translateY on target", () => { | ||
| cy.visit("?test=scroll-target-translate") | ||
| .wait(200) | ||
| .scrollTo(0, 1000, { duration: 0 }) | ||
| .wait(500) | ||
| .get("#indicator") | ||
| .then(([$el]: any) => { | ||
| const opacity = parseFloat(getComputedStyle($el).opacity) | ||
| /** | ||
| * Target layout position: 1000px (spacer height). | ||
| * Target has transform: translateY(500px), visual position = 1500px. | ||
| * With offset ["start end", "end start"] and 660px viewport: | ||
| * | ||
| * With fix (accounts for translate): | ||
| * progress at scroll 1000 ≈ 0.19, opacity ≈ 0.81 | ||
| * | ||
| * Without fix (ignores translate): | ||
| * progress at scroll 1000 ≈ 0.77, opacity ≈ 0.23 | ||
| * | ||
| * Assert opacity > 0.5 to verify translate is accounted for. | ||
| */ | ||
| expect(opacity).to.be.greaterThan(0.5) | ||
| }) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,28 @@ | ||
| import { isHTMLElement } from "motion-dom" | ||
|
|
||
| function addTranslateOffset( | ||
| inset: { x: number; y: number }, | ||
| element: HTMLElement | ||
| ) { | ||
| const style = getComputedStyle(element) | ||
| const { translate, transform } = style | ||
|
|
||
| if (translate && translate !== "none") { | ||
| const parts = translate.split(" ") | ||
| inset.x += parseFloat(parts[0]) || 0 | ||
| inset.y += parseFloat(parts[1] || "0") || 0 | ||
| } | ||
|
|
||
| if (transform && transform !== "none") { | ||
| const match = transform.match(/matrix\(([^)]+)\)/) | ||
| if (match) { | ||
| const values = match[1].split(",") | ||
| inset.x += parseFloat(values[4]) | ||
| inset.y += parseFloat(values[5]) | ||
| } | ||
| } | ||
|
Comment on lines
+16
to
+23
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.
The regex only matches the 2D For Suggested fix: if (transform && transform !== "none") {
const matrix3dMatch = transform.match(/matrix3d\(([^)]+)\)/)
if (matrix3dMatch) {
const values = matrix3dMatch[1].split(",")
inset.x += parseFloat(values[12])
inset.y += parseFloat(values[13])
} else {
const match = transform.match(/matrix\(([^)]+)\)/)
if (match) {
const values = match[1].split(",")
inset.x += parseFloat(values[4])
inset.y += parseFloat(values[5])
}
}
} |
||
| } | ||
|
|
||
| export function calcInset(element: Element, container: Element) { | ||
| const inset = { x: 0, y: 0 } | ||
|
|
||
|
|
@@ -8,6 +31,7 @@ export function calcInset(element: Element, container: Element) { | |
| if (isHTMLElement(current)) { | ||
| inset.x += current.offsetLeft | ||
| inset.y += current.offsetTop | ||
| addTranslateOffset(inset, current) | ||
| current = current.offsetParent | ||
| } else if (current.tagName === "svg") { | ||
| /** | ||
|
|
||
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.
translateproperty not resolvedgetComputedStyle(el).translatereturns the computed value, which preserves<percentage>values (e.g."50% 100px"is returned as-is, not resolved to pixels).parseFloat("50%")returns50, treating it as 50 px regardless of the element's actual size.This only affects the standalone CSS
translateproperty (nottransform: translateX(...), which does resolve to a pixel matrix). If users writetranslate: 50% 0, the inset will be wrong.Consider resolving the percentage against the element's bounding box, or documenting the limitation: