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

Safari's Selection is broken in shadow roots #414

Closed
bramkragten opened this issue Mar 4, 2021 · 47 comments
Closed

Safari's Selection is broken in shadow roots #414

bramkragten opened this issue Mar 4, 2021 · 47 comments

Comments

@bramkragten
Copy link

Selections on iOS don't seem to work well, when selecting something it is shown correctly, but when you try to type, remove or copy the selection, it will either not work, or target some other line.

Also a select all doesn't work.

The explanation is a bit poor, hope you can reproduce it. I couldn't find a lead in the code...

@marijnh
Copy link
Member

marijnh commented Mar 4, 2021

I can't reproduce this—typing or copying does the right thing when I have a selection in Mobile Safari. Select all is available in the menu when I tap the cursor, and seems to work.

@hagenuck1
Copy link

Don't know why my mention isn't being shown here... Please have a look at the gif I uploaded in the issue mentioned above.

@aptonline
Copy link

aptonline commented Mar 4, 2021

I can't reproduce this—typing or copying does the right thing when I have a selection in Mobile Safari. Select all is available in the menu when I tap the cursor, and seems to work.

I'm getting the same results when using the Codemirror 6 demo (https://codemirror.net/6/) but its failing as part of the recent Codemirror 6 implementation within Home-Assistant. Seeing the same issues mentioned.

@bramkragten
Copy link
Author

bramkragten commented Apr 16, 2021

I created a repo, it happens as soon as I use Codemirror in a shadowRoot.

