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.

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

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

Closed
thesunny opened this issue Jan 9, 2018 · 75 comments
Closed

Comments

@thesunny
Copy link
Collaborator

thesunny commented Jan 9, 2018

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
Copy link
Collaborator

zGrav commented Jan 9, 2018

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

Will investigate.

@zGrav
Copy link
Collaborator

zGrav commented Jan 9, 2018

@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
Copy link
Collaborator Author

thesunny commented Jan 9, 2018

@zGrav @ianstormtaylor Additional details:

  • The jump happens no matter what character is typed. It can be a letter, space or carriage return for example.

  • I can confirm that it does not appear in the simulator (initially tried to replicate it there and failed)

  • I've been trying to diagnose, fiddling with scroll positions and such, but what seems to completely break this is that window.scrollTo(x, y) line ALWAYS scrolls to the top no matter what the value of y is. So, for example, I hard-coded 100, -100, and various other values and they all scroll to the top.

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
Copy link
Collaborator

zGrav commented Jan 9, 2018

@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
Copy link
Collaborator

zGrav commented Jan 9, 2018

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

We should be good to go now.

@thesunny
Copy link
Collaborator Author

thesunny commented Jan 9, 2018

@zGrav

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

@zGrav
Copy link
Collaborator

zGrav commented Jan 9, 2018

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

@thesunny
Copy link
Collaborator Author

thesunny commented Jan 9, 2018

@zGrav More details...

  • I removed all the white space in the demos and the bug still appears

  • I tried it in the "Plain Text" demo which appears to have no plugins and is almost as plain as you can make it and I still get the jumpy scroll.

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
Copy link
Collaborator Author

thesunny commented Jan 9, 2018

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
Copy link
Collaborator

zGrav commented Jan 9, 2018

@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
Copy link
Collaborator Author

thesunny commented Jan 9, 2018

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
Copy link
Collaborator

zGrav commented Jan 9, 2018

@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
Copy link
Collaborator Author

thesunny commented Jan 9, 2018

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
Copy link
Collaborator Author

thesunny commented Jan 9, 2018

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
Copy link
Collaborator Author

thesunny commented Jan 9, 2018

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
Copy link
Collaborator

zGrav commented Jan 9, 2018

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

@zGrav
Copy link
Collaborator

zGrav commented Jan 9, 2018

@thesunny
Copy link
Collaborator Author

thesunny commented Jan 9, 2018

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
Copy link
Collaborator

zGrav commented Jan 9, 2018

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
Copy link
Collaborator Author

thesunny commented Jan 9, 2018

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
Copy link
Collaborator Author

thesunny commented Jan 9, 2018

Okay, I'm going ahead with the update.

@zGrav
Copy link
Collaborator

zGrav commented Jan 9, 2018

@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
Copy link
Collaborator

zGrav commented Jan 9, 2018

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
Copy link
Collaborator Author

thesunny commented Jan 9, 2018

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

@thesunny
Copy link
Collaborator Author

thesunny commented Jan 9, 2018

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

Details:

  • My iPad is on 11.1 (15B93) exhibits bug
  • iPhone is on 11.2.1 (15C153)
  • Simulator iPad is on 11.1 (15B87)

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
Copy link
Collaborator Author

thesunny commented Jan 9, 2018

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...

mcasimir/mobile-angular-ui#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
Copy link
Collaborator

zGrav commented Jan 9, 2018

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.

@skogsmaskin
Copy link
Collaborator

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

@zGrav
Copy link
Collaborator

zGrav commented Jan 10, 2018

Interesting @skogsmaskin @thesunny , I am able to replicate it on landscape mode on my iPhone but not portrait.

@thesunny
Copy link
Collaborator Author

@skogsmaskin

Yeah, that's how I honed in on the window.scrollTo being the issue. Basically I just commented out everything else and saw that the window.scrollTo wasn't working properly.

I hard coded the y value and it still jumped to the top. I also did a console.log to make sure I was on that specific version of code (in case it was a caching issue).

@skogsmaskin
Copy link
Collaborator

@thesunny, right! Ok, so no expections then. This is really strange. For me it works fine on the iPad in both landscape and portrait mode. Which model of iPad do you have?

