-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 zoom animation scrollbar #67536
Fix zoom animation scrollbar #67536
Conversation
- Keeps it in line with the correct TransitionState type - Separates out computeScrollHeightNext from computeScrollTopNext - Refactors copmuteScrollTopNext to use prev prefixed constants for all transitionFrom values to normalize the naming
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
transitionToRef.current.scrollHeight = computeScrollHeightNext( | ||
transitionFromRef.current, | ||
transitionToRef.current | ||
); |
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.
On first render (previously), transitionToRef
did not have a scrollHeight
value.
@@ -47,12 +62,12 @@ function computeScrollTopNext( transitionFrom, transitionTo ) { | |||
containerHeight: prevContainerHeight, | |||
frameSize: prevFrameSize, | |||
scaleValue: prevScale, | |||
scrollTop, | |||
scrollHeight, | |||
scrollTop: prevScrollTop, |
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.
Renamed to keep naming consistent with all transitionFrom
values having a prev
prefix for clarity.
transitionFromRef.current.containerHeight = | ||
transitionFromRef.current.containerHeight ?? | ||
containerHeight; // Use containerHeight, as it's the previous container height value if none was set. |
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.
Using the transitionFromRef.current.containerHeight
won't be as accurate, as it could have changed from a manual resize between animations. Relying on containerHeight
will be more accurate.
Size Change: +93 B (+0.01%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
scrollHeight * ( scaleValue / prevScale ) + | ||
frameSize * 2 - | ||
containerHeight; | ||
const maxScrollTop = scrollHeight - containerHeight; |
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.
Nice! This is a lot more understandable after the refactoring!
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.
Thanks for the fix! I have confirmed that the issue has been resolved on Windows OS. |
Co-authored-by: jeryj <jeryj@git.wordpress.org> Co-authored-by: ajlende <ajlende@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: matiasbenedetto <mmaattiiaass@git.wordpress.org>
Co-authored-by: jeryj <jeryj@git.wordpress.org> Co-authored-by: ajlende <ajlende@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: matiasbenedetto <mmaattiiaass@git.wordpress.org>
Fixes #67360
What?
When animating between zoom levels, we were forcing a scrollbar during the
positioned: fixed
part of the animation to prevent a width snap. However, this assumes the container always has a scrollbar, which isn't true. To fix it, we only add the scrollbar into the frame if necessary.Why?
Improve layout animation when the editor canvas does not have a scrollbar.
How?
scrollHeight
is greater than the container (if it is, we force a scrollbar)TransitionState
type (previously,transitionTo
did not have ascrollHeight
buttransitionFrom
did.computeScrollHeightNext
fromcomputeScrollTopNext
computeScrollTopNext
to useprev
prefixed constants for alltransitionFrom
values to normalize the namingTesting Instructions
With scrollbar
With scrollbar
Testing Instructions for Keyboard
Screenshots or screencast