fix: autoscroll when block is bigger than viewport #3746

gztomas · 2020-06-20T05:05:29Z

Is this adding or improving a feature or fixing a bug?

This is fixing an issue #3146

What's the new behavior?

How does this change work?

scroll-into-view-if-needed can only scroll elements into view. In this case, the element is bigger than the viewport, and we actually want to scroll the range of the selection into the viewport. Range already implements getBoundingClientRect. That should be all what scroll-into-view-if-needed needs to do its thing. But scroll-into-view-if-needed won't just work if we pass a range instead of an element. While being a bit hacky, overriding getBoundingClientRect of the block with the getBoundingClientRect from the range works wonderfully.

Have you checked that...?

The new code matches the existing patterns and styles.
The tests pass with yarn test.
The linter passes with yarn lint. (Fix errors with yarn fix.)
The relevant examples still work. (Run examples with yarn start.)

Does this fix any issues or need any specific reviewers?

Fixes: #3146
Reviewers: @CameronAckermanSEL @ianstormtaylor

rglusk

@ianstormtaylor can you please take a look at adding in this fix? getting this improvement out would be a big win for a lot of projects facing this bug :)

rglusk · 2020-06-30T16:56:06Z

@gztomas can you please tag @ianstormtaylor as a reviewer? maybe that will help to get this reviewed and merged sooner

rglusk · 2020-06-30T17:38:37Z

packages/slate-react/src/components/editable.tsx

