[view] Occasional "undefined is not an object (evaluating 'e.contentEditable')" error in Safari

[hubgit]

Describe the issue

I haven't been able to reproduce this error, so I'm not sure what triggers it, but we're seeing a reasonably-frequent logging of an error in @codemirror/view:

undefined is not an object (evaluating 'e.contentEditable')

The error is triggered here:

https://github.com/codemirror/view/blame/3f1b991f3db20d152045ae9e6872466fc8d8fdac/src/dom.ts#L304

That code was added here, around the time the error started being logged:

codemirror/view@d7312a2

From the logs, this seems to only be triggered in Safari (versions 16 and 15, at least) on macOS.

Would it be possible to add a line that handles the situation where prev is undefined, to avoid this error?

Browser and platform

Safari, macOS

Reproduction link

No response

[marijnh]

Interesting. prev is the result of node[offset - 1] in an if where offset is non-zero. node and offset originally come from the selection, which is supposed to have have a valid DOM position, and are only updated by offset = domIndex(node); node = node.parentNode, which again seems like it should always produce a valid DOM position (except is parentNode is null, but that would create a different crash).

So either Safari is reporting an invalid position in the selection, or ProseMirror's selection object is out of date, or the DOM is being sneakily mutated by the browser as we are in the process of inspecting it (it's happened before).

If all else fails, I guess we could add a check for this and bail out here without too much adverse effects, but if this is a situation where we end up with the wrong selection range, that would indicate a bigger problem. Do you know if the editor is inside a shadow root when this occurs? This could be related to a kludge to work around Safari's lack of working selection object in shadow roots.

[marijnh]

It seems the Safari issue that is the reason for the hack that might be causing this has been closed a few months ago. I am not well-versed enough in Apple release cycles to know what Safari version that fix will end up in, though.

[hubgit]

Do you know if the editor is inside a shadow root when this occurs?

The editor is not inside a shadow root.

I'll try to spend some time in Safari and see if I can reproduce the error.

[hubgit]

I haven’t yet got a reliable reproduction for this, but I’ve managed to catch the exception in Safari in a couple of situations:

• where selection.focusNode is a div with class "cm-content", selection.focusOffset is 3, but the focused node only has 1 child node
• where selection.focusNode is a div with class "cm-line", selection.focusOffset is 4, but the focused node only has 3 child nodes.

Seeing selection.focusOffset defined as a number of characters, I wonder whether it might be inappropriate for use as a child node index?

[marijnh]

focusOffset's meaning depends on the type of focusNode—if it's an element, it's a child index, if it's a text node, it's a string offset.

focusOffset pointing beyond the end of focusNode should definitely not happen. If you can catch this again, can you see if window.getSelection()'s focusNode/focusOffset correspond to the selection being used in the code there? It is synced from there a few lines before the trace happens, but the question is whether that syncing somehow goes wrong, or whether the browser randomly updated the selection in that time.

[hubgit]

Apologies for the incomplete nature of the bug report still - I'm able to trigger it by dispatching a remote change that deletes all the content in the doc while the selection’s inside a snippet highlight, which isn't exactly minimal.

I can try to answer questions as much as possible:

can you see if window.getSelection()'s focusNode/focusOffset correspond to the selection being used in the code there?

window.getSelection() appears to be the same as selection at the point the exception is thrown:

This is where the exception is thrown, after dispatching the change:

This is what selection looks like:

This is what focusNode looks like:

I found an issue which may or may not be relevant; it at least contains a jsfiddle showing that Safari updates anchorOffset differently from Chrome and Firefox.

[marijnh]

 window.getSelection() appears to be the same as selection at the point the exception is thrown

Then I suppose we are looking at a Safari bug (a Selection should never contain an invalid DOM position), and I guess we have to find some way to mitigate it without crashing.

Attached patch adds validation for these offsets. Could you see if you can apply that and then try to reproduce this again?

[hubgit]

Thank you - it looks like I can no longer reproduce the issue in Safari, using v6.11.2 which includes this patch.