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 scrolling to the inserted block issue in the iFramed block editor #31448

Merged
merged 7 commits into from
May 14, 2021
Merged

Fix scrolling to the inserted block issue in the iFramed block editor #31448

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0eed49e
Turn BlockList into a scroll container
david-szabo97 May 4, 2021
a1c2baa
Apply to the writing flow instead
david-szabo97 May 4, 2021
2bc13fa
Revert reset.scss
david-szabo97 May 4, 2021
f4e7af0
Use the window as scroll container
david-szabo97 May 5, 2021
691419e
Add dom-scroll-into-view patch
david-szabo97 May 5, 2021
782544a
Add missing return
david-szabo97 May 5, 2021
24c57d7
Simplify hook
david-szabo97 May 7, 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
4 changes: 3 additions & 1 deletion packages/block-editor/src/components/block-list/use-block-props/use-scroll-into-view.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@ export function useScrollIntoView( clientId ) {
return;
}

const scrollContainer = getScrollContainer( extentNode );
const scrollContainer =
getScrollContainer( extentNode ) ||
extentNode.ownerDocument.defaultView;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think this should this be in the getScrollContainer function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we have the dom-scroll-into-view patched, it should work fine in all the other usages.

But I'd rather not add it to getScrollContainer to let the consumer determine whether they still want to proceed on with the scroll or not. Just like in this case. I'm not familiar with the other usages of getScrollContainer throughout the codebase.

If we plan to move this into getScrollContainer, it would be better to experiment in a separate PR.


// If there's no scroll container, it follows that there's no scrollbar
// and thus there's no need to try to scroll into view.
Expand Down
25 changes: 25 additions & 0 deletions patches/dom-scroll-into-view+1.2.1.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
diff --git a/node_modules/dom-scroll-into-view/lib/dom-scroll-into-view.js b/node_modules/dom-scroll-into-view/lib/dom-scroll-into-view.js
Copy link
Member

Choose a reason for hiding this comment

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

Could you remove this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't. This patch is needed. Without the patch it won't scroll upward, only downward. You can check the CodeSandbox example for a proof.

Copy link
Contributor

Choose a reason for hiding this comment

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

How des this file works? How it should be included when we move this code into Core? Should we propose a patch upstream instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, is this useful for the "template editor" or just the "site editor"?

Copy link
Member Author

@david-szabo97 david-szabo97 May 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is useful for anything that's iframed.

How des this file works? How it should be included when we move this code into Core?

We have a postinstall script that patches the package:

"postinstall": "patch-package && node ./patches/patch-xcode.js",

patch-package applies the patch after npm i.

Should we propose a patch upstream instead?

I will bring this up in the code's repo.

index 557ed5c..23dfbad 100644
--- a/node_modules/dom-scroll-into-view/lib/dom-scroll-into-view.js
+++ b/node_modules/dom-scroll-into-view/lib/dom-scroll-into-view.js
@@ -21,6 +21,7 @@ function scrollIntoView(elem, container, config) {
allowHorizontalScroll = allowHorizontalScroll === undefined ? true : allowHorizontalScroll;

var isWin = util.isWindow(container);
+ var isFramed = !!(isWin && container.frameElement);
var elemOffset = util.offset(elem);
var eh = util.outerHeight(elem);
var ew = util.outerWidth(elem);
@@ -35,7 +36,11 @@ function scrollIntoView(elem, container, config) {
var ww = undefined;
var wh = undefined;

- if (isWin) {
+ if (isFramed) {
+ container = container.document.scrollingElement || container.document.body;
+ }
+
+ if (isWin || isFramed) {
win = container;
wh = util.height(win);
ww = util.width(win);