@@ -186,10 +186,14 @@ export const Editable = (props: EditableProps) => {
    if (newDomRange) {
      domSelection.addRange(newDomRange!)
      const leafEl = newDomRange.startContainer.parentElement!
+      leafEl.getBoundingClientRect = newDomRange.getBoundingClientRect.bind(
+        newDomRange
+      )


when testing this fix, did you scroll around a lot? Worried about performance issues when building and destroying this every time you scroll

I haven't found any performance issues so far. If that was the case however I think we would see the issue on typing rather than on scrolling given this piece of code is only called when the selection changes, not the scroll. I believe this should be a cheap operation either way. Or at least probably cheaper than the DOM access operations that happen on toDOMRange sequential to this.

cameracker · 2020-07-02T18:01:28Z

Hi @gztomas , thank you so much for the contribution.

I went to go test this out on the deployed preview examples and it didn't seem to have any effect. What I tried was going to the rich text example, adding enough text for there to be a scollbar, and then I typed while the cursor was in and out of the view port. In both cases, the behavior was identical to what is now on latest. I also tried this by ensuring the text was in a single block and still no improvement.

Is there something I'm missing in the steps to reproduce?

gztomas · 2020-07-02T18:13:55Z

Hi @CameronAckermanSEL
Thanks for looking into this. What I have seen this change fixes happens when the slate editor height has a limit. I can repro the issue in the preview examples only after some css changes:

Please let me know if that makes sense

rglusk · 2020-07-08T17:48:57Z

@CameronAckermanSEL have you had a chance to re-review this? Is there anything else you need to repro?

rglusk · 2020-08-31T23:30:10Z

@gztomas can we push this change out? its been a few months and there's been no comments around this change causing issues.

cc: @CameronAckermanSEL @ianstormtaylor

gztomas · 2020-08-31T23:47:01Z

@rglusk I don't think there is anything to be done on our side. There are a lot of PRs unattended fixing really important stuff.

rglusk · 2020-09-01T00:57:16Z

@timbuckley, i see you recently merged new code. Can you please either merge this for us, or give @gztomas and I write access to this repo? Thank you!!

anhphung97 · 2020-09-01T16:40:56Z

This is a problem for me as well, would like to see it gets fixed soon 👍 @timbuckley

gztomas · 2020-09-01T16:58:56Z

In case it was useful for you, this is the hack I'm using to fix this on my side

// HACK for fixing Slate auto-scroll issue
// Overriding the rect for the Slate block with the selection block so Slate
// auto-scrolls to where the cursor is rather than auto-scrolling the whole
// block. If the block is big enough then the auto-scroll can make the cursor be
// outside of the viewport.
const { toDOMRange } = ReactEditor;
ReactEditor.toDOMRange = (editor, selection) => {
  const range = toDOMRange(editor, selection);
  range.startContainer.parentElement?.getBoundingClientRect = range.getBoundingClientRect.bind(
    range
  );
  return range;
};

rglusk · 2020-09-01T21:23:42Z

thanks @gztomas, that works :) hopefully this change will get merged at some point so other people can use your fix too!

changeset-bot · 2021-03-31T19:34:42Z

🦋 Changeset detected

Latest commit: 95a235d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
slate-react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

ianstormtaylor · 2021-03-31T19:50:51Z

Thanks @gztomas!

juliankrispel · 2021-04-20T05:38:57Z

packages/slate-react/src/components/editable.tsx

      scrollIntoView(leafEl, {
        scrollMode: 'if-needed',
        boundary: el,
      })
+      delete leafEl.getBoundingClientRect


🤔 this throws a typescript error could you elaborate why this is needed @gztomas?

@juliankrispel this restores the override happening at line 208, so any future call to leafEl.getBoundingClientRect will get the native getBoundingClientRect implementation from leafEl prototype chain. This is for cleanup purposes, as this code doesn't know who else might need to call getBoundingClientRect for this element. I guess TS can't know that even deleting this, getBoundingClientRect will still be available, from the prototype.

thanks for explaining @gztomas

…tormtaylor#3746)

fix: autoscroll when block is bigger than viewport

af9faf0

rglusk reviewed Jun 29, 2020

View reviewed changes

rglusk reviewed Jun 30, 2020

View reviewed changes

rglusk approved these changes Sep 1, 2020

View reviewed changes

ulion mentioned this pull request Jan 5, 2021

Support custom scrollIntoView behaviour from user side. #4037

Merged

4 tasks

Merge branch 'master' into scroll-when-block-is-bigger-than-viewport

6178862

ianstormtaylor added 3 commits March 31, 2021 15:36

Create lazy-trains-sell.md

bbad026

Update lazy-trains-sell.md

db3b47b

Update lazy-trains-sell.md

95a235d

ianstormtaylor merged commit f8be509 into ianstormtaylor:master Mar 31, 2021

ianstormtaylor added the bug label Mar 31, 2021

This was referenced Mar 31, 2021

Version Packages #4156

Merged

Version Packages msc117/slate#1

Open

juliankrispel reviewed Apr 20, 2021

View reviewed changes

pzhine added a commit to databyss-org/slate that referenced this pull request Apr 23, 2021

apply patch: fix: autoscroll when block is bigger than viewport (ians…

407e09e

…tormtaylor#3746)

dylans mentioned this pull request Sep 15, 2022

Partially hidden blocks/images don't scroll into view from the top #3576

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: autoscroll when block is bigger than viewport #3746

fix: autoscroll when block is bigger than viewport #3746

gztomas commented Jun 20, 2020 •

edited

Loading

rglusk left a comment

rglusk commented Jun 30, 2020

rglusk Jun 30, 2020

gztomas Jun 30, 2020

cameracker commented Jul 2, 2020

gztomas commented Jul 2, 2020

rglusk commented Jul 8, 2020

rglusk commented Aug 31, 2020

gztomas commented Aug 31, 2020 •

edited

Loading

rglusk commented Sep 1, 2020

anhphung97 commented Sep 1, 2020

gztomas commented Sep 1, 2020 •

edited

Loading

rglusk commented Sep 1, 2020

changeset-bot bot commented Mar 31, 2021 •

edited

Loading

ianstormtaylor commented Mar 31, 2021

juliankrispel Apr 20, 2021

gztomas Apr 22, 2021 •

edited

Loading

juliankrispel Apr 23, 2021

fix: autoscroll when block is bigger than viewport #3746

fix: autoscroll when block is bigger than viewport #3746

Conversation

gztomas commented Jun 20, 2020 • edited Loading

Is this adding or improving a feature or fixing a bug?

What's the new behavior?

How does this change work?

Have you checked that...?

Does this fix any issues or need any specific reviewers?

rglusk left a comment

Choose a reason for hiding this comment

rglusk commented Jun 30, 2020

rglusk Jun 30, 2020

Choose a reason for hiding this comment

gztomas Jun 30, 2020

Choose a reason for hiding this comment

cameracker commented Jul 2, 2020

gztomas commented Jul 2, 2020

rglusk commented Jul 8, 2020

rglusk commented Aug 31, 2020

gztomas commented Aug 31, 2020 • edited Loading

rglusk commented Sep 1, 2020

anhphung97 commented Sep 1, 2020

gztomas commented Sep 1, 2020 • edited Loading

rglusk commented Sep 1, 2020

changeset-bot bot commented Mar 31, 2021 • edited Loading

🦋 Changeset detected

ianstormtaylor commented Mar 31, 2021

juliankrispel Apr 20, 2021

Choose a reason for hiding this comment

gztomas Apr 22, 2021 • edited Loading

Choose a reason for hiding this comment

juliankrispel Apr 23, 2021

Choose a reason for hiding this comment

gztomas commented Jun 20, 2020 •

edited

Loading

gztomas commented Aug 31, 2020 •

edited

Loading

gztomas commented Sep 1, 2020 •

edited

Loading

changeset-bot bot commented Mar 31, 2021 •

edited

Loading

gztomas Apr 22, 2021 •

edited

Loading