ProseMirror / prosemirror

The ProseMirror WYSIWYM editor
http://prosemirror.net/
MIT License
7.61k stars 336 forks source link

After ts migration, posFromCoords in a node that lacks a contentDOM now biases towards the node's end instead of the start #1348

Closed sarahriouslydt closed 1 year ago

sarahriouslydt commented 1 year ago

I've been migrating our version of prosemirror-view from 1.23.4 to 1.29.1 and ran into a logic change that was difficult to pin down.

We have draggable nodes and the previous behaviour was when one node was dragged onto another (e.g node B was dragged over node A), node B would be inserted before node A, but now it's inserted after. This seems specifically to affect the case where the node we're dragging over (node A) lacks a contentDOM, so prosemirror tries to find a safe pos to insert the node A slice.

I did some digging and found the root cause: the posFromCaret function now calls view.docView.posFromDOM with a default bias of 1:

  return outside > -1 ? outside : view.docView.posFromDOM(node, offset, 1)

Prior to the typescript migration no bias was provided:

  return outside > -1 ? outside : view.docView.posFromDOM(node, offset)

This changes the behaviour of localPosFromDOM if no contentDOM is present. Before a default value of this.posAtStart would be returned but now it defaults to this.posAtEnd, causing the observed behaviour.

    return (atEnd == null ? bias > 0 : atEnd) ? this.posAtEnd : this.posAtStart

This is a super minor issue in the scheme of things but I wanted to raise it as it's a small logical change that may not have been intended.

I can try and provide a demo if needed but I hope that explanation is sufficient.

marijnh commented 1 year ago

Is the node you're dropping something on an inline or a block node?

marijnh commented 1 year ago

More specifically, would behavior where the chosen position depends on where the drop happens relative to the center of the node (vertically for block nodes, horizontally for inline nodes) work for you?

sarahriouslydt commented 1 year ago

Hey! Sorry, should have clarified - these are block nodes.

More specifically, would behavior where the chosen position depends on where the drop happens relative to the center of the node (vertically for block nodes, horizontally for inline nodes) work for you?

That would be fantastic and an improvement over how it works for us currently 🎉

marijnh commented 1 year ago

Attached patch implements that behavior.

jljorgenson18 commented 1 year ago

Because of this change, it seems like the pos returned from posAtCoords when inside of a table is now always returning the position just before or just after the table as opposed to the positions inside of cells. Is this expected? Maybe I am missing something but the patch seems to be missing the "break" statement that was there before https://github.com/ProseMirror/prosemirror-view/commit/5814486c2d11cb2c4b6a8810c764e500bbb55443#diff-8711f18f44af6275714f55277ddd18fb124ab348b03cd4b86a4fdb253b205330L222

Edit: Yea it seems like a break would be still be needed once you found a position inside of another node

marijnh commented 1 year ago

Does attached patch help for you?

jljorgenson18 commented 1 year ago

@marijnh I'll try first thing on Monday but conceptually I think it makes sense.