ianstormtaylor / slate

A completely customizable framework for building rich text editors. (Currently in beta.)
http://slatejs.org
MIT License
29.71k stars 3.24k forks source link

Screen Jumps/Scrolls cursor out of view during editing on iOS (iPad, iPhone) #1513

Closed thesunny closed 6 years ago

thesunny commented 6 years ago

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

Report a Bug

What's the current behavior?

Using the rich text editor on the SlateJS demo site (or any of the demos for that matter):

http://slatejs.org/#/rich-text

If you start editing and the cursor goes below the virtual keyboard, the text doesn't scroll up properly. Instead, it jumps all the way to the top of the screen.

image-2

Note that the jump happened as soon as I typed anything. In this case, I hit the SPACE on the virtual keyboard.

This is on an iPad iOS 11 but the same happens on my iPad. The latest version of Slate as on the demo as of today.

From what I can remember, this did not happen before; however, it may have been quite an old version where it worked properly.

What's the expected behavior?

Normally, the screen will stay scrolled to the part of the editor we are editing.

zGrav commented 6 years ago

I cannot seem to replicate this on my iPhone nor Simulator.

Will investigate.

zGrav commented 6 years ago

@ianstormtaylor are you using a whiteSpace styler in the demos?

EDIT: Can confirm.

This is a weird issue due to the way that Safari handles the whiteSpace CSS var. As we've previously discussed in issue #1481 .

thesunny commented 6 years ago

@zGrav @ianstormtaylor Additional details:

Somehow it seems unlikely that window.scrollTo is broken in the newest version of iOS but if I remove that line, the bug does goes away. Of course it doesn't scroll the cursor into position though. I can only theorize that the scrollTo is triggering a scroll event and the function attached to the scroll event is not working properly. That's just a guess at this point though.

I feel like a decent temporary fix is simply doing:

