Skip to content
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

Chrome 61+ unreliable cursor position after replaceWith() #710

Closed
1 task done
Thinkscape opened this issue Oct 16, 2017 · 12 comments
Closed
1 task done

Chrome 61+ unreliable cursor position after replaceWith() #710

Thinkscape opened this issue Oct 16, 2017 · 12 comments

Comments

@Thinkscape
Copy link

Thinkscape commented Oct 16, 2017

Issue details

With Chrome v 61 and up, there's a new glitch with cursor positions.

  1. Dispatch a tr.replaceWith() transaction on a document with 1 node.
  2. The transaction replaces a single node with array of 2 nodes: [ customInlineNode, text(" ") ]
  3. By default PM will put the cursor after the space in the text (position 3)
  4. The state.selection.$anchor.pos === 3 which is correct.
  5. On Chrome (only) the browser's cursor appears BEFORE the space.
  6. Pressing RIGHT ARROW will move the cursor to the correct position (3) but without dispatching a new transaction (because the position is already 3).

Steps to reproduce

https://chrome-selection-offsets-glitch.glitch.me

Please specify which version of ProseMirror you're running

ProseMirror v[ 0.24 ]

Affected platforms

  • Chrome v61 (stable) v62 (beta)

Screenshots / Screencast (Optional)

http://g.recordit.co/oC0A2z0Rd0.gif

Investigation

The critical part is in prosemirror-view/src/viewdesc.js:

ViewDesc.prototype.setSelection = function (anchor, head, root) {
// [...]
  var anchorDOM = this.domFromPos(anchor), headDOM = this.domFromPos(head)

// The below will get current document's selection
  var domSel = root.getSelection(), range = document.createRange()

// Here we decide if we need to extend the selection or keep the one that browser has provided us with
  if (isEquivalentPosition(anchorDOM.node, anchorDOM.offset, domSel.anchorNode, domSel.anchorOffset) &&
      isEquivalentPosition(headDOM.node, headDOM.offset, domSel.focusNode, domSel.focusOffset))
    { return }

The value of domSel in the same moment in time is:

  • In Safari:
    {anchorNode: <p>, anchorOffset: 1, focusNode: <p>, focusOffset: 1, isCollapsed: true, …}
  • In Chrome:
    {anchorNode: p, anchorOffset: 2, focusNode: p, focusOffset: 2, isCollapsed: true, ...}

Chrome will return early at the isEquivalentPosition() condition, while other browsers will continue to line 371 where this happens:

  // Selection.extend can be used to create an 'inverted' selection
  // (one where the focus is before the anchor), but not all
  // browsers support it yet.
  if (domSel.extend) {
    range.setEnd(anchorDOM.node, anchorDOM.offset)
    range.collapse(false)
  } else {
    if (anchor > head) { var tmp = anchorDOM; anchorDOM = headDOM; headDOM = tmp }
    range.setEnd(headDOM.node, headDOM.offset)
    range.setStart(anchorDOM.node, anchorDOM.offset)
  }
  domSel.removeAllRanges()
  domSel.addRange(range)
  if (domSel.extend)
    { domSel.extend(headDOM.node, headDOM.offset) }
@marijnh
Copy link
Member

marijnh commented Oct 16, 2017

This doesn't appear to be happening on Chrome 61 Linux. But maybe I have a different version (61 is currently unstable on Linux). Which platform did you test on?

@Thinkscape
Copy link
Author

Thinkscape commented Oct 16, 2017 via email

@kapouer
Copy link

kapouer commented Oct 16, 2017

Reproduced on Chromium Version 61.0.3163.100 (Build de développement) built on Debian buster/sid, running on Debian buster/sid (64 bits).

@Thinkscape
Copy link
Author

Same happens on Windows 10 with Chrome 61:
image

@marijnh
Copy link
Member

marijnh commented Oct 18, 2017

I can reproduce this now. When it happens, the cursor position that the browser is displaying does not correspond to the cursor position that it is reporting—it claims that the cursor is at offset 2 in the paragraph node, but the cursor is blinking (and typing happens) directly after the custom node (which would be offset 1).

So this seems to be a browser bug. Then the question is whether we can work around it. That test you show, which prevents DOM selection updates when the DOM selection is already where it should be, isn't just an optimization—it is critical to avoid disrupting spell checking, horizontal position during vertical motion, and other pieces of hidden selection state. So we can't just turn it off.

@marijnh
Copy link
Member

marijnh commented Oct 18, 2017

I've reproduced this outside of ProseMirror with the following code:

<!doctype html><meta charset="utf-8">

<div contentEditable=true style="white-space: pre-wrap"><p>a</p></div>

<button onmousedown="runTest(); return false">Click</button>

<script>
let p = document.querySelector("p")
// Set the selection to the last index in the paragraph
let s = getSelection(), r = document.createRange()
r.setEnd(p, 1)
r.setStart(p, 1)
s.removeAllRanges()
s.addRange(r)

function runTest() {
  p.insertBefore(document.createTextNode("x"), p.firstChild)
  console.log("Selection is now supposedly at:", getSelection().anchorNode, getSelection().anchorOffset)
}
</script>

@marijnh
Copy link
Member

marijnh commented Oct 18, 2017

Reported as https://bugs.chromium.org/p/chromium/issues/detail?id=775939

Found that it doesn't happen when the selection is inside of a text node, rather than at the top of the paragraph, but there's no way to ensure that without causing the aforementioned selection reset issues.

@Thinkscape
Copy link
Author

Thanks for looking into that. I understand the limitations...
I'm trying to find a workaround for our use-case as it affects a huge chunk of our customers and is a quite common interaction (inserting the node into document and continuing typing after space).

@marijnh
Copy link
Member

marijnh commented Oct 19, 2017

If you have specific code that's typically running when the problem happens, you could, until the Chrome bug is fixed, add a kludge that resets the DOM selection—moving it into a nearby text node should prevent the issue from occurring.

@Thinkscape
Copy link
Author

I've tested a few approaches, but decided on one of the simplest:

    // prepare transaction with the content insertion
    const tr = state.tr.replaceWith(start, end, nodes);

    // do this right before we dispatch PM transaction
    if (isChromeWithSelectionBug) {
      document.getSelection().empty();
    }

    // now viewdesc will have to re-apply selection putting it in the right spot
    view.dispatch(tr);

I do it conservatively for the identified use-cases when we inject inline block nodes into document, so that we don't interfere with spellcheckers in all other cases.

@mitar
Copy link

mitar commented Mar 18, 2018

Interesting is that comment here is saying that workaround should not work.

marijnh added a commit to ProseMirror/prosemirror-view that referenced this issue Nov 9, 2018
FIX: Work around a Chrome bug where programmatic changes near the cursor
sometimes cause the visible and reported selection to disagree.

Issue ProseMirror/prosemirror#710
@marijnh
Copy link
Member

marijnh commented Nov 9, 2018

Attached patch tries to kludge around this. It's super-ugly, but covers this case and hopefully most other related ones. In case anyone wants to test it, I'm going to hold off releasing it a few more days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants