Rewrite ReactDOMSelection to use fewer ranges #9992
Conversation
|
I think this logic is right. I tested it by hand and it seemed to match in all cases I tried (in the Notes composer especially). Works with backwards selections. Apparently "old IE" (not sure which IEs) uses this path for textareas so I should probably test that too. cc @n8schloss |
| start: isBackward ? end : start, | ||
| end: isBackward ? start : end, | ||
| start: start, | ||
| end: end, | ||
| }; |
There was a problem hiding this comment.
New allocation per update? 😲
| let start = null; | ||
| let end = null; | ||
|
|
||
| function traverse(node) { |
There was a problem hiding this comment.
Seems like this could be rewritten without a closure and non-recursive by utilizing parentNode. Like the Fiber traversals.
There was a problem hiding this comment.
ok ok fine, I wanted to be lazy but I can do that too
There was a problem hiding this comment.
I think I need it this way because I need the stack of indices. Since I only need the index if node is one of [anchorNode, focusNode] I could technically do it in constant space but it would be a huge pain. Can we leave it like this?
There was a problem hiding this comment.
You could have two index variables. On for anchor and one for focus. You only increment the index if you're currently traversing anchor or focus parent.
We could leave the recursion but can we at least avoid the closure by passing anchorNode/focusNode/anchorOffset/focusOffset as arguments? There are two return values but we're already allocating for that so just return the object from the recursive function.
Also, isn't text in selection something we should know the optimal solution for? :)
When the focus is inside a contenteditable, apparently we are. |
|
Oh ok. It's not as bad as I thought. I thought we did this for non-content editable. Phew. |
sophiebits
force-pushed
the
fewer-ranges
branch
from
78c53f5
to
60507b0
Compare
|
Rewritten with no closures or recursion. I am not proud of the complexity but I think it hits all the requirements. I included a fuzz test against a much simpler (slower?) implementation, added to our test suite. jsdom doesn't support Range objects, but I copied the fuzz test code into my browser and manually compared it against our old implementation https://gist.github.com/sophiebits/2e6d571f4f10f33b62ea138a6e9c265c; with 200,000 trials no differences were found. Think this should be good. |
sophiebits
force-pushed
the
fewer-ranges
branch
from
60507b0
to
d8ca082
Compare
| while (true) { | ||
| if ( | ||
| node === anchorNode && | ||
| (anchorOffset === 0 || node.nodeType === TEXT_NODE) |
There was a problem hiding this comment.
I think this basically counts as zeugma. My code is poetry.
sophiebits
force-pushed
the
fewer-ranges
branch
from
d8ca082
to
e660bcd
Compare
| let node = outerNode; | ||
| let parentNode = null; | ||
|
|
||
| outer: while (true) { |
There was a problem hiding this comment.
I think V8 (at least the old one) deopts in some cases when it thinks it might be an infinite loop. We have more of these though so we can probably do a pass over all of them and work around it, if that's the case.
| focusOffset, | ||
| ) { | ||
| let length = 0; | ||
| let start = null; |
There was a problem hiding this comment.
-1 might help these be monomorphic in type-driven optimizations.
We heard from Chrome engineers that creating too many Range objects slows down Chrome because it needs to keep track of all of them for the case that anchor/focus nodes get removed from the document. We can just implement this calculation without ranges anyway. jsdom doesn't support Range objects, but I copied the fuzz test code into my browser and manually compared it against our old implementation https://gist.github.com/sophiebits/2e6d571f4f10f33b62ea138a6e9c265c; with 200,000 trials no differences were found.
sophiebits
force-pushed
the
fewer-ranges
branch
from
e660bcd
to
93e5b75
Compare
| length += node.nodeValue.length; | ||
| } | ||
|
|
||
| if ((next = node.firstChild) === null) { |
There was a problem hiding this comment.
node.firstChild can never be undefined?
There was a problem hiding this comment.
No, only null or an actual Node.
|
Let’s land it? |
| * This source code is licensed under the BSD-style license found in the | ||
| * LICENSE file in the root directory of this source tree. An additional grant | ||
| * of patent rights can be found in the PATENTS file in the same directory. | ||
| * |
There was a problem hiding this comment.
Thank you. Will fix in the morning.
There was a problem hiding this comment.
Maybe we need a lint rule 😄
We heard from Chrome engineers that creating too many Range objects slows down Chrome because it needs to keep track of all of them for the case that anchor/focus nodes get removed from the document. We can just implement this calculation without ranges anyway.