function scrollToSelection(selection) {
  if (IS_IOS) return
// ...

With the matching import statement for IS_IOS at the top of course.

Because it's on iOS, having to scroll into view manually is not that painful (just drag screen with finger). This would probably be unacceptable on desktop. At any rate, on mobile devices it's actually quite usable whereas the current version is unusable on iOS. I can create a pull request if you like (or somebody can just add it if you have permissions as it's such an easy fix).

Would like to get at least this temporary fix working quickly. As it is, all my clients cannot use this on iOS devices right now which is not good for my users.

zGrav commented 6 years ago

@thesunny

The scrolling is working for me perfectly on iOS both iPhone and Simulator when I omit the whiteSpace, I do not believe the issue is at scrollToSelection.

The error that I get lies in:

// COMPAT: IE 11 does not support Selection.extend

if (native.extend) { // COMPAT: Since the DOM range has no concept of backwards/forwards // we need to check and do the right thing here. if (isBackward) { native.collapse(range.endContainer, range.endOffset); native.extend(range.startContainer, range.startOffset); } else { native.collapse(range.startContainer, range.startOffset); native.extend(range.endContainer, range.endOffset); <- here } } else { // COMPAT: IE 11 does not support Selection.extend, fallback to addRange native.addRange(range); }

EDIT:

A fix that I can find temporarily is adding a IS_IOS to the if statement above, not 100% perfect.

It really feels like that Safari just doesn't respect whiteSpace.

zGrav commented 6 years ago

I published new code to #1490 @ianstormtaylor , can you please take a look?

We should be good to go now.

thesunny commented 6 years ago

@zGrav

I just tested the code and the bug still appears on my iPad.

zGrav commented 6 years ago

That is very strange, I do not experience it on my iPhone/simulator anymore nor OSX Safari

thesunny commented 6 years ago

@zGrav More details...

I'm going to try again by doing a fresh pull. For my quick test I just cut and paste the RAW text into my current (up to date) repo of SlateJS. I did add a console.log just to make sure I was running the updated version and it showed up.

thesunny commented 6 years ago

Never pulled in a pull request before so had to figure that out first!

Unfortunately, the bug still appears to be there.

For further clarification, the jumpiness appears when the cursor is below the virtual keyboard. If it is above, there is no problem. So if you are testing on an iPhone, you have to make sure to hit enter a few times to get the cursor lower.

Also, I'm on iOS Version 11.1 (15B93) on the iPad in case that helps. There appears to be newer versions but I literally bought this iPad to fix this bug a few days ago so it isn't an old iOS either.

I'm trying to test this on my iPhone now as well but it's having trouble connecting to the server for some reason. Will give you an update once I get that working.

zGrav commented 6 years ago

@thesunny thank you for the prompt replies.

Please do. I am on the latest version on my iPhone and 11.2 on my simulator.

thesunny commented 6 years ago

On my iPhone, I don't get the jumpiness but scroll into view seems to just be disabled. It's kind of the equivalent of my if (IS_IOS) return code.

This is still preferable to the behavior on the iPad though which automatically scrolls to top hiding whatever you are typing.

Is this what you are seeing/expecting on the iPhone as well? Simply no scroll into view behavior?

zGrav commented 6 years ago

@thesunny at the moment, yes, the scrolltoselection does happen, but you do not see it, that is due to the keyboard not being taken in account.

I am currently trying to tackle that issue right now :)

thesunny commented 6 years ago

My iPad is on 11.1 (15B93) and my iPhone is on 11.2.1 (15C153).

I wonder if that jumpy behavior which, as mentioned earlier, seems to be a broken scrollTo is part of iOS 11.1. I'm trying to see if I can replicate it on the simulator using an older OS version. Just downloading the older version now.

thesunny commented 6 years ago

It's possible this is a bug in exactly the 11.1 version of iOS. So my iPad at home may have coincidentally been the same version as the iPad I just bought and hence is experiencing the same issue.

https://greensock.com/forums/topic/17394-scrollto-plugin-issues-with-ios-update/

It's not 100% clear that this IS the same bug though. In the link above, they claim that this occurs only when scrollTo is called within an iFrame. In our case, we don't have an iFrame.

thesunny commented 6 years ago

Just to keep you updated, I'm download the iOS 11.1 Simulator and will try to replicate the scroll issue there. The download is going very slowly for some reason.

I am considering upgrading my iPad to the newest iOS version but also am a little worried to have the bug disappear and then it can't be replicated anymore; hence, I'm waiting to see if we can get it working on the simulator first.

Since I'm waiting for the Simulator to download, I'm going to grab a coffee. ☕️

zGrav commented 6 years ago

scrollTo seems to work fine for me here, however, it's behind the keyboard due to iOS not reporting the correct height :(

zGrav commented 6 years ago

w/o keyboard: http://g.recordit.co/KIB4az4egi.gif w/ keyboard: http://g.recordit.co/VlveRHl5mx.gif

thesunny commented 6 years ago

So I couldn't replicate the scrollTo bug in the 11.1 simulator.

I am going to upgrade my iPad OS though anyways and see if it fixes the bug. At least if it does I can tell people to upgrade their iPad for a fix.

Let me know if you think it's better to keep on this version for testing. I'll start the upgrade in about 10 minutes if I don't hear back. :)

zGrav commented 6 years ago

Please update it @thesunny , I want to see if it's something cache related as well or not, let's all be on the latest versions!

thesunny commented 6 years ago

FYI, I had to resolve a similar issue in Slate before regarding the virtual keyboard but found it difficult to find the height of the keyboard. All the solutions seemed to be for apps, not for the browsers... There are lists of virtual keyboard heights which was as far as I got. For my purposes, I think I just guesstimated and it worked good enough...

thesunny commented 6 years ago

Okay, I'm going ahead with the update.

zGrav commented 6 years ago

@thesunny for Android, I believe as is should work fine, alas I do not have a Android device to test at the moment...

for iOS, it seems... way trickier since Apple doesn't do it "natively"...

zGrav commented 6 years ago

I'm currently downloading iOS 11.1 for Simulator to try to replicate the bug and Android Emulator just to see if everything works as intended on that end

thesunny commented 6 years ago

Let me know if there is anything I can do to help. Research, testing, etc.

thesunny commented 6 years ago

FYI, I couldn't replicate the scrollTo bug in the simulator.

Details:

So the build on the simulator is slightly different but it could be just a difference between the simulator and the physical hardware.

Note: I did on multiple occasions make sure the latest code was running on the iPad and not a cached version by injecting console.log with some random string to make sure the copy was fresh.

thesunny commented 6 years ago

Let me know if I'm overloading you with information.

This might help with respect to the auto-zooming that iOS does. The auto-zooming may make it hard to scroll to the right position...

https://github.com/mcasimir/mobile-angular-ui/issues/264

Not sure if this can be implemented in a generic way, but it could at the least be used in conjunction with any fixes and added to the documentation to make iOS work properly.

Going to test this on the demo to see if it works.

zGrav commented 6 years ago

Can confirm that it works perfectly fine on Android. It's just a matter of Apple not implementing it in a generic way I guess.

lxcid commented 6 years ago

Sorry for budging like that. I’m still digesting this issue but I was wondering could it be related to Safari have issue with selection range where it return 0 as described in https://stackoverflow.com/a/44261188

I have the following workaround in my codebase previously…

    const selection = window.getSelection();
    let range = selection.getRangeAt(0);
    if (IS_SAFARI) {
      // `getBoundingClientRect` is buggy on Safari when there is no selected text active.
      // A workaround is available at https://stackoverflow.com/a/44261188
      range = range.cloneRange();
      range.setStart(range.startContainer, 0);
    }

I check a bit of the code (through my phone) and seems like selection is unmodified. https://github.com/zGrav/slate/blob/3f44580ad18ee548e5fa20a98a61d2c6b924e6fa/packages/slate-react/src/components/content.js#L147-L148

and

https://github.com/zGrav/slate/blob/3f44580ad18ee548e5fa20a98a61d2c6b924e6fa/packages/slate-react/src/components/content.js#L167

I was wondering have the we check if native is not zero’ed due to Safari bug

thesunny commented 6 years ago

@lxcid,

That was my initial thought as well but as for the iPad bug, I can effectively delete all that code and the window.scrollTo still doesn't work.

zGrav commented 6 years ago

@lxcid sadly, i do not believe native is the issue here :(

thesunny commented 6 years ago

@zGrav

Sorry but my iPad is simply not updating. It's new and it says it is restoring from iCloud backup. Which is odd because I don't seem to have anything that's missing from my iCloud backup on this iPad. Also tried initiating update from my Mac but after clicking the button nothing seems to be happening either.

I'll give this a go once I get home. I have another iPad there.

zGrav commented 6 years ago

No problem @thesunny , I just find it odd that you have a totally different version from Simulator and I cannot seem to get a hand on it 🤔

thesunny commented 6 years ago

Yeah, looks like simulator has all the point versions but they don't differentiate by builds.

I have a sneaking suspicion that the bug only manifests on the hardware anyways.

zGrav commented 6 years ago

Still bad :p

Apple just couldn't do this easier...

thesunny commented 6 years ago

Ooh, looks like the upgrade may actually be working now.

Not sure what changed but it's going through the Backup procedure before the upgrade now.

Will keep you updated.

thesunny commented 6 years ago

Okay, sadly this bug is still present in iOS 11.2.2 (15C202).

I think the only solution at this point that I can see is to detect iPad and don't do the scrollTo. Since it works on iPhone, we can detect iPad only. I can keep testing and if it gets fixed, we can filter based on iOS version as well.

lxcid commented 6 years ago

@zGrav @thesunny I see, I'm on my desk.

I'm trying to replicate the bug on my iPhone 6 on slatejs.org but to no avail, I know I had this bug in my work in production thus I'm interested in solving this as well.

zGrav commented 6 years ago

@thesunny works for me on iPad under 11.2 15C107... which is the Simulator version, wtf is Apple doing with HW versions?

thesunny commented 6 years ago

Works for me in the simulator too. I'm creating a CodePen independant of Slate just to see if there is something in Slate that is wonky causing this issue.

It just feels like window.scrollTo would be something that would be working!

zGrav commented 6 years ago

@thesunny can you record a gif of what you see?

thesunny commented 6 years ago

Okay, freaking confirmed this bug outside Slate.

https://codepen.io/thesunny/pen/VyQgJJ

The thing is, this doesn't work for me on my iPhone either.

However, it does work in Chrome and Safari on the Mac.

@zGrav Can you confirm that this works or doesn't work on your iPhone. Basically when you click the scroll button at the top or the bottom, it should scrollTo(0, 100) or 100 pixels down from the top.

Just checked and this is also broken on the simulator.

I wrote this code quickly so you may want to check it. I'm fearing that I did something stupid in the code that is forgiven by Mac OSX but not on iOS.

zGrav commented 6 years ago

@thesunny nope, it does not work on iOS/Simulator, good catch!

thesunny commented 6 years ago

Man, this bug is painful...

Okay, so for some clarification, the window.scrollTo is not working for anyone in the codepen in any iOS version that we've tried (which are all pretty recent).

It does work on desktop browsers, the ones I've personally tested being Chrome and Safari on an iMac.

But on the iPad in Slate, typing a character JUMPS the page to the top. So in one particular case which is the iPad, the function call does do something. It scrolls to (0,0).

So, at best, we can say that window.scrollTo is unpredictable. Sometimes it does nothing (on the codepen on iOS), sometimes it goes to (0,0) on the iPad on hardware. @zGrav it sounds like on your iPhone it did work properly though to some degree? Is that correct. Or was this simply not doing any scrolling and scrollTo was broken there as well?

I wonder if the best course of action is just to disable this first on all iOS devices, commit that, then see if we can find a proper solution. At least this way the editor is useful for everybody if a little inconvenient...

zGrav commented 6 years ago

@thesunny could of just been broken or a cache mishap 👎

I believe iOS Safari just has... multiple issues when it comes to a plethora of things

skogsmaskin commented 6 years ago

Not seeing this behavior on my iPad3 Mini (iOS 11.0.3) The Codepen example is broken though (i think it's another issue).

thesunny commented 6 years ago

I submitted my PR that I had prepared earlier for now.

Note: Don't have Collaborator access yet...

thesunny commented 6 years ago

@skogsmaskin For clarity, you aren't seeing the scroll working (i.e. scrollTo is broken) or you aren't seeing the bug (i.e. scrollTo works)?

skogsmaskin commented 6 years ago

I can't replicate the original bug as in the opening post. In Slate it works just as expected.

zGrav commented 6 years ago

Flushed out all my cache on Simulator, iPhone, even grabbed my mother's iPad, I am now seeing the same result as @skogsmaskin . Everything is working as expected.

skogsmaskin commented 6 years ago

@thesunny Have you tried debugging it via USB on a desktop Safari (developer console)?