Selection.range collapses when selecting the last word of a paragraph

[sauerbraten]

In Firefox 62.0.3, I can't select the last word of a paragraph, then use 'createLink' to wrap that word in an anchor tag. (You can reproduce this in the demo.) It works when using just document.execCommand(), so I suspected it had to do with the createLink-patch in the core plugin.

Turns out it is a bug in Selection():

1. Firefox's document.getSelection() returns a selection with anchorNode set to a #text node containing the word and focusNode set to the parent

   node (and focusOffset set so that the selection ends after the #text node child (= the anchorNode).

2. Later in the code, the isBefore() helper function is used which does not take into account that child nodes of N come after N in the document, i.e. Selection incorrectly assumes the selection is backwards.
3. Finally, creating a new Range and calling setEnd() with the incorrectly swapped start and end results in a collapsed range as per https://developer.mozilla.org/en-US/docs/Web/API/Range/setEnd.

I suggest fixing this by checking isBefore() and Node.contains() in Selection() to prevent wrongly inverting the range. I'll prepare a pull request containing this fix.

[jukecraft]

@sauerbraten thank you for bringing this up. Just a note before you go to work, you might want to know about our current text editor plans: https://github.com/guardian/scribe/pull/512#issuecomment-413149196

[sauerbraten]

Thanks for the quick feedback @franziskas! Interesting, you don't happen to have more information about the timeline for the integration of ProseMirror?

I'll create the PR anyway, but have trouble writing a test for this or getting the demo to work correctly (npm run build and ./build-cjs.sh in a fresh checkout produced a buggy demo for me). I'm also not sure how to port the old selection.spec.js tests.

[danburzo]

@sauerbraten Could you outline the concrete reproducing steps? I'm on Firefox 63 and at first brush I couldn't reproduce it, and I'm curious to see if it's a FF 62 thing.

[sauerbraten]

@danburzo sure:

1. remove all content from the demo window
2. put in just 'foo'
3. the output area should now show <p>foo<br></p> and nothing else
4. double click the foo to select it
5. if you want, run document.geSelection() in the console to verify that the endNode is a p node, for example, I get Selection { anchorNode: #text, anchorOffset: 0, focusNode: p, focusOffset: 1, isCollapsed: false, rangeCount: 1, type: "Range", caretBidiLevel: 0 }
6. click the Link button, type "http://a.com" into the modal (or anything really)
7. confirm with enter key or by clicking OK
8. output area will now show <p><a href="http://a.com">http://a.com</a>foo<br></p> and the contenteditable area reflects this

Looking into these buggy behaviors some more, we found something else (which is probably unrelated but also occurs with links, so I'll mention it here):

The state of the Link button depends on how a link is selected. Double clicking the a.com link created above will cause in the Link button showing as "inactive" (?), i.e. pressing it will not pre-fill the modal with the link's URL. This is also the case when selecting the entire link by moving the cursor from outside the link (| a.com) to the start (|a.com) (or end), then holding Shift and using the right (or left) arrow key to select all the characters of the link. Doing so results in the same "inactive" Link button, even though a link is now selected. Selecting the whole link using Shift and arrow keys works when the cursor's last position before selecting was inside the link, i.e. when you go from a|.com to |a.com and then select to the right.

For a custom linking button we use

const isLink = node => node && node.nodeName.toLowerCase() === 'a';
const getLinkParent = sel => sel.getContaining(isLink);
command.queryEnabled = function () {
    let sel = new scribe.api.Selection();
    return !getLinkParent(sel) && !sel.range.cloneContents().querySelector('a');
};

to disable the button when the selection is inside a link, or a link is (even partly) selected. Maybe this could be used as command.queryState for the createLink command?

[jukecraft]

Thanks for the quick feedback @franziskas! Interesting, you don't happen to have more information about the timeline for the integration of ProseMirror?

@sauerbraten I apologise for the long delay in replying - we've now published a blog post that should explain in detail.