Fix scrolling to the inserted block issue in the iFramed block editor #31448
Conversation
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
david-szabo97
requested review from
mattwiebe,
youknowriad,
vindl,
creativecoder,
Copons,
jeyip and
Addison-Stavlo
|
Size Change: +844 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
May 4, 2021
youknowriad
reviewed
May 4, 2021
david-szabo97
added
the
[Status] In Progress
david-szabo97
removed
the
[Status] In Progress
|
@youknowriad Thanks for the review. After all, I decided to take a different approach. 🙇 |
|
I'm not a big fan of patching a package, but |
ellatrix
reviewed
May 5, 2021
| isFramed && | ||
| scrollingElement.scrollHeight > scrollingElement.clientHeight | ||
| ) { | ||
| scrollContainer = ownerDocument.defaultView; |
There was a problem hiding this comment.
Do we need all these checks? Why does getScrollContainer( extentNode ) || extentNode.ownerDocument.defaultView not work? I don't think it matters whether it's in a frame or not.
There was a problem hiding this comment.
Just added a missing return. The goal here is to save all the checks done by scrollIntoView. If we default to getScrollContainer( extentNode ) || extentNode.ownerDocument.defaultView then the scrollContainer will never be falsy, so scrollIntoView will be called every time.
david-szabo97
force-pushed
the
fix/edit-site-scroll-to-block
branch
from
165d38f
to
782544a
Compare
youknowriad
reviewed
May 7, 2021
| const scrollContainer = getScrollContainer( extentNode ); | ||
| const scrollContainer = | ||
| getScrollContainer( extentNode ) || | ||
| extentNode.ownerDocument.defaultView; |
There was a problem hiding this comment.
Do you think this should this be in the getScrollContainer function?
There was a problem hiding this comment.
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.
creativecoder
approved these changes
May 12, 2021
There was a problem hiding this comment.
✅ This fixes the scrolling issue for me when inserting patterns
I'm not a big fan of patching a package, but scroll-dom-into-view doesn't take iframes into account. The package doesn't seem to be active so I'm not sure if we can get a PR merged there.
I think it's worth a try! The package owner is recently active in other packages they own, so there's a chance. But I think this PR can proceed as is, as long as we loop back and remove the patch if the upstream change is accepted.
ellatrix
reviewed
May 13, 2021
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
How des this file works? How it should be included when we move this code into Core? Should we propose a patch upstream instead?
There was a problem hiding this comment.
Also, is this useful for the "template editor" or just the "site editor"?
There was a problem hiding this comment.
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.
ellatrix
approved these changes
May 13, 2021
|
I'm not sure I agree to patching a package like that. I'll check what else we can do. |
|
Yeah, this is definitely not a great way to fix the issue because it doesn't fix it in the npm package, only in the Gutenberg Plugin. What happens when npm consumers don't use the patch? will it degrade gracefully? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #31251
Description
dom-scroll-into-viewlibrary doesn't play nicely with iframes if there is no explicit scrolling container.Here is a simple CodeSandbox explaining the issue.
This PR patches the package by checking whether the container is an iframe window. In that case it scrolls the
scrollingElementinstead of the window. We can't scroll the iframe window because it's an iframe so the typicalwindow.scrollTo()doesn't work.How has this been tested?
Run
npm installto patch thedom-scroll-into-viewpackage.3.1.1. Make sure the insertion point is outside of your viewport, and it's below your current position.
3.1.2. Insert a pattern, make sure it automatically scrolled down to the inserted pattern
3.2.1 Make sure the insertion point is outside of your viewport, and it's above your current position.
3.2.2. Insert a pattern, make sure it automatically scrolled up to the inserted pattern
Screenshots
Screen.Recording.2021-05-03.at.10.55.11.mov
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).