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.

Sign up for GitHub

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 legacy widget previews #32260

Merged
merged 3 commits into from
May 28, 2021
Merged

Fix legacy widget previews #32260

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c46d849
Fix legacy widget block previews being set to zero height
talldan May 27, 2021
abcd880
Use intersection observer with load event
talldan May 27, 2021
d13d0b9
Use or instead of nullish-coalescing operator
talldan May 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 34 additions & 7 deletions packages/block-library/src/legacy-widget/edit/preview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,44 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { useRefEffect } from '@wordpress/compose';
import { addQueryArgs } from '@wordpress/url';
import { useState } from '@wordpress/element';
import { Placeholder, Spinner, Disabled } from '@wordpress/components';
import { __ } from '@wordpress/i18n';

export default function Preview( { idBase, instance, isVisible } ) {
const [ iframeHeight, setIframeHeight ] = useState( null );
const [ iframeHeight, setIframeHeight ] = useState();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why removing null? I believe it was served to be the default initial value as the loading state.

Maybe we should just use a loading state here, and set the height directly via DOM API (iframe.style.height).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok, didn't notice that. I think using null in that way is definitely confusing. I appreciate the change in #33191, which improves that.


// Resize the iframe on either the load event, or when the iframe becomes visible.
const ref = useRefEffect( ( iframe ) => {
function onChange() {
const boundingRect = iframe?.contentDocument?.body?.getBoundingClientRect();
if ( boundingRect ) {
// Include `top` in the height calculation to avoid the bottom
// of widget previews being cut-off. Most widgets have a
// heading at the top that has top margin, and the `height`
// alone doesn't take that margin into account.
setIframeHeight( boundingRect.top + boundingRect.height );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still seeing scrollbars when expanding a widget area though.

image

Maybe we need to do something like this:

const height = Math.max(
  boundingRect.top + boundingRect.height,
  iframe.contentDocument.documentElement.scrollHeight,
);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha I was just popping in to say we could use contentDocument.documentElement.scrollHeight instead of the body boundingRect to simplify calculations and get rid of the scrollbar 😄

Copy link
Contributor Author

@talldan talldan Jul 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scrollHeight is how it used to work, but it doesn't take into account margins.

I didn't see any scrollbars in testing, but I guess it needs to be iterated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was to measure the scrollHeight of the html element, so it includes the margins of all the elements inside, whereas with the body they sometimes collapse out of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, sorry, I missed that bit of nuance.

}
}

const { IntersectionObserver } = iframe.ownerDocument.defaultView;

// Observe for intersections that might cause a change in the height of
// the iframe, e.g. a Widget Area becoming expanded.
const intersectionObserver = new IntersectionObserver( onChange, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also call intersectionObserver.disconnect() to free up the memory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. First time using intersectionObserver, so wasn't aware it needed to be disconnected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does IntersectionObserver actually do? There seem to be no docs for it and the name confuses the heck out of me. I don't understand what an intersection might be in this context 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API

It's usually used for lazily showing stuff that's scrolled into view, but also works in this case. The idea is to observe for when the block preview element becomes visible (the widget area is expanded) and only calculate the height of the content when that happens. This was the first time I've had the chance to use it because I've always had to support IE before. 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooh nice, I didn't realise it was an actual web API!

threshold: 1,
} );
intersectionObserver.observe( iframe );

iframe.addEventListener( 'load', onChange );

return () => {
iframe.removeEventListener( 'load', onChange );
};
}, [] );

return (
<>
{ /*
Expand Down Expand Up @@ -41,6 +72,7 @@ export default function Preview( { idBase, instance, isVisible } ) {
load scripts and styles that it needs to run.
*/ }
<iframe
ref={ ref }
className="wp-block-legacy-widget__edit-preview-iframe"
title={ __( 'Legacy Widget Preview' ) }
// TODO: This chokes when the query param is too big.
Expand All @@ -53,12 +85,7 @@ export default function Preview( { idBase, instance, isVisible } ) {
instance,
},
} ) }
height={ iframeHeight ?? 100 }
onLoad={ ( event ) => {
setIframeHeight(
event.target.contentDocument.body.scrollHeight
);
} }
height={ iframeHeight || 100 }
/>
</Disabled>
</div>
Expand Down