-
-
Notifications
You must be signed in to change notification settings - Fork 337
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
Consider whether to generate state updates for each composition update #543
Comments
Looks like
|
I did some work on this but haven't finished anything yet. The main issue is that, if we want people to be able to treat the view as consistent (state and DOM match, so that coordsAtPos etc work), we need to somehow create a view on the DOM that is consistent with its updated state, but without actually mutating the DOM, since that'll reset the composition. This is tricky, especially if you add the fact that there could be other (programmatic, collab) transactions coming in during a composition, which, if we want to display them, would require the DOM to be mutated. |
I don't plan on using |
Any decorations you added/removed won't be updated. What did you want to use the updated state for? Wasn't it some kind of UI update? |
Android w/ spellcheck or suggestions enabled causes the following issues with composition:
In testing, I found that disabling |
This goes for any command, right? Or are you talking about inputrule-based list wrapping?
Yes, that's a similar problem to the This is a major issue that I'll be putting some more energy into at some point, but right now I have a bunch of deadlines coming up that'll have to take priority. |
I'm testing with |
Ditto. |
I don't know what the current status of this is, but maybe it's a good case for an RfC? |
Yes, it would be. If I get to this, I'd probably see what I can implement first, and then draft an RFC. If anyone is interested in picking this up, that'd probably be the way to go for them too (the main work would be finding a way to keep view descs consistent during composition—without resetting the composition by touching the wrong parts of the DOM). |
I'm interested in using prosemirror as messages editor in my forum software. Obviously editor crashing on some android devices is a no-no for internet forum, but I've started wondering, is there a way to test client for supporting required behavior before loading Prosemirror, or I have no choice but to go nuclear on all devices with I would still have to implement fallback editor that's just textarea with buttons for shortcut actions for users that don't like WYSIWYM experience, so "making dumbed down fallback is not worth it" doesn't matter for me ;) |
@marijnh Any updates on this? I am encountering (what appears to be) this problem as well on Android, even on the demos on prosemirror.net (once they are edited to not contain any initial content, anyway). I've noticed other Android bugfixes that went into some of the more recent releases, but haven't seen anything which addresses the problems here or in the related closed issues. |
@steveccable Are you sure the problem you're seeing is related to this issue? Could you describe the actual problem? |
@marijnh I've been seeing a few issues, all specifically on Android:
This second issue is presenting differently between my application rendering in a WebView and running the sample in Chrome on Android. Both display problems, but the specific behavior is what was happening in my app. If it is unrelated to this issue, I can open a new issue and place some sample code in there. Main reason I didn't already is because I'm unsure how tied together all the problems I'm seeing are, and the above definitely looks like part of this issue.
What I have found through debugging is that on Android when you focus into an empty editor, the selection state is AllSelection, and it stays that way until you either click inside of some text or add whitespace. This differs from on a desktop web browser, where it goes from AllSelection to TextSelection as soon as you start typing. Oddly enough, when using a hardware keyboard instead of a soft keyboard, Android is behaving the same as web. |
An addendum: the main difference between my code and the sample is just that after toggling the mark I call |
@steveccable I don't think these are really related to this issue, sounds more like cursor wrappers aren't working as intended on Android—could you please file separate issues? |
Status update on this: Yes, it's probably something that should be implemented, but it's really hard (updating the DOM without disrupting the composition is tricky, and sometimes impossible). We're working on a new version of CodeMirror where we try to get this right from the beginning, and if that works out, I hope to be able to port that solution to ProseMirror. But for the time being, I'm very busy, and it will be a while before there will be progress on this. |
I'm interested in helping solve this issue. I've written a react component as part of my slashr-react library. It works fantastic, except for this. These issues make it a tough sell for production. If you give me some background and point me in the right direction I'd be more than happy to at least try to figure out a hack. |
The problem is that if we programmatically set the selection during a composition, it'll reset the composition. This means we can't redraw the DOM around the cursor. Right now, this is solved by putting the view on hold during composition—it won't read from the DOM or update the DOM until the composition finishes. I do not know what a solution would look like. We could read from the DOM during composition, and update the state accordingly, but not redraw the DOM. But that wouldn't really solve the issue—even if plugins can respond to state changes, if the DOM stays in its old state, the interface will still appear unresponsive. |
When I get a chance I'll do some debugging in an emulator. Research why Android is behaving differently and if there are any vendor specific tricks. If pm isn't really the issue, then I don't think trying to change pm logic is a viable solution. Is this chrome specific, or does it persist in webview? |
ProseMirror is absolutely the issue, I think. What Android does is use composition for all virtual keyboard input, which makes the issue extremely visible there, but the same problem exists for IME input for languages like Japanese and Korean on other platforms. |
Understood, so it's an architectural compatibility issue? ProseMirror is already way more usable on mobile than draft. I'll see what I can do to help, because getting this 100% working on mobile devices would be huge. I'll let you know what I find, and if I have any questions. |
On a Pixel 2 running Android 9 / Chrome 72.0.3626.121, this version is a huge improvement!
There're probably other improvements I didn't notice in my initial quick test, but those two were immediately apparent. Unfortunately, on an iPhone 6 Plus running iOS 12.1.4, all keyboard input is extremely laggy. It can take 10+ seconds for the view to catch up with input, and Safari tends to reload the page ("This webpage was reloaded because a problem occurred.") after a short period of consecutive keypresses (hitting backspace 4-5 times in a row, or just typing at a quick - but reasonable - speed). The behavior of the return key also differs in some custom nodes, moving the cursor to the next node instead of splitting the node (as configured per keymap). |
Thanks for testing!
Weird, I'll debug this early next week. |
Unfortunately, my iPhone (mini with iOS 12.1) doesn't seem to exhibit the behavior you're describing—typing in Safari is smooth, as is backspacing out entire sentences. Return also seems to do the right thing. Are these issues something that appeared in prosemirror-view 1.9.0-prerelease or do they also show up in 1.8.3? |
The issues don't show up in 1.8.3, and I just verified they occur using the branch from GitHub directly in package.json (vs. the npm prerelease tag). I don't have access to a full macOS setup to further diagnose until this Wednesday, but I'll see if I can run some profiling via a linux host prior to that. |
Thanks for looking into that. Fixed in ProseMirror/prosemirror-view@4b14dd6 and published as 1.90-prerelease2.
Interesting. During normal typing, those functions should already have no effect—only when a key that's bound to some command is pressed should these handlers step in at all. Or when an arrow function takes you to a node selection or similar. What exactly were you doing when the predictive keyboard fails you—what happened, and what did you expect to happen? |
The only key that matters in this regard seems to be enter. Using 1.9.0-prerelease2 (thanks for the quick update!) with The two ways that the predictive keyboard behaves incorrectly are:
I believe these are essentially the same issue - the enter key press being handled by ProseMirror - but can also be observed in the prosemirror.net demo. Load prosemirror.net on an iOS device, long press to select all and backspace to clear the editor, then type "blah" (observe that it is auto-capitalized to "Blah"), hit return, and type "and". Observe that the "and" does not get auto-capitalized, and the predictive input bar above the keyboard lists This is be diverging pretty far from this issue at this point though, so let me know if this info should be added to a separate issue. It's not quite #310 (backspace updates the predictive input fine in my testing) and #627 only describes it in part (and is closed). To the original issue: Android (GBoard) is working great with this update! |
Agreed, this issue isn't the place for this discussion. Feel free to comment elsewhere or open a new issue, but in general this is really broken behavior on the part of mobile Safari, and reporting it (again) with Apple might be the only reasonable way forward. |
Thanks for this composition release, quick testing shows obvious wins and definitely fixes #576. However, we found this regression that when hit you backspace, try to edit a word which is "composing" and then hit enter, the original word appears to come back in the document, i.e. backspace does not remove the characters from that word. |
@vijaysutrave - what version of Android? I'm unable to reproduce that on a Pixel 2 running the latest release of Android 9. When I press enter after the "gh", it stays "gh". |
Same here, @vijaysutrave — doing that on my device (GBoard, Andoid 8.1) doesn't show that problem. What virtual keyboard are you running? (That tends to make a lot of difference, since the virtual keyboard software decides what composition events you get.) |
@marijnh @jordoh I tested this on a couple of devices and can reproduce this :( Steps to reproduce:
I'm adding the screencast below (using Pixel XL, Android 8.1, inside a webview): |
Strange. Still no luck getting this to show up on my device. Your latest screencast looks like you're using GBoard too, so that's likely not a factor. Are there any errors in the console when this happens? (Getting at those might require connecting your phone to a desktop chrome using USB debugging.) |
@marijnh I'm also testing the composition branch and something noticeable happens with inline nodes:
As you can see the first time I hit backspace it works, second time actually most of the time it behaves like this) it doesn't delete the node and actually duplicates text after it. That's easily reproduceable, you can take my demo example from here and link it against Is there anything wrong in my code or could this come from the composition fix? |
Also @marijnh (still from my example above), I've found an issue when hitting Enter and you're at the end of the document:
Looking at the code below I see we deliberately disregard the event when in between composition:
Is this intended? What's the magic to have similar behavior between desktop and mobile then? (btw let us know if you would like us to issue this bugs in a separate issue) |
I've put this code on the master branch and will release 1.9.0 in a few days. Testing is much appreciated—specifically, testing for regressions in regular desktop use, since quite some code changed. |
lib-commons.js:formatted:71330 Uncaught TypeError: Cannot read property 'storeDOMState' of undefined
at Pn.updateStateInner (lib-commons.js:formatted:71330)
at Pn.updateState (lib-commons.js:formatted:71298)
at n.setUserFocusedPurpleNumber (lib-commons.js:formatted:3477)
at HTMLDivElement.<anonymous> (lib-commons.js:formatted:3452)
at HTMLDivElement.<anonymous> (lib-commons.js:formatted:260) this.selectionReader.storeDOMState(e.selection)), Not sure if It's my setup (I am using quite a bit of custom plugins and decorations) but I wanted to post this here just in case any one else runs into it to support it's case as a regression in [email protected]. This is on Desktop,Chrome Version 73.0.3683.86 (Official Build) (64-bit) EDIT: It's my setup, I'm force recreating the document, I suppose, in a non-conventional way and the same error is happening on 1.8.9, feel free to delete this comment; sorry for the false alarm @marijnh |
FIX: Fix a regression where mouse selection would sometimes raise an error. Issue ProseMirror/prosemirror#543
No, that was an actual bug (via a bad merge between the composition branch and patches on the main branch). I've released 1.9.1 with a fix. |
Question about typing in Android, why is the last word not being saved if you don't click on the word after typing or pressing space? The word has been underscored. E.g Whatsapp saves the word. |
Is that with 1.9 or with a previous version? That should be one of the issues solved by this work. |
@marijnh Sorry, you are right. Good work! |
When composed text is only flushed to the state at the end of a composition, the UI can get out of sync with the visible document state during composition.
One solution could be to re-parse and update constantly during composition. This would be somewhat more expensive (but since this is still happening on human-interaction timescales, and wouldn't be much different from fast non-composed typing, that may not be an argument).
The text was updated successfully, but these errors were encountered: