Cusor wrapper overhaul: Please help test

1

[Summary: there’s a pre-release of prosemirror-view available for a somewhat invasive but hopefully helpful change, and I’d feel better if some people could help test it before I release it.]

So, traditionally, when the cursor is in a position where the DOM inline styles of the thing before it don’t correspond to the current marks (because there’s stored marks or a non-inclusive mark before), ProseMirror would create a <span> element with a zero-width space inside of it, wrap that in the appropriate style nodes, and put the selection inside of it. (The hidden character was necessary for this to actually work—without it typed text would still receive the style of the surrounding DOM in Webkit/Chrome.)

This meant that A) ProseMirror would have to mess with the selection and DOM at unpredictable times, confusing browsers, and B) that the zero-width space inserted in the wrapper would easily end up in places where it shouldn’t be, and caused a bunch of issues (1, 2, 3, 4, 5, 6, etc).

The main reason we want DOM updates to happen inside the right marks is that, previous to prosemirror-view 1.9.0, there was a delay between the DOM change and the ProseMirror update, so you’d see the unmarked text you typed briefly and then it would snap to the proper, marked form. But since 1.9.0, DOM reading is synchronous, so that delay no longer occurs.

Only composition input does have a similar problem—there we can’t touch the DOM nodes around the cursor until the composition is finished. So that’s bad—you see the text styled in the wrong way as long as you are composing, if there is no cursor wrapper.

The experimental (and, on second thought, poorly named) no-cursor-wrapper branch removes the eager cursor wrapping, and replaces with a last-second on-demand approach that does the wrapping on the compositionstart event. This should greatly help avoiding cursor-wrapper related bugs, while still neatly hiding visual issues caused by the browser not understanding our mark model.

Still, since this was a hack that could influence editing in all kinds of ways, and contenteditable behavior can be extremely unpredictable, I’d like to see some more serious testing happen before I release this change as a regular version. The new code is available as prosemirror-view version 1.9.14-prerelease3. If you have an automatic browser test routine, or are willing to run through some tests scripts, I’d very much appreciate if you could give it a spin and let me know if you run into any issues. (Or if everything works, I’d still like to know.) Thanks!

6 Likes
2

This is directly related to a number of IME and input issues I’m working on so I can dig into this tomorrow and let you know if anything pops up. When I initially ran 1.9.14-prerelease4 through our test suite, there were a ton of failures around selection. However, we are manually flushing the dom observer in a few spots to force a sync so the problem might be on our end.

Just for context, we have about 1000 tests that actually spin up our Prosemirror view in Karma and they test close to everything in our project. I’ll let you know if there is a trend in the failures.

3

I ran through manual test scripts using 1.9.14-prerelease4 and did not noticed any issue or weird behaviour.

I will continue to use 1.9.14-prerelease4 in development environment for coming few days and I will post here if I see any issue.

4

Not sure how many browser tests I have that touch this but all of them passed.

5

I am seeing issues around losing focusing in our testing suite. Not sure why but “hasFocus” is returning false, even though it seems to be correct. I’m going to dig in more but I’m not seeing issues in the actual live instance yet.

6

@marijn The issue was due to the removal of the dom.focus call in view.focus. Removing that meant that the dom wasn’t actually being focused. I fixed it by calling dom.focus manually in the test suite, but the editor instance not being focused on view.focus could be problematic. Other than that, there were no other issues and all of our tests passed!

1 Like
7

Argh, that patch wasn’t actually related to the cursor wrapper stuff, but it did apparently break something. Do you have an example of a situation (+ browser used) where it fails to focus the editor? I haven’t been able to get that to happen.

Edit: On closer look, I can reproduce it, and my previous patch was obviously wrong. This patch should fix that regression. I’ll follow up with a new prerelease version soon.

8

There’s a 1.9.14-prerelease5 with the focus fix now.

9

Awesome, that fixed the focus problems!

The only other issue I am seeing now is in Firefox. If you do the following

  1. Hit bold to create a pending mark
  2. Type a composition
  3. Finish the composition (e.g. hitting enter in Japanese)
  4. Start another composition

