Skip to content
This repository has been archived by the owner on Jun 15, 2021. It is now read-only.

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

Open
sauerbraten opened this issue Oct 30, 2018 · 5 comments · May be fixed by #527
Open

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

sauerbraten opened this issue Oct 30, 2018 · 5 comments · May be fixed by #527

Comments

@sauerbraten
Copy link

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
Copy link
Contributor

@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: #512 (comment)

@sauerbraten
Copy link
Author

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
Copy link
Contributor

@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
Copy link
Author

@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
Copy link
Contributor

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.

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

Successfully merging a pull request may close this issue.

3 participants