The repo can be found here: (it also shows a bug in the YAML language with the ' 😅 )

https://shrub-smoggy-phosphorus.glitch.me/

Code: https://glitch.com/edit/#!/shrub-smoggy-phosphorus

@bramkragten bramkragten changed the title iOS selection issues iOS selection issues when codemirror is used in a shadowroot Apr 16, 2021
@KennethLavrsen
Copy link

KennethLavrsen commented Apr 16, 2021

I am not a JS dev so please forgive if this is irrelevant. I hope these details can add hints

when you open the edit window on e.g. an iPad and tap to place the cursor at the end of a line and hit the backspace (only way to delete text on IOS) nothing happend. If you start typing the characters are added at the beginning of the first line of text in the window. So what is happening with backspace is probably that the code thinks you are at position 0 at line 0 and behaves like that. I would assume a lost track of cursor would also create havoc when you copy and paste as the other reports describe

It seems the bug relates to the actual cursor position being lost. Only on IOS. No issue on Linux or Windows browsers

@bramkragten
Copy link
Author

@marijnh would you have any pointers where I could look for this?

@marijnh
Copy link
Member

marijnh commented Apr 20, 2021

Not really—it's probably some Mobile Safari misbehavior that I'm not aware of. I guess reproducing the issue and trying to trace the source of the problem is the only reasonable way to diagnose this.

@bramkragten
Copy link
Author

some Mobile Safari misbehavior

One of the few looking at the code 😅

I guess reproducing the issue

Is this enough for that? #414 (comment)

I tried to look at the code, but not sure where to start... If there is anything I can do to help, let me know.

@bramkragten
Copy link
Author

bramkragten commented Apr 20, 2021

@marijnh

One thing I do notice is that getSelection does not work in Safari with shadowRoot...
It will always return the <body> element, could that be the problem?

https://github.com/GoogleChromeLabs/shadow-selection-polyfill

@bramkragten
Copy link
Author

bramkragten commented Apr 20, 2021

@marijnh

OK, yeah so Safari on the desktop uses mouseEvents to get selection working, but we ignore touch interaction there (https://github.com/codemirror/view/blob/b06ae0399118f2ad2292d131f524b289e2acb090/src/input.ts#L357), so we don't get any selection data from iOS in the shadowRoot since we also can't use getSelection.

If I remove that return, it kinda starts working (I can type again!), but not fully, selections are still wrong (there is a reason it was ignored probably!)...

@marijnh
Copy link
Member

marijnh commented Apr 21, 2021

(Touch events are ignored because our code doesn't implement proper touch selection — handle dragging, and whatever custom behavior the various platforms have.)

@marijnh
Copy link
Member

marijnh commented Apr 21, 2021

Okay, so on closer look this is in fact something I've encountered with ProseMirror before. Safari still doesn't implement any reasonable way to get the selection inside a shadow root. As you found, it'll just say the selection is on document.body when it is inside a shadow root. There are some "polyfills" but they are very problematic hacks with limitations and side effects.

So yeah, this is bad. Apple apparently doesn't consider this a priority, and I'm not sure how we can work around it. Some research into whether any other contentEditable libraries are managing to work around this might be useful.

@emufan
Copy link

emufan commented Apr 21, 2021

any other contentEditable libraries are managing to work around

Hm. It worked before the last HA release, so either a change in HA or a change in the codemirror version is responsible, that it is now not working anymore. So imho there is no need to search for other libraries, because the last working version is not that far away, ;)

Before, editing, deleting, putting the cursor, etc. was working. Only the copy and paste from multiple line/block was not possible, and it copied only one line from the selection.

@bramkragten
Copy link
Author

@emufan
That isn't really a useful comment, is it?

That obviously also was not the right solution as you already mention, and one of the main targets for CodeMirror 6.

In case you did not know, the change you are talking about is a complete rewrite of CodeMirror.

@bramkragten
Copy link
Author

This might be a solution... https://jsfiddle.net/dbalcomb/oLnkyv78/ ?

Another option if you don't want to ship a polyfill with CodeMirror, is to let the user supply a getSelection function?

All these polyfills are based on bugs/hacks or deprecated functions, the odds of it breaking again are quite high...

@emufan
Copy link

emufan commented Apr 21, 2021

In case you did not know, the change you are talking about is a complete rewrite of CodeMirror.

No, was not aware of this. Sorry for that.

That isn't really a useful comment, is it?

Depending on the line above, yes or no. Thought that were no major changes and marijnh was only not asware, that it was working right before. So in this case, I thought that it is easier to search in the last working release instead of searching other libraries. And then it was imho a useful comment.

@marijnh
Copy link
Member

marijnh commented Apr 22, 2021

@bramkragten I saw that polyfill before too, but it doesn't look very confidence-inducing or complete. Allowing client code to pass an implementation would not really be any better than just having them polyfill window.getSelection themselves, no?

There's apparently a Webkit bug for this, but it doesn't seem to be taken very seriously https://bugs.webkit.org/show_bug.cgi?id=163921

@marijnh
Copy link
Member

marijnh commented Apr 22, 2021

Interestingly (and ridiculously), setting a selection inside a shadow root through the top-level Selection object does work. So the problem is just with reading the selection.

@bramkragten
Copy link
Author

It would allow for ponyfills instead of just polyfills, but other than that no...

@marijnh marijnh changed the title iOS selection issues when codemirror is used in a shadowroot Safari's Selection is broken in shadow roots Apr 23, 2021
marijnh added a commit to codemirror/view that referenced this issue Apr 23, 2021
FIX: Add a workaround for Safari's broken selection reporting when the
editor is in a shadow DOM tree.

Issue codemirror/dev#414
@marijnh
Copy link
Member

marijnh commented Apr 23, 2021

I've pushed a patch. It's not great, but it's also not entirely terrible, and should limit any potential dodginess it introduces to the shadow dom + safari situation. I only tested on desktop (Epiphany) so far, but the general problem could also be observed there in some corner cases. Please take a look and let me know how it works for you.

@bramkragten
Copy link
Author

bramkragten commented Apr 26, 2021

Thanks for the patch, it is not quite there yet though.

The selection seems to work, I can copy and paste, so does typing on a new line, but when typing at the end of an existing line, my cursor goes backward while typing instead of forwards:

RPReplay_Final1619427822.mp4

@bramkragten
Copy link
Author

Maybe a bit clearer here:

So typing the first pieces foo: goes ok, then after the first char behind the column and space, it will set the cursor to before the column. If I then move it manually back to behind the last entered char it works again. Then when I press enter, it will move the cursor back to where I last placed it.

RPReplay_Final1619428496.mp4

@marijnh
Copy link
Member

marijnh commented Apr 27, 2021

I'm not seeing those failures on an iPhone w/ iOS 14.4.2

@bramkragten
Copy link
Author

Mmmm I updated my Glitch and also don't have this behavior there. Will do some more digging.

@bramkragten
Copy link
Author

bramkragten commented Apr 28, 2021

Got a repo here: https://shrub-smoggy-phosphorus.glitch.me/

It is not just iOS btw, it is also Safari on Mac.

When using basic-setup it doesn't happen, but using the same setup manually it does... 😕

@bramkragten
Copy link
Author

bramkragten commented Apr 29, 2021

That seems to fix my issues! 😄 🎉

Thanks for all your help and work! Really appreciated!

@bramkragten
Copy link
Author

@marijnh could you create a new release with this patch so I can get it into our beta and get more feedback? (hopefully just positive this time!)

@marijnh
Copy link
Member

marijnh commented Apr 30, 2021

Sure, I've tagged 0.18.11

@hagenuck1
Copy link

hagenuck1 commented May 1, 2021

Thanks for the fast fix. I’m not able to reproduce this issue with the latest codemirror version in home assistant beta 5 anymore. (Tested with iOS 14.5)

@hagenuck1
Copy link

Oh no, I found one more issue.

If you use the cursor on the touchpad and scroll over the Edge of the Screen, and it scrolls in a Textfield the cursor gets lost and you can't type text unless you tap somewhere to type.
This happens if you scroll horizontally and vertically.
https://user-images.githubusercontent.com/15811973/116792576-d2624e00-aac1-11eb-90c2-73b8fc68a522.MOV

If you use the touchpad and to mark text above the Edge of the screen everything is working fine.
https://user-images.githubusercontent.com/15811973/116792612-0e95ae80-aac2-11eb-9e62-ecd353b72e56.MOV

@marijnh
Copy link
Member

marijnh commented May 1, 2021

Could it be that that same thing also happens without shadow DOM? If so—could you open a separate issue for it?

@bramkragten
Copy link
Author

It seems like this issue is back 😢

@habitoti
Copy link

habitoti commented Nov 7, 2021

Yup, definitely came back. Feels like it‘s being worse even. No way to e.g. insert a new line or edit. RETURN or BACK makes cursor hop instantly to pos 1,1 in the code.

@marijnh
Copy link
Member

marijnh commented Nov 7, 2021

Are the last two comments talking about Mobile Safari or Desktop? Did you verify that it is related to shadow roots? Does @codemirror/view 0.19.14 help?

@dentra
Copy link

dentra commented Nov 7, 2021

Affects Mobile Safari on iOS

@LewisSpring
Copy link

LewisSpring commented Nov 7, 2021

I'm having this issue via Home Assistant's iOS app. (home-assistant/iOS#1904)

Video: https://user-images.githubusercontent.com/40576482/140513371-9ced1440-ba98-44d9-8e74-8591d6319980.MOV

@habitoti
Copy link

habitoti commented Nov 7, 2021

Yeah, that video pretty much describes the whole pain!

marijnh added a commit to codemirror/view that referenced this issue Nov 8, 2021
Because that somehow makes beforeinput events on iOS asynchronous, breaking
our shadow-root-getSelection kludge.

FIX: Fix selection reading inside a shadow root on iOS.

Issue codemirror/dev#414
@marijnh
Copy link
Member

marijnh commented Nov 8, 2021

So, it turned out that codemirror/view@e3cdb43 broke this, because (hold on to your hat), setting contenteditable=plaintext-only makes beforeinput events asynchronous, somehow, though a -webkit-user-modify: read-write-plaintext-only style, which otherwise has the same effect, does not. And that broke our awful getSelection hack. Which nicely illustrates the stack of horrible fragility we're building on here.

Attached patch seems to help for me—please test if you have a chance.

@GrizzlyAK
Copy link

@emufan said in Apr that this worked prior to Mar HA update:

Hm. It worked before the last HA release, so either a change in HA or a change in the codemirror version is responsible, that it is now not working anymore. So imho there is no need to search for other libraries, because the last working version is not that far away, ;)

I want to point out that HA users also began experiencing another editor issue immediately following a Feb 21 update to Home Assistant Frontend that updated to use CodeMirror 6.

@marijnh could this somehow be obliquely related to the issue we discussed HERE?

@marijnh
Copy link
Member

marijnh commented Nov 9, 2021

Possibly. So did you test the patch?

@GrizzlyAK
Copy link

I just tested the editor in the latest release of HA on iOS and it still exhibits the "jump to fist line" issue. That is my only exposure to Codemirror and I do not know if this patch has been included. Sorry.

@bramkragten
Copy link
Author

@marijnh, tested the patch and seems to work great! 🎉
Thank you very much again! 🙏

Let's hope Apple gives this some love in the future 😞

@marijnh
Copy link
Member

marijnh commented Nov 9, 2021

Great, thanks for testing. I've released @codemirror/view 0.19.15

@lishid
Copy link

lishid commented Nov 10, 2021

Wanted to chime in that I've ran into a similar issue with the new contenteditable=plaintext-only where the users were unable to focus on the text box entirely on iOS (keyboard does not pop up). I didn't file an issue because I couldn't repro on the official cm6 website and instead worked around the issue by forcing contenteditable=true and adding the css rule equivalent. So... thanks for the fix!

@marijnh
Copy link
Member

marijnh commented May 4, 2023

The Safari issue has been marked fixed a few months ago, but Safari 16.4.1 unfortunately still has the bug.

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

No branches or pull requests