-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[6.7] Zoom in/out to correct location #66618
Merged
+371
−70
Merged
Changes from all commits
Commits
Show all changes
30 commits
Select commit
Hold shift + click to select a range
b0d073a
Midway commit. Scaling out is broken
jeryj 122790d
Fix zoom in animation
jeryj 1dedf7a
Fix scaling by preserving CSS properites
jeryj 114e23b
Prevent reflow from removing and re adding scrollbar in animation
jeryj 39eb8dd
Only rerun zoom out use effects if zoom out has changed.
jeryj d08a6f6
Allow all CSS vars to update when scale changes
jeryj 046902a
Midway commit. Math is wrong for addressing top/bottom exceptions
jeryj ab88dfa
Remove usePrevious usage
ajlende be9ade9
Add prefers-reduced-motion to setTimeout delay
ajlende 9f2d384
WIP Working zoom without frame size
ajlende c4f97ba
Account for changes to client height when determining edge threshold
jeryj 8bc12b2
Zoom to center unless it will reveal top or bottom
jeryj fffc48d
Account for a top threshold when zooming in and out
jeryj 42dd030
Clean up math and add comments
ajlende 5fff8f9
Add event listener instead of timeout
ajlende c182b70
Fix reduced motion
ajlende a9211e7
Remove timeout ref
ajlende 6c91090
Refactor callback as separate effect
ajlende 615f2b3
Fix JSDoc comment
ajlende 907dde4
Try to add back useEffect cleanups
ajlende fe8fd4e
Initialize prevClientHeight in the useEffect
ajlende 624be01
use useReducedMotion
jeryj b9eb0d2
Add test for zoom in/out location
jeryj d51db38
Hack to fix reduced motion
ajlende 1e7f1ee
Clean up the frameSizeValue and scaleValue calculations
ajlende 740cd9a
Replace TODO comments with HACK comments
ajlende 37111fa
Add cleanup for raf
ajlende ce8665d
Simplify CSS diff for 6.7 review
ajlende ab31e44
Add one more HACK comment
ajlende 0b4d86d
Do not allow scrollTopNext to be smaller than 0
jeryj File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ import { | |
useMergeRefs, | ||
useRefEffect, | ||
useDisabled, | ||
useReducedMotion, | ||
} from '@wordpress/compose'; | ||
import { __experimentalStyleProvider as StyleProvider } from '@wordpress/components'; | ||
import { useSelect } from '@wordpress/data'; | ||
|
@@ -121,6 +122,7 @@ function Iframe( { | |
}; | ||
}, [] ); | ||
const { styles = '', scripts = '' } = resolvedAssets; | ||
/** @type {[Document, import('react').Dispatch<Document>]} */ | ||
const [ iframeDocument, setIframeDocument ] = useState(); | ||
const initialContainerWidth = useRef( 0 ); | ||
const [ bodyClasses, setBodyClasses ] = useState( [] ); | ||
|
@@ -130,6 +132,7 @@ function Iframe( { | |
useResizeObserver(); | ||
const [ containerResizeListener, { width: containerWidth } ] = | ||
useResizeObserver(); | ||
const prefersReducedMotion = useReducedMotion(); | ||
|
||
const setRef = useRefEffect( ( node ) => { | ||
node._load = () => { | ||
|
@@ -252,6 +255,19 @@ function Iframe( { | |
containerWidth | ||
); | ||
|
||
const frameSizeValue = parseInt( frameSize ); | ||
|
||
const maxWidth = 750; | ||
const scaleValue = | ||
scale === 'default' | ||
? ( Math.min( containerWidth, maxWidth ) - frameSizeValue * 2 ) / | ||
scaleContainerWidth | ||
: scale; | ||
|
||
const prevScaleRef = useRef( scaleValue ); | ||
const prevFrameSizeRef = useRef( frameSizeValue ); | ||
const prevClientHeightRef = useRef( /* Initialized in the useEffect. */ ); | ||
|
||
const disabledRef = useDisabled( { isDisabled: ! readonly } ); | ||
const bodyRef = useMergeRefs( [ | ||
useBubbleEvents( iframeDocument ), | ||
|
@@ -303,47 +319,173 @@ function Iframe( { | |
|
||
useEffect( () => cleanup, [ cleanup ] ); | ||
|
||
const zoomOutAnimationClassnameRef = useRef( null ); | ||
|
||
// Toggle zoom out CSS Classes only when zoom out mode changes. We could add these into the useEffect | ||
// that controls settings the CSS variables, but then we would need to do more work to ensure we're | ||
// only toggling these when the zoom out mode changes, as that useEffect is also triggered by a large | ||
// number of dependencies. | ||
useEffect( () => { | ||
if ( ! iframeDocument || ! isZoomedOut ) { | ||
if ( | ||
! iframeDocument || | ||
// HACK: Checking if isZoomedOut differs from prevIsZoomedOut here | ||
// instead of the dependency array to appease the linter. | ||
( scaleValue === 1 ) === ( prevScaleRef.current === 1 ) | ||
) { | ||
return; | ||
} | ||
|
||
const handleZoomOutAnimationClassname = () => { | ||
clearTimeout( zoomOutAnimationClassnameRef.current ); | ||
// Unscaled height of the current iframe container. | ||
const clientHeight = iframeDocument.documentElement.clientHeight; | ||
|
||
// Scaled height of the current iframe content. | ||
const scrollHeight = iframeDocument.documentElement.scrollHeight; | ||
|
||
// Previous scale value. | ||
const prevScale = prevScaleRef.current; | ||
|
||
// Unscaled size of the previous padding around the iframe content. | ||
const prevFrameSize = prevFrameSizeRef.current; | ||
|
||
// Unscaled height of the previous iframe container. | ||
const prevClientHeight = prevClientHeightRef.current ?? clientHeight; | ||
|
||
// We can't trust the set value from contentHeight, as it was measured | ||
// before the zoom out mode was changed. After zoom out mode is changed, | ||
// appenders may appear or disappear, so we need to get the height from | ||
// the iframe at this point when we're about to animate the zoom out. | ||
// The iframe scrollTop, scrollHeight, and clientHeight will all be | ||
// accurate. The client height also does change when the zoom out mode | ||
// is toggled, as the bottom bar about selecting the template is | ||
// added/removed when toggling zoom out mode. | ||
const scrollTop = iframeDocument.documentElement.scrollTop; | ||
|
||
// Step 0: Start with the current scrollTop. | ||
let scrollTopNext = scrollTop; | ||
|
||
// Step 1: Undo the effects of the previous scale and frame around the | ||
// midpoint of the visible area. | ||
scrollTopNext = | ||
( scrollTopNext + prevClientHeight / 2 - prevFrameSize ) / | ||
prevScale - | ||
prevClientHeight / 2; | ||
|
||
// Step 2: Apply the new scale and frame around the midpoint of the | ||
// visible area. | ||
scrollTopNext = | ||
( scrollTopNext + clientHeight / 2 ) * scaleValue + | ||
frameSizeValue - | ||
clientHeight / 2; | ||
|
||
// Step 3: Handle an edge case so that you scroll to the top of the | ||
// iframe if the top of the iframe content is visible in the container. | ||
// The same edge case for the bottom is skipped because changing content | ||
// makes calculating it impossible. | ||
scrollTopNext = scrollTop <= prevFrameSize ? 0 : scrollTopNext; | ||
|
||
// This is the scrollTop value if you are scrolled to the bottom of the | ||
// iframe. We can't just let the browser handle it because we need to | ||
// animate the scaling. | ||
const maxScrollTop = | ||
scrollHeight * ( scaleValue / prevScale ) + | ||
frameSizeValue * 2 - | ||
clientHeight; | ||
|
||
// Step 4: Clamp the scrollTopNext between the minimum and maximum | ||
// possible scrollTop positions. Round the value to avoid subpixel | ||
// truncation by the browser which sometimes causes a 1px error. | ||
scrollTopNext = Math.round( | ||
Math.min( | ||
Math.max( 0, scrollTopNext ), | ||
Math.max( 0, maxScrollTop ) | ||
) | ||
); | ||
|
||
iframeDocument.documentElement.classList.add( | ||
iframeDocument.documentElement.style.setProperty( | ||
'--wp-block-editor-iframe-zoom-out-scroll-top', | ||
`${ scrollTop }px` | ||
); | ||
|
||
iframeDocument.documentElement.style.setProperty( | ||
'--wp-block-editor-iframe-zoom-out-scroll-top-next', | ||
`${ scrollTopNext }px` | ||
); | ||
|
||
iframeDocument.documentElement.classList.add( 'zoom-out-animation' ); | ||
|
||
function onZoomOutTransitionEnd() { | ||
// Remove the position fixed for the animation. | ||
iframeDocument.documentElement.classList.remove( | ||
'zoom-out-animation' | ||
); | ||
|
||
zoomOutAnimationClassnameRef.current = setTimeout( () => { | ||
iframeDocument.documentElement.classList.remove( | ||
'zoom-out-animation' | ||
// Update previous values. | ||
prevClientHeightRef.current = clientHeight; | ||
prevFrameSizeRef.current = frameSizeValue; | ||
prevScaleRef.current = scaleValue; | ||
|
||
// Set the final scroll position that was just animated to. | ||
iframeDocument.documentElement.scrollTop = scrollTopNext; | ||
} | ||
|
||
let raf; | ||
if ( prefersReducedMotion ) { | ||
// Hack: Wait for the window values to recalculate. | ||
raf = iframeDocument.defaultView.requestAnimationFrame( | ||
onZoomOutTransitionEnd | ||
); | ||
} else { | ||
iframeDocument.documentElement.addEventListener( | ||
'transitionend', | ||
onZoomOutTransitionEnd, | ||
{ once: true } | ||
ajlende marked this conversation as resolved.
Show resolved
Hide resolved
|
||
); | ||
} | ||
|
||
return () => { | ||
iframeDocument.documentElement.style.removeProperty( | ||
'--wp-block-editor-iframe-zoom-out-scroll-top' | ||
); | ||
iframeDocument.documentElement.style.removeProperty( | ||
'--wp-block-editor-iframe-zoom-out-scroll-top-next' | ||
); | ||
iframeDocument.documentElement.classList.remove( | ||
'zoom-out-animation' | ||
); | ||
if ( prefersReducedMotion ) { | ||
iframeDocument.defaultView.cancelAnimationFrame( raf ); | ||
} else { | ||
iframeDocument.documentElement.removeEventListener( | ||
'transitionend', | ||
onZoomOutTransitionEnd | ||
); | ||
}, 400 ); // 400ms should match the animation speed used in components/iframe/content.scss | ||
} | ||
}; | ||
}, [ iframeDocument, scaleValue, frameSizeValue, prefersReducedMotion ] ); | ||
|
||
handleZoomOutAnimationClassname(); | ||
iframeDocument.documentElement.classList.add( 'is-zoomed-out' ); | ||
// Toggle zoom out CSS Classes only when zoom out mode changes. We could add these into the useEffect | ||
// that controls settings the CSS variables, but then we would need to do more work to ensure we're | ||
// only toggling these when the zoom out mode changes, as that useEffect is also triggered by a large | ||
// number of dependencies. | ||
useEffect( () => { | ||
if ( ! iframeDocument ) { | ||
return; | ||
} | ||
|
||
return () => { | ||
handleZoomOutAnimationClassname(); | ||
if ( isZoomedOut ) { | ||
iframeDocument.documentElement.classList.add( 'is-zoomed-out' ); | ||
} else { | ||
// HACK: Since we can't remove this in the cleanup, we need to do it here. | ||
iframeDocument.documentElement.classList.remove( 'is-zoomed-out' ); | ||
} | ||
|
||
return () => { | ||
// HACK: Skipping cleanup because it causes issues with the zoom out | ||
// animation. More refactoring is needed to fix this properly. | ||
// iframeDocument.documentElement.classList.remove( 'is-zoomed-out' ); | ||
}; | ||
}, [ iframeDocument, isZoomedOut ] ); | ||
|
||
// Calculate the scaling and CSS variables for the zoom out canvas | ||
useEffect( () => { | ||
if ( ! iframeDocument || ! isZoomedOut ) { | ||
if ( ! iframeDocument ) { | ||
return; | ||
} | ||
|
||
const maxWidth = 750; | ||
// Note: When we initialize the zoom out when the canvas is smaller (sidebars open), | ||
// initialContainerWidth will be smaller than the full page, and reflow will happen | ||
// when the canvas area becomes larger due to sidebars closing. This is a known but | ||
|
@@ -354,11 +496,7 @@ function Iframe( { | |
// but calc( 100px / 2px ) is not. | ||
iframeDocument.documentElement.style.setProperty( | ||
'--wp-block-editor-iframe-zoom-out-scale', | ||
scale === 'default' | ||
? ( Math.min( containerWidth, maxWidth ) - | ||
parseInt( frameSize ) * 2 ) / | ||
scaleContainerWidth | ||
: scale | ||
scaleValue | ||
); | ||
|
||
// frameSize has to be a px value for the scaling and frame size to be computed correctly. | ||
|
@@ -384,27 +522,29 @@ function Iframe( { | |
); | ||
|
||
return () => { | ||
iframeDocument.documentElement.style.removeProperty( | ||
'--wp-block-editor-iframe-zoom-out-scale' | ||
); | ||
iframeDocument.documentElement.style.removeProperty( | ||
'--wp-block-editor-iframe-zoom-out-frame-size' | ||
); | ||
iframeDocument.documentElement.style.removeProperty( | ||
'--wp-block-editor-iframe-zoom-out-content-height' | ||
); | ||
iframeDocument.documentElement.style.removeProperty( | ||
'--wp-block-editor-iframe-zoom-out-inner-height' | ||
); | ||
iframeDocument.documentElement.style.removeProperty( | ||
'--wp-block-editor-iframe-zoom-out-container-width' | ||
); | ||
iframeDocument.documentElement.style.removeProperty( | ||
'--wp-block-editor-iframe-zoom-out-scale-container-width' | ||
); | ||
// HACK: Skipping cleanup because it causes issues with the zoom out | ||
// animation. More refactoring is needed to fix this properly. | ||
// iframeDocument.documentElement.style.removeProperty( | ||
// '--wp-block-editor-iframe-zoom-out-scale' | ||
// ); | ||
// iframeDocument.documentElement.style.removeProperty( | ||
// '--wp-block-editor-iframe-zoom-out-frame-size' | ||
// ); | ||
// iframeDocument.documentElement.style.removeProperty( | ||
// '--wp-block-editor-iframe-zoom-out-content-height' | ||
// ); | ||
// iframeDocument.documentElement.style.removeProperty( | ||
// '--wp-block-editor-iframe-zoom-out-inner-height' | ||
// ); | ||
// iframeDocument.documentElement.style.removeProperty( | ||
// '--wp-block-editor-iframe-zoom-out-container-width' | ||
// ); | ||
// iframeDocument.documentElement.style.removeProperty( | ||
// '--wp-block-editor-iframe-zoom-out-scale-container-width' | ||
// ); | ||
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. Seems fine. So we always want to calculate these variables I guess. Unmounting is not needed then because when the iframe unmounts these attributes are automatically removed too. |
||
}; | ||
}, [ | ||
scale, | ||
scaleValue, | ||
frameSize, | ||
iframeDocument, | ||
iframeWindowInnerHeight, | ||
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Could you comment on why fixed positioning is needed on the
html
element to make this work?