The second composition does not have the mark applied until after the composition is complete. This visually shows the composition text as un-styled. This seems to be due to the Firefox moving the cursor out of the mark element when the new composition starts but the model itself has the mark applied at that text. I did not see this on Safari or Chrome.

10

Thanks for spotting that. I think I’ve fixed it with this patch, and I’ve released a 1.9.14-prerelease6 with the fix.

1 Like
11

That seems to have fixed that problem!

However, our QA team noticed some other issues with Firefox where we lost font our font mark when typing Japanese. The mark ends up as a span with a single class attribute and when typing a composition, something is triggering the mark to be removed (my guess is an extra mutation). When I changed the tag to “font” instead of span, it worked just fine. My guess is that there is something specific to spans that is causing a problem on FF.

12

Looks like the issue is related to https://github.com/ProseMirror/prosemirror-view/pull/56. It looks like Firefox fires extra mutations where it changes the style attribute value constantly, even though the updated value is null. If we skip attribute changes where the attribute name is “style” and the data is null, we should be safe. If you’d like I can create a PR and try it out.

31%20PM
Screen Shot 2019-08-09 at 12.10.31 PM.png693×471 62.4 KB

13

Yep, the fix looks like this in the CompositionViewDesc

  ignoreMutation(mut) {
   // Firefox occasionally fires mutations during compositions where the values do not change.
   // These mutations will sometimes trigger marks to be removed. We can skip them if nothing meaningful has changed.
   if(mut.type === 'characterData' && mut.target.nodeValue == mut.oldValue) {
     return true
   }
   // Firefox will often set the style attribute on spans but not actually set a value. We can ignore these mutations
   if(mut.type === 'attributes' && mut.attributeName === 'style' && !mut.target.getAttribute('style') && !mut.oldValue) {
     return true
   }
   return false
 }

I’m going to play with a few more scenarios to see if this is general enough. We might want to ignore it if the value is exactly the same as well.

EDIT:

Ok, I just found out that Firefox will fire style attribute mutations constantly, with oldValue set to “null” on all of them. It might be heavy handed, but we might want to ignore the style mutations if the oldValue is null. Either way, we could fix it on our side by filtering transactions that remove certain marks during a composition. We would just need to look for any marks that need this fix.

I added a style attribute with a test var in toDOM. The “null” is the oldValue field.

37%20PM
Screen Shot 2019-08-09 at 12.35.37 PM.png689×675 118 KB

EDIT 2: Filtering out “removeMark” transaction during a composition somehow causes an endless loop. Not sure why or how yet.

EDIT 3:

I apologize for any confusion, but the issue was due to our parseDOM functions. The style mutations from the compositions were triggering our parseDOM rules and our mark logic was getting confused. We actually use different parse methods so the problem is specific to our setup. I think we should watch out for these extra errant mutations, but I believe prosemirror-view itself is just fine.

14

What does record.target.style.cssText evaluate to when these happen?

15

I can double check on Monday but the style attribute was set to an empty string. Here is roughly what was happening.

  1. Our schema’s toDom function for font family rendered a span with a class. The parseDom was not set up to parse the result of toDom (this was our big mistake)
  2. When typing the composition, Firefox would mutate the span by adding style=''.
  3. The mutation would fire and the span would be reparsed by parseDom.
  4. Since that span ended up returning false in parseDom, the font family would be removed as a mark. It would visually snap back to the default font family after the composition is complete.

I had added a filterTransaction to ignore the mark removal during the composition, but this actually created an infinite loop and prosemirror kept reacting to the dom change and kept trying to remove the mark.

The fix was just to add a data attribute to the span so it could be reparsed if the style attribute was mutated.

16

Right, seems like the library could still prevent some wasted cycles and confusion by ignoring such events. Can you check whether this patch catches the mutations that you’re seeing?

17

I had tried something similar to that patch and although it did work for the null to empty string case, it still seemed to fire the mutation for spans even with a value set on style. I added that dummy style and it still tried to unset the style attribute and move it back to an empty string. I would honestly ignore this issue for now until another problem arises. It shouldn’t cause any underlying issues if people are using prosemirror correctly.

18

Thanks for all the feedback. I’ve released these changes as 1.10.0.

2 Likes