Slate: Scroll-to-cursor is broken inside scrollable containers

[jennaplusplus]

Do you want to request a feature or report a bug?

bug

What's the current behavior?

Set the editor to a fixed height and overflow-y: auto (standard procedure for scrolling content). Press enter several times to exceed this height and the and cursor disappears; a scrollbar appears but the editor does not scroll down. However, if you type a few characters (instead of just creating new lines with 0-1 characters each), the editor suddenly snaps down as expected.

Demo (direct from example page):

Slate JSFiddle with same issue:
https://jsfiddle.net/f4150bbq/

This is not reproduceable in a simple JSFiddle with a contenteditable div:
https://jsfiddle.net/gz2wrkxz/1/

What's the expected behavior?

When content exceeds available height, scroll editor down to cursor (i.e., the behavior that happens when you type several characters in the demo above).

I've tried my best to figure out the cause of this, but I'm coming up short- could there be a problem with scrollToSelection? Seeing the expected behavior when typing several characters is also interesting; are we accidentally overriding some default browser behavior?

Mac OS 10.12.3, Chrome 60.0.3112.101, Slate current

[YurkaninRyan]

@jennaplusplus I also struggled with this in my editor, have you found a solution?

The solution I came up with was using componentWillReceiveProps to compare the last focusBlock with the current focusBlock in a variety of ways. If they were found different I would scrollIntoView the dom node

[jennaplusplus]

@YurkaninRyan I haven't :( Do you mind sharing your componentWillReceiveProps method?

[oyeanuj]

@jennaplusplus @YurkaninRyan I used @conorcussell's solution here. See if that works for you?

[YurkaninRyan]

I wonder if something like this should go into core, or if there is another underlying issue here. I tried debugging through this, but it seems in some cases it works, and in others it doesn't. Potentially a browser compat issue.

[conorcussell]

I can't actually replicate it at the moment - but if I remember correctly was something to do with all the values for the ClientRect in an empty selection being 0 https://github.com/ianstormtaylor/slate/blob/7cfbbfc5e4024003858a0d8a194d9e473f580675/src/utils/scroll-to-selection.js#L17

[ianstormtaylor]

I'm not sure why it's currently broken, since it was working before, but I'd love for this to be fixed in core if anyone is open to submitting a PR! Thank you!

[YurkaninRyan]

@ianstormtaylor @jennaplusplus @conorcussell Looking at this more, I think I see some potential issues.

No matter what ends up happening here, we do a window.scrollTo, everything in this function is assuming that the scrolling container is the window, i'd wager a guess that for most people experiencing this issue, the window isn't there scrolling container. I know that even for myself I use a custom scrolling container, because my editor can be positioned around the screen using position fixed.

So now the interesting part, what do we do about this? Should we try and figure out every time we run this function what the true scrolling container is?

[ianstormtaylor]

@YurkaninRyan ah good point, mine's always window so I never run into this. I think we need to do exactly as you say, and figure out the scrolling containers (plural).

I believe the correct code should recursively find each scrolling container above the editor (including the editor itself maybe) and check to make sure the cursor is visible inside of it. Because you could need to scroll both the window and an element container.

[RobertDaleSmith]

Recent changes make it so that now the content area scrolls when cursor is out of view, but there is an issue where it scrolls too far. It looks like the updated logic does not take into consideration the containers distance from the top of the window.

I was able to reproduce this issue on the examples page by adding overflow: auto to the parent element of the content area. I think if we just subtract the distance between the top and the content area from the calculated scrollTop = y then it should resolve this.

[erquhart]

Note: behavior outlined in this issue was further regressed in #1383. Details in #1416.

[bwalex]

Just thought I'd add my experience, hopefully it'll help someone else.

I ran into this issue, where the editor wouldn't scroll correctly on pressing enter in a scrollable container. However, after setting some breakpoints around the relevant logic, the issue I had was that findScrollContainer() was finding the "wrong" container. I had another container with auto overflow around the editor, which, however, wasn't doing the scrolling as its parent also had auto scroll. findScrollContainer() was finding the inner one.

I fixed it/worked around it by just setting overflow: visible on the container that was incorrectly identified by slate as the scroller by findScrollContainer().

[bryanph]

@bwalex Also, if you have any divs wrapping the currently selected block with overflow: auto, these will be selected as the scroll container causing the same issue.

[dmitrizzle]

I'd like to add my 2¢ by saying that scrolling logic should probably be a separate plugin.

Not sure if this is possible to implement with the way Slate operates now, however, scrolling behaviour is not universal for all types of applications. For example, focusing on void blocks (i.e. an image) may make sense in some cases to have the view scroll to the top edge of the block, and in others, to the bottom edge; and in some cases no scrolling is desired at all. Another case is a textfield with a limited input that requires no scrolling, which could make Slate component a lot lighter if it isn't requiring additional logic. Finally, there may be apps that prefer smooth scrolling or other clever methods.

[valmack]

Seems this has been an ongoing bug. Totally agree this should be a plugin, and not the shipped behavior.

I just commented out the scroll login and implemented my own.

https://github.com/ianstormtaylor/slate/issues/1551#issuecomment-465136126

[ianstormtaylor]

Either way, this is still something that would be nice to fix in core. Since the current behavior seems obviously wrong. If someone wants to make a PR I'd be happy to have this fixed. Thanks!

[fizcoder]

I second that! ☝️😁 I just encountered the issue too.

[lazybios]

Need a plugin for it, I have the same issue.

[ianstormtaylor]

I believe that this may be fixed by https://github.com/ianstormtaylor/slate/pull/3093, which has changed a lot of the logic in Slate and slate-react especially. I'm going to close this out, but as always, feel free to open a new issue if it persists for you. Thanks for understanding.