@thesunny
Copy link
Collaborator Author

@zGrav

You can probably replicate it in portrait too, you just have to type more content so that the cursor is not just under the keyboard, but below the bottom of the visible page.

I've been primarily testing in landscape though because I have to type less to get the bug to appear.

@thesunny
Copy link
Collaborator Author

@skogsmaskin

Make sure you add many lines of content to the bottom of the text or else the bug won't manifest. This is because scrolling to the top doesn't cause any ill effects since there isn't any scroll anyways.

That said, it could be a bug introduced after 11.1 since I've confirmed its existance there and zGrav and I have both confirmed it in 11.2 now...

@thesunny
Copy link
Collaborator Author

@skogsmaskin

Based on the serial number A1822 and macworld, I have:

iPad 9.7in (2017) (aka iPad, iPad 2017 or iPad fifth generation)

I just bought it a few days ago basically to solve this problem!

@skogsmaskin
Copy link
Collaborator

@thesunny Yeah, I know, but I have. I also tried with a bluetooth keyboard connected to the iPad, and it works just fine with that too (with the keyboard I can move the cursor beyond the viewport, and it scolls automatically along as it is supposed to. I will set my iPad on update before going to bed and try again tomorrow and see if I can replicate.

@thesunny
Copy link
Collaborator Author

It likely was introduced in 11.1. Does the CodePen work for you?

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

If so, I can try to fix my PR so that it only disables scrollTo on 11.1 and forward.

@zGrav
Copy link
Collaborator

zGrav commented Jan 10, 2018

@thesunny I do not see this issue on iOS 10.1

@skogsmaskin
Copy link
Collaborator

@thesunny: No, the CodePen example is broken - but I suspect that has something to do with frames and is a different bug like @zGrav mentioned.

@skogsmaskin
Copy link
Collaborator

skogsmaskin commented Jan 10, 2018

Guess it's like with all new Apple software releases nowadays - lack of proper QA. I'm still using El Capitan on my work Macbook. Just too many issues with the their latest releases.

@thesunny
Copy link
Collaborator Author

@zGrav

Sorry, earlier I misread that you can replicate on your iPhone. I though you said you could replicate on your iPad.

I can confirm as well that it's broken on iPhone in Landscape mode in the same way as the iPad. In portrait it kind of works as in the virtual keyboard covers the typing but it doesn't do the annoying jump to top.

@thesunny
Copy link
Collaborator Author

@skogsmaskin

Sorry to be a pain and you probably have already done all this but I want to make sure that you definitely don't have the problem so that we could potentially exclude that version.

  • Put your iPad in landscape orientation
  • Open up Safari on your iPad to this URL http://slatejs.org/#/rich-text
  • Make sure that when you enter text you are using the default virtual keyboard (i.e. not bluetooth and just in case not the little mini one where the keys are just on the left and right). Basically, that big ol keyboard that takes half the bottom of the iPad.
  • Start adding text to the end of the content. Maybe 10 lines or so hitting "ENTER" at the end of each line.
  • close virtual keyboard
  • click on the buttom line of text and type anything

Did everything work as expected?

@lxcid
Copy link
Collaborator

lxcid commented Jan 10, 2018

This is my test though on my phone, I do have the jumpiness, but not the scrollTo(0, 0) issue.

https://youtu.be/Yryq_q-w8yY

edit: I'm on iOS 11.2

@thesunny
Copy link
Collaborator Author

AHA! Okay, I figured out how to make this bug appear on the simulator.

This should help us all get on the same page and test better.

From the simulator menu, go: Hardware -> Keyboard -> "UNCHECK" Connect Hardware Keyboard

Successfully have this bug show up "iPad 5th generation" and "iOS 11.2"

@skogsmaskin
Copy link
Collaborator

I upgraded ios and have this issue now @thesunny. So it was working fine on 11.0.3 at least.

@thesunny
Copy link
Collaborator Author

Okay, I've tested using the simulator and have been able to replicate this bug on all versions of iOS 11 including 11.0 on iPad Air. I didn't test any other versions so it may work on some versions or on some hardware but I think given that at least one device doesn't work that we should probably include all 11.x in the list of "don't use scrollTo" exceptions.

This bug does NOT appear on iOS 10 series that I can see so I will limit to 11.

I have to go home to my family now but will update the PR tomorrow morning (maybe tonight).

If anybody else wants to do it before tomorrow though, feel free.

@skogsmaskin
Copy link
Collaborator

Great @thesunny! This makes it a lot easier to work with. 👏

@thesunny
Copy link
Collaborator Author

As an aside, this is officially the longest Issue comment thread in the history of Slate...

@lxcid
Copy link
Collaborator

lxcid commented Jan 10, 2018

Hey, I was able to replicate it and took a video with a console logging (the end of the function scrollToSelection()) and the simulator side by side.

screenshot 2018-01-10 11 18 57

The video is uploaded here https://youtu.be/VCDaAUjYlUk

Observations

  • (x, y) definitely not (0, 0)
  • The (0, 0) effect seems to affect if page is non scrollable without the keyboard, scrollable with the keyboard.
  • The cursor will always be hidden behind the keyboard, this is probably calculation bug…

@thesunny
Copy link
Collaborator Author

Nice catch @lxcid !

Didn't notice that the jumping changed depending on whether the page was or was not scrollable.

I think one thing that we can try is to force the page to always be scrollable. This may be by setting the CSS scroll value to something other than auto or it might be by setting the height to 1px more than the height of the viewport...

One thing we could do code-wise is if we are in iOS 11, we check if the element is scrollable, and disable scrollTo when it is not scrollable. Then, we can also add documentation that for the auto-scroll to work in iOS 11, you must force the Editor to be scrollable (assuming that setting the CSS value and/or manually setting the height to force scroll).

This would be a decently good solution. It won't break like it is doing now and if you did the fix (assuming it works) you will have a working autoscroll.

Of course this all assumes that the scroll to values can be calculated properly with the virtual keyboard that @zGrav is working on.

@zGrav
Copy link
Collaborator

zGrav commented Jan 10, 2018

Alas, Apple removed the resizing event on Safari 10 apparently when the keyboard pops up, so we no longer have window.innerheight changes nor a "we have a keyboard on screen" event...

This is just stupid at this point...

@thesunny
Copy link
Collaborator Author

@zGrav

I have experienced the same difficulty in dealing with the virtual keyboard on iOS.

In order to get it working for me, I made certain assumptions about the size of the virtual keyboard.

My personal measurements that will probably be close (but likely not exact) on my iOS devices is as follows:

  • iPad in Landscape: keyboard is 60% of height

  • iPhone in Landscape: 70%

  • iPad in Portrait: 35%

  • iPhone in Portrait: 50%

I feel like there are a few approaches here:

  • Take the worst case scenario and it probably will work decently if not perfectly. For example, in landscape, we assume the keyboard is 70% of the height and we auto-scroll within the 30%. In Portrait, we assume it's 50% and auto-scroll within the 50%. That means that as we hit enter, if we have a mismatch, there will be potentially be some text showing below the cursor as it's auto scrolling instead of the cursor being at the bottom of the screen.
  • Just disable auto-scroll altogether on iOS devices. This isn't that bad for editing text in the middle of the screen. Because the UI for scrolling is just a flick of the finger, it's relatively easy to adjust.
  • Disable auto-scroll unless the cursor is at the bottom of the screen (potentially last block and we hit enter?). In this scenario, we could force auto-scroll all the way to the bottom.

I admit, none of these solutions are ideal, but ANY solution is a million times better than what is happening now where you have to constantly rescroll to the bottom of the page.

@thesunny
Copy link
Collaborator Author

Ugh, spent much of today trying to get two monitors working with my mac setup.

Then went to fix the PR but hitting ENTER caused two ENTERs. Tried everything until I figured out that master is actually broken on this repo so when I merged with upstream it was broken. Anyways, for those watching this issue, I haven't forgotten but I think maybe somebody has to revert an earlier merge to fix the double ENTER bug first...

@thesunny
Copy link
Collaborator Author

Fixed by this PR

#1515

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants