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

Fix selections with non-void non-editable focus #5716

Merged
merged 5 commits into from
Sep 12, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/brown-ears-tap.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'slate-react': patch
---

Fix selections with non-void non-editable focus
4 changes: 1 addition & 3 deletions packages/slate-react/src/components/editable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -255,9 +255,7 @@ export const Editable = forwardRef(
ReactEditor.hasEditableTarget(editor, anchorNode) ||
ReactEditor.isTargetInsideNonReadonlyVoid(editor, anchorNode)

const focusNodeSelectable =
ReactEditor.hasEditableTarget(editor, focusNode) ||
ReactEditor.isTargetInsideNonReadonlyVoid(editor, focusNode)
const focusNodeSelectable = ReactEditor.hasTarget(editor, focusNode)
Copy link
Contributor Author

@TyMick TyMick Sep 9, 2024

Choose a reason for hiding this comment

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

I'm not sure focusNodeSelectable is still the best name for this variable, since the non-void non-editable nodes still aren't selectable. ReactEditor.toSlateRange just changes the focus to a node that is selectable now. Can't think of a better name at the moment, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, this PR only fixes non-selectable focus points now, but should non-selectable anchor points be fixed as well? It wouldn't be necessary for my project, but conceivably, if a non-void non-editable node contained non-interactive text, it would be easy to make a browser selection with the anchor inside that node and the focus outside it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing non-selectable anchor points would add extra complexity in that we'd have to decide what to do when a selection starts and ends in the same non-selectable node... This PR is already an improvement on the existing behavior as-is, so maybe fixing anchors can be saved for a future PR? 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the variable name to focusNodeInEditor with 98630d1.


if (anchorNodeSelectable && focusNodeSelectable) {
const range = ReactEditor.toSlateRange(editor, domSelection, {
Expand Down
62 changes: 49 additions & 13 deletions packages/slate-react/src/plugin/react-editor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import {
DOMText,
getSelection,
hasShadowRoot,
isAfter,
isBefore,
isDOMElement,
isDOMNode,
isDOMSelection,
Expand Down Expand Up @@ -244,6 +246,11 @@ export interface ReactEditorInterface {
options: {
exactMatch: boolean
suppressThrow: T
/**
* The direction to search for Slate leaf nodes if `domPoint` is
* non-editable and non-void.
*/
searchDirection?: 'forward' | 'backward'
}
) => T extends true ? Point | null : Point

Expand Down Expand Up @@ -681,9 +688,10 @@ export const ReactEditor: ReactEditorInterface = {
options: {
exactMatch: boolean
suppressThrow: T
searchDirection?: 'forward' | 'backward'
}
): T extends true ? Point | null : Point => {
const { exactMatch, suppressThrow } = options
const { exactMatch, suppressThrow, searchDirection } = options
const [nearestNode, nearestOffset] = exactMatch
? domPoint
: normalizeDOMPoint(domPoint)
Expand All @@ -702,6 +710,13 @@ export const ReactEditor: ReactEditorInterface = {
potentialVoidNode && editorEl.contains(potentialVoidNode)
? potentialVoidNode
: null
const potentialNonEditableNode = parentNode.closest(
'[contenteditable="false"]'
)
const nonEditableNode =
potentialNonEditableNode && editorEl.contains(potentialNonEditableNode)
? potentialNonEditableNode
: null
let leafNode = parentNode.closest('[data-slate-leaf]')
let domNode: DOMElement | null = null

Expand Down Expand Up @@ -778,6 +793,35 @@ export const ReactEditor: ReactEditorInterface = {
offset -= el.textContent!.length
})
}
} else if (nonEditableNode) {
// Find the edge of the nearest leaf in `searchDirection` from the
// non-editable node
const leafNodes = Array.from(
editorEl.querySelectorAll(
// This editor's leaves (not those of a nested editor)
'[data-slate-leaf]:not(:scope [data-slate-editor] [data-slate-leaf])'
)
)
leafNode =
(searchDirection === 'forward'
? leafNodes.find(leaf => isAfter(nonEditableNode, leaf))
: leafNodes.findLast(leaf => isBefore(nonEditableNode, leaf))) ??
null
TyMick marked this conversation as resolved.
Show resolved Hide resolved

if (!leafNode) {
offset = 1
} else {
TyMick marked this conversation as resolved.
Show resolved Hide resolved
textNode = leafNode.closest('[data-slate-node="text"]')!
domNode = leafNode
if (searchDirection === 'forward') {
offset = 0
} else {
offset = domNode.textContent!.length
domNode.querySelectorAll('[data-slate-zero-width]').forEach(el => {
offset -= el.textContent!.length
})
}
}
}

if (
Expand Down Expand Up @@ -978,18 +1022,6 @@ export const ReactEditor: ReactEditorInterface = {
focusOffset--
}

// COMPAT: Triple-clicking a word in chrome will sometimes place the focus
// inside a `contenteditable="false"` DOM node following the word, which
// will cause `toSlatePoint` to throw an error. (2023/03/07)
if (
'getAttribute' in focusNode &&
(focusNode as HTMLElement).getAttribute('contenteditable') === 'false' &&
(focusNode as HTMLElement).getAttribute('data-slate-void') !== 'true'
) {
focusNode = anchorNode
focusOffset = anchorNode.textContent?.length || 0
}
TyMick marked this conversation as resolved.
Show resolved Hide resolved

const anchor = ReactEditor.toSlatePoint(
editor,
[anchorNode, anchorOffset],
Expand All @@ -1002,11 +1034,15 @@ export const ReactEditor: ReactEditorInterface = {
return null as T extends true ? Range | null : Range
}

const focusBeforeAnchor =
isBefore(anchorNode, focusNode) ||
(anchorNode === focusNode && focusOffset < anchorOffset)
const focus = isCollapsed
? anchor
: ReactEditor.toSlatePoint(editor, [focusNode, focusOffset], {
exactMatch,
suppressThrow,
searchDirection: focusBeforeAnchor ? 'forward' : 'backward',
})
if (!focus) {
return null as T extends true ? Range | null : Range
Expand Down
18 changes: 18 additions & 0 deletions packages/slate-react/src/utils/dom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -337,3 +337,21 @@ export const getActiveElement = () => {

return activeElement
}

/**
* @returns `true` if `otherNode` is before `node` in the document; otherwise, `false`.
*/
export const isBefore = (node: DOMNode, otherNode: DOMNode): boolean =>
Boolean(
node.compareDocumentPosition(otherNode) &
DOMNode.DOCUMENT_POSITION_PRECEDING
)

/**
* @returns `true` if `otherNode` is after `node` in the document; otherwise, `false`.
*/
export const isAfter = (node: DOMNode, otherNode: DOMNode): boolean =>
Boolean(
node.compareDocumentPosition(otherNode) &
DOMNode.DOCUMENT_POSITION_FOLLOWING
)
Loading