-
Notifications
You must be signed in to change notification settings - Fork 151
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
Spellcheck underlines flicker when typing #621
Comments
On further testing it would appear the flickering is actually caused by changing the window selection. Each selection change triggers Chrome's spellcheck to run resulting in the flicker as can be seen in this code pen https://codepen.io/lookingsideways/pen/gKpWPO?editors=1010 (As an aside, this pen also highlights how the selection change breaks spellcheck behaviour in Safari). I believe the re-render is actually a symptom of an underlying problem 😞 Digging into Quill's source code it's apparent they are using MutationObserver's to update the underlying data structure whilst leaving the DOM updates and selection up to the browser avoiding the flickering entirely. Mobiledoc-kit handles updates very differently, it captures keypresses and uses those to update the underlying data structure before re-rendering, which in turn requires the selection modification. |
ProseMirror had a similar problem, discussion is here for reference ProseMirror/prosemirror#390 I believe we'd run into similar problems as ProseMirror when adopting MutationObservers. There would still be cases where we have to replace content and re-position the cursor, immediately obvious ones:
|
Thanks for digging into this @kevinansfield. We're having the same issue at Sutori and would be happy to help getting it fixed.
When Quill updates the underlying data structure, by what mechanism is that reflected in the DOM? Or it simply by letting the browser execute its native event listener for the key press. Would it be a solution to selectively rerender based on the type of event? So
The crucial requirement for (1) is that there should never be mismatch between what the DOM shows and what the underlying Mobiledoc is. |
Hey @YoranBrondsema 👋
The DOM update is left up to the browser which triggers the MutationObserver that is then used to update the data structure, no rendering or window range updates take place. This works fine for basic text editing in Quill but certain changes that involve adding/removing markups such as toggling bold do sometimes cause a slight flicker. Medium is worth looking at because typing forward there doesn't cause flicker except for Space. Backspace will also always cause a flicker, so it seems they use mutation observers but still perform manual range updates in some cases. Maybe mobiledoc will need to adopt a similar approach to start? The steps you outlined sound like they would get rid of the flicker. The key thing is to ensure the window range is never manually updated (that's what causes spellcheck to re-run), it will be necessary to determine if that's possible in mobiledoc-kit simply by avoiding the re-render or if there are deeper changes needed. For my use case I think as long as normal forward typing doesn't flicker then that's enough. I'm coming up to the end of a dev cycle at the moment and don't have much time to dedicate to this problem but I'll try and get some time allocated to it in a few weeks. |
Hi! I think that we can indeed be pragmatic and just remove the flickering for normal typing. On a more general note, I noticed that you and your colleagues from Ghost have been responsible for quite some activity on this repository (I mean that in a good way). Mobiledoc-kit is quite central to our application too but due to more pressing things, we never got around to fixing some of the issues that you raised and we also encounter, such as this one. I think it would be good to use the current momentum to get some of these issues fixed. If we could sync our schedules somehow so that we can even find some time slots to pair over the next few weeks, I think it'll be easier to get some of the issues fixed. On my side, I think I'll take a jab at this around the same time next week, i.e. Friday afternoon. |
I'm definitely up for this 😄 I'm on holiday for a week starting this Friday but I'll be back on Ghost's editor and mobiledoc-kit as soon as I'm back. Are you on the mobiledoc-kit Slack team? It may be easier to discuss and coordinate there. |
Great! Yes, I'm on the Slack. Just ping me when you're back on it (username |
Any progress on this? I'm seeing the same behavior, and it'd be really nice to avoid it. As it is, I have a workaround, which is to turn spellcheck off on keydown, and then after N milliseconds of inactivity, turn it back on. It prevents the flicker from being obnoxious, but it also means you don't see misspellings right away, so you may miss them, etc. There's a related problem, which is that if you have a Mac with a touchbar, it flickers as you type. It's pretty distracting right in the periphery of my vision. |
When typing in a markup section any spellcheck underlines will flicker on every character change which can be rather distracting. This is possibly related to #522 but I haven't noticed slowdown, just the flickering.
Examining other editors such as Medium, Dropbox Paper, and Quill, it would seem they do not suffer the same problem because they are not re-rendering when typing. Quill is the most well behaved out of the ones I looked at, both Medium and Paper flicker on Backspace, Medium also flickers on Space.
I had a look into how to re-use elements in editor-dom rather than re-rendering with new elements. I think there are a couple of issues that need to be resolved to get rid of the flickering:
_joinSimilarMarkers
marks the existing Marker (and therefore TextNode) for deletion. Removal happens before the render which means it's not possible update the TextNode'snodeValue
in the markup renderer but a completely new TextNode has to be generated and added to the parent Marker or MarkupSectionI'm happy to dig into this but I'm interested to hear if there are any recommended approaches.
QUESTIONS:
The text was updated successfully, but these errors were encountered: