Skip to content
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

Fix projection of fixed elements when scrolling #2647

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

koop
Copy link

@koop koop commented May 2, 2024

Fixes the failing position: fixed; projection tests added in #1768.

  • I verified that all other projection tests still passed (I verified this manually because I couldn't get the cypress configuration to run on my local machine).

Fixes issues #2514, #2006, and #1972.

Before After
Sandbox to reproduce the issue Sandbox to verify fix (with patch applied)
2024-05-02 13 37 24 2024-05-02 13 38 16

Details

To handle projecting fixed elements within scrollable pages: as I understand it, the existing logic accounted for scroll root ancestors within removeElementScroll by iterating over the path, but this didn't account for the case where the current element was a fixed element, because the path does not contain the current node. Instead, we now avoid adding the scroll position in measurePageBox when the projection is or is within a fixed element.

To handle projected fixed elements within scrollable elements: this was a similar case where we weren't accounting for the current element's scroll information. If the current element is fixed, we can return early from removeElementScroll.

To handle projecting when an element changes between fixed and static positions: we track whether the position was fixed during the snapshot phase (instead of relying on the most recently measured value) and use that to determine whether the projection considers an element fixed or not. Two considerations here:

  • I'm not sure about the nuances of resuming projections and whether or not there would be a more correct way to track this information.
  • This does not solve for the case where an element transitions between position: sticky; and position: fixed;. Fixing this may involve tracking more than just whether or not the element is fixed when the snapshot is taken; I haven't spent enough time with that failing test case.

@mattgperry
Copy link
Collaborator

Hey this is really cool, thanks for the PR! I have some stuff to deal with first around getting deferred keyframes into Framer but will check it out after.

We don't need to worry so much about position: sticky - stickied elements themselves can flick between "fixed" and "static" modes and there's no good way of tracking this. But I'll probably double check we have the tests to check for regressions still.

@koop
Copy link
Author

koop commented May 6, 2024

Sounds good!

We don't need to worry so much about position: sticky - stickied elements themselves can flick between "fixed" and "static" modes and there's no good way of tracking this.

That's roughly what I expected (and why I didn't include it here), monitoring sticky position/state is a pain.

@lilingxi01
Copy link

Thank you for making this PR! This could resolve a lot of transition glitches related to position: fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants