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

Remove unnecessary DOM selection updates #31

Merged
merged 16 commits into from
Oct 16, 2023

Conversation

srubin
Copy link

@srubin srubin commented Oct 13, 2023

This PR contains two changes to how we update selection in leaves:

  • Don't update the selection on a leaf if it only intersects the selection on the leaf's trailing edge and there are additional leaves in the block.
  • Don't update the selection on a leaf if it only intersects the selection on the leaf's leading edge and there is an earlier leaf in the block that is included in the selection

It also contains a bigger change, which can be enabled via the globalDomSelectionUpdate prop on the root Editor component. Instead of having each leaf modify the DOM selection, the leaves instead register the selection change that they want to make. Then, once all the leaves have rendered, the Editor component updates the DOM selection only if necessary. This takes care of a few issues:

  • Ensures that if the current DOM selection is equivalent to the target DOM selection, we don't modify the DOM selection at all
  • Ensures that if multiple leaves attempt to modify the DOM selection, we only actually apply the final focus and anchor updates. That way, we don't do excess work for selection updates that will be immediately thrown away.
  • Ensures that if all we are doing is modifying the selection's focus, we can just modify the focus instead of throwing away and rebuilding the entire selection.

I guess this is a sort of "virtual DOM" for selection.

Tested with some unit tests (for the changes references at the top) and pulling this new lib into our codebase and ensuring that selections was working properly, and more performantly.

Don't update the selection on a leaf if it only intersects the selection on the leaf's trailing edge and there are additional leaves in the block.

Before this change, in this case, we were clearing the DOM selection and adding a point to it only to immediately clear it again when the following leaf processed. In this case we do not need to update the DOM selection at all (it isn't changing) so this was 100% unnecessary overhead. It has a significant performance impact in long editor states.
@srubin srubin self-assigned this Oct 13, 2023
This handles the opposite case: where the selection would be set on a node that is only on the trailing edge of the selection when there are already other leaves in the block within the selection. There's no reason that this should be added to the selection.
@srubin srubin requested a review from scottcheng October 13, 2023 21:00
Copy link

@scottcheng scottcheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚡⚡⚡

package.json Outdated Show resolved Hide resolved
src/component/selection/DOMSelectionUpdate.ts Outdated Show resolved Hide resolved
/**
* Returns whether the selection starts (if forward) or ends (if backward)
* on the trailing edge of a leaf node, and if there are additional leaves
* in the block after it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not immediately clear to me why it's important that there are additional leaves in the block after it. Maybe add a comment about it?

Also, this is more of a nit, the two pieces of logic in this function ("the selection starts/ends on the trailing edge of a node" and "there are additional leaves in the block") seem independent, it might be easier to read if they're split up? Maybe like, a function for isSelectionOnTrailingEdge, one for isSelectionOnLeadingEdge, and one that tie them together that also checks if there are additional leaves before/after it, maybe like isSelectionIntersectWithLeaf (not sure if this is the accurate semantic)?

/**
* Returns whether the selection ends (if forward) or starts (if backward)
* on the leading edge of a leaf node, and if there are additional leaves
* in the block _and in the selection_ before it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also not clear to me why here we require that the preceding leaves are in the selection, but in the function above we don't require the succeeding ones are in selection?

Copy link
Author

@srubin srubin Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Selection is processed in leaf-order. The main problem these functions are trying to solve is that there are 2 ways to represent selection if it falls on the edge of a node: the end of node n, or the beginning of node n+1. These functions standardize the approach by always biasing for the latter. But if there is no node n+1, we need to do the former.

I'll make this more clear in the documentation, possibly with a diagram.

@srubin srubin merged commit 75cebc9 into main-pojo Oct 16, 2023
3 checks passed
@srubin srubin deleted the steve/selection-update-perf branch October 16, 2023 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants