fix editing issues in Firefox #49
Conversation
|
Awesome! Give me some time to review it. |
|
Hello once again! Do you feel like you can give some kind of estimation about when you might be a able to take a look at this? I just wanted to be able to know so I can decide whether I’ll wait for it to be merged, or find some way to work around the fixed problems for my project. I know you might be busy, and I hate to potentially sound annoying, but these changes are important to me. |
|
Hi, I’ll try to review changes at this weekend. Is it okay? |
|
Yeah, that’s fine by me! Of course it’s okay, it’s not like I’m trying to demand that you review it or anything. 😅 I do appreciate it, though! 🎉 |
|
|
||
| let {anchorNode, anchorOffset, focusNode, focusOffset} = s | ||
| // selection anchor and focus are expected to be text nodes, so normalize them | ||
| if (anchorNode.nodeType === Node.ELEMENT_NODE) { |
There was a problem hiding this comment.
error TS2531: Object is possibly 'null'.
131 if (anchorNode.nodeType === Node.ELEMENT_NODE) {
~~~~~~~~~~
| anchorNode = node | ||
| anchorOffset = 0 | ||
| } | ||
| if (focusNode.nodeType === Node.ELEMENT_NODE) { |
There was a problem hiding this comment.
error TS2531: Object is possibly 'null'.
137 if (focusNode.nodeType === Node.ELEMENT_NODE) {
~~~~~~~~~
| // selection anchor and focus are expected to be text nodes, so normalize them | ||
| if (anchorNode.nodeType === Node.ELEMENT_NODE) { | ||
| const node = document.createTextNode('') | ||
| anchorNode.insertBefore(node, anchorNode.childNodes[anchorOffset]) |
There was a problem hiding this comment.
anchorNode.childNodes[anchorOffset]
This code is strange, childNodes is a list of elements and anchorOffset is a character offset. From my understanding it's not possible to use it this way.
There was a problem hiding this comment.
The meaning of anchorOffset is overloaded depending on the nodeType of the anchorNode.
Per MDN on anchorOffset, emphasis mine:
Returns a number representing the offset of the selection's anchor within the
anchorNode. IfanchorNodeis a text node, this is the number of characters withinanchorNodepreceding the anchor. IfanchorNodeis an element, this is the number of child nodes of theanchorNodepreceding the anchor.
On Chrome, this appears to only have an impact when modifying those properties. (anchorNode is only ever set to a text node by the browser), but Firefox makes use of this overloaded definition, and sometimes sets the anchorNode to an element, setting the anchorOffset appropriately.
(Analogously for focusNode and focusOffset.)
This is per my third bullet item:
Normalize the selection before using it. Firefox will occasionally report elements (rather than text nodes) as selection endpoints […]
I suppose the comment I wrote wasn’t clear enough, though. What I meant was that the following code expects for the anchor and focus to be text nodes. (Which is not necessarily the case in Firefox.) I can rewrite the comment if you think it’s important enough.
There was a problem hiding this comment.
Got it. Yes, please, update the comment.
There was a problem hiding this comment.
Also is there a way to test this issue? Is there some reliable way in firefox to force the selection to be on an element node?
| const pos: Position = {start: 0, end: 0, dir: undefined} | ||
|
|
||
| let {anchorNode, anchorOffset, focusNode, focusOffset} = s | ||
| // selection anchor and focus are expected to be text nodes, so normalize them |
There was a problem hiding this comment.
Please, clarify, when this can happen and why this part of the code is needed?
|
Would whenever highlight is called, the cursor jumping back to 0:0 also be these issues? in that case it also happens with edge along with some other issues. |
|
|
I pushed it manually. Merged! |
2 participants
By request, I’m moving the patch in #19 (comment) to a pull request.
This pull request aims to fix important usability features in Firefox and other Gecko browsers.
Changes include:
contenteditable="plaintext-only".0) of a text node containing only a line break. Firefox expects the selection to be at the end of the previous node in those cases, and will misbehave otherwise.keyupevent is dispatched. Modifying the selection seems to suppress this behavior, so it suffices to writerestore(save())during thekeydownevent dispatch.