Writing Flow: Collapse range in horizontal edge check by selection direction

[aduth]

Fixes #5095

This pull request seeks to resolve an issue where it is not possible to unexpand a selection from the beginning of a paragraph block. I'm not entirely clear the intent with collapsing range in the isHorizontalEdge function, though it would seem that it should be based on the direction of the current selection itself, not necessarily the direction being considered as the edge.

Testing instructions:

Verify that various shift-selection within and across blocks works as expected, particularly the previously failing behavior:

1. Insert a new paragraph block with some text
2. Make a selection of the text from the beginning of the block
3. Press Shift+ArrowLeft to unexpand the selection
4. Note that the selection becomes unexpanded as expected

[ellatrix]

I'll have a look in a bit. Some previous art, but unsure why it is done that way: https://github.com/tinymce/tinymce/blob/d941efc13979b175e6c4c61946b631edc79b75bc/src/core/main/ts/api/dom/Selection.ts#L472-L491.

[ellatrix]

@aduth After a few attempts, I'm unsure how to test this PR? Could you explain in more detail or make a screen recording of the steps? Thanks!

[aduth]

On master: From the beginning of the paragraph, making a shift-selection to the right works fine, but while continuing to hold Shift and starting to press left to reduce the selection, nothing happens.

A bit hard to demonstrate in the GIF since you can't see me furiously trying to press Shift+Left 😄

[ellatrix]

Hm, I'm not able to get this to work... even when pressing Shift+ArrowLeft furiously. :) The selection just seems stuck which I guess should collapse on this branch. I'll have a closer look at the code.

[ellatrix]

Is the goal to set this range to the document? This is just used for calculation purposes (a clone) right?

[ellatrix]

I'm not quite sure here how the direction of a selection relates to this check. This would collapse a range at the edge of a field to the end or the start depending on direction (so in one case this will collapse to the end and the check fails), while we should check whether or not the range touches the edge? See 1bb9658

[ellatrix]

@aduth Tried something different here: 1bb9658

Benefit is that it requires less code and no cloned range.

[aduth]

@iseulde I like it, makes sense that we'd not care to consider the edge as breached if pressing arrows while having a non-callapsed selection (since the intent of this is to simply collapse in the direction of the arrow press).

Two thoughts though:

• Should this be the responsibility of isHorizontalEdge ? The name isHorizontalEdge doesn't imply that we'd be considering it strictly as in the context of an arrow press. Alternatively we could make this a combined condition isSelectionCollapsed && isHorizontalEdge, though not sure if there's a worrysome impact of calling getSelection or getRange twice.
• If we do keep it as-is, we should update the documentation for the requireCollapsedRange parameter

[aduth]

Feel free to merge your commit into this branch as well.

[aduth]

I'm going to write an end-to-end test for verifying this expected behavior.

[ellatrix]

though not sure if there's a worrysome impact of calling getSelection or getRange twice.

This should be negligible and I'm not opposed to separating that. We'd have to generalise it for all input fields.

[aduth]

A few more notes after playing with it a bit:

• Should we want to update isVerticalEdge as well?
• I think we want the inverse to be the default?

diff --git a/editor/components/writing-flow/index.js b/editor/components/writing-flow/index.js
index 48f7c08ce..25a0ba852 100644
--- a/editor/components/writing-flow/index.js
+++ b/editor/components/writing-flow/index.js
@@ -205,13 +205,13 @@ class WritingFlow extends Component {
 			// Moving from block multi-selection to single block selection
 			event.preventDefault();
 			this.moveSelection( isReverse );
-		} else if ( isVertical && isVerticalEdge( target, isReverse, isShift ) ) {
+		} else if ( isVertical && isVerticalEdge( target, isReverse, ! isShift ) ) {
 			const closestTabbable = this.getClosestTabbable( target, isReverse );
 			if ( closestTabbable ) {
 				placeCaretAtVerticalEdge( closestTabbable, isReverse, this.verticalRect );
 				event.preventDefault();
 			}
-		} else if ( isHorizontal && isHorizontalEdge( target, isReverse, isShift ) ) {
+		} else if ( isHorizontal && isHorizontalEdge( target, isReverse, ! isShift ) ) {
 			const closestTabbable = this.getClosestTabbable( target, isReverse );
 			placeCaretAtHorizontalEdge( closestTabbable, isReverse );
 			event.preventDefault();
diff --git a/utils/dom.js b/utils/dom.js
index 9e6c3f606..87ca3f18d 100644
--- a/utils/dom.js
+++ b/utils/dom.js
@@ -19,7 +19,7 @@ const { TEXT_NODE, ELEMENT_NODE } = window.Node;
  *
  * @return {boolean} True if at the horizontal edge, false if not.
  */
-export function isHorizontalEdge( container, isReverse, requireCollapsedRange = false ) {
+export function isHorizontalEdge( container, isReverse, requireCollapsedRange = true ) {
 	if ( includes( [ 'INPUT', 'TEXTAREA' ], container.tagName ) ) {
 		if ( container.selectionStart !== container.selectionEnd ) {
 			return false;
@@ -48,7 +48,7 @@ export function isHorizontalEdge( container, isReverse, requireCollapsedRange =
 		return false;
 	}
 
-	if ( ! requireCollapsedRange && ! range.collapsed ) {
+	if ( requireCollapsedRange && ! range.collapsed ) {
 		return false;
 	}

[aduth]

Specifically, another behavior I'm noticing (on master and here prior to the above changes) is that when having a selection which extends to the beginning or end of a block, pressing the arrow should still just collapse the selection, not advance the caret to the previous / next block (at least this is how it behaves in Google Docs).

[ellatrix]

@aduth If I understand you correctly, you mean this:

If my selection is forward, and I press arrow up or left, the selection should collapse and continue on the other side.

If my selection is backward, and I press arrow up or left, the selection should not collapse and continue on the other side.

[ellatrix]

Will this work on textarea and input? Should a container be provided just like isHorizontalEdge etc.?

[aduth]

Ah, it should be considered, yes.

That said, do we need to pass the container? Shouldn't there only be a single selection (ignoring Firefox's multi-selection behavior), and if so, could we determine the type of container where the selection exists within automatically?

[ellatrix]

That's a good point, and we never actually seem to check if the selection is within the container in case of an contenteditable field. I do think we should provide the container (needed at least of textarea and input) and ensure that contains a collapsed selection, not just the document in general? Or do you view this differently?

[ellatrix]

E.g. you might argue that since this is triggered by a keydown event, we can assume the selection is in the container.

[aduth]

not just the document in general

My thinking was toward the document selection in general. Like container = document.activeElement.

[aduth]

Playing with the native textarea a bit, I think directionality of the selection has no impact here.

If there is an uncollapsed selection, pressing left or right should simply collapse to the extent in the direction of the arrow press.

If there is an uncollapsed selection, pressing up or down should move upward/downward as if the selection was collapsed.

[aduth]

If there is an uncollapsed selection, pressing up or down should move upward/downward as if the selection was collapsed.

Noting that with recent changes, this isn't working as expected. Thinking implementing the end-to-end tests sooner than later can help in iterating toward the ideal behavior.

[ellatrix]

There's also selection.isCollapsed which might work too. https://developer.mozilla.org/en-US/docs/Web/API/Selection

[aduth]

There's also selection.isCollapsed which might work too.

At that point, we may as well not even have this function, since window.getSelection().collapsed is barely more verbose.

[ellatrix]

Yeah :)

const { getSelection } = window;

getSelection().isCollapsed;

[ellatrix]

Might want to update "caret" to "selection", as "caret" implies a collapsed selection I think.

[aduth]

Were you able to determine why it's the case that sometimes startOffset exceeds the number of childNodes ?

[ellatrix]

I've not encountered it. This is just to make sure it never does, if it would for some strange reason.

[aduth]

Hmm, not entirely comfortable leaving this as an unknown.

[ellatrix]

Hm, me neither. Let's remove this check and address any problems if they come up?

[aduth]

Yeah, I think so. Will give it another round of testing this morning.

[aduth]

The issue with removing the safety check can be seen when, with two paragraphs, you press ArrowLeft from the second paragraph (to the first).

Specifically, it can occur that startOffset is 1 even though the first paragraph only has a single child node (where startContainer.childNodes[ 1 ] is undefined). Based on the documentation for startOffset, this might be expected ("number of child nodes between [...]"), where perhaps the caret is not considered as being within the text node, but rather after it?

While setting a bound with Math.min avoids the error, I don't think it's returning an accurate result. In the above scenario, it returns a bounding rect for the entire text node, effectively the same box as the entire paragraph, whereas with a collapsed selection at the end of the paragraph, we'd expect it to be zero width at an X coordinate aligned to the end of the paragraph.

I'm still finding 573269e to be the most durable approach, despite the fact that it requires manipulating the DOM to retrieve the rect. I wonder if we could exhaust all reasonable attempts to retrieve the rect before using the temporary span technique only if we still cannot get a valid rect (where invalid is either undefined or one with 0'd property values).

Annoyingly, this appears to be a main point of browser inconsistency / bugginess. For example, in this snippet, adding a new line to the contenteditable causes the rect to become zero'd in latest Chrome, despite a handful of resolved Chromium bug reports which have claimed to have addressed these sorts of issues ([1], [2]).

Seeing modules like rangefix has me sad for the state of browser compatibility on these functions, and sadder that our end-to-end approach won't be able to test against a broad set of browsers. Nothing I've found seems to work perfectly, aside from bounding-client-rect which uses basically the same approach as in 573269e . If it's a point of frequent regression across non-Chromium browsers that our E2E suite can't cover well, we could always extract these out these normalization functions into a separate project which uses a more thorough end-to-end approach (e.g. Selenium).

[aduth]

TinyMCE obviously handles this well, where replacing this line:

gutenberg/blocks/rich-text/index.js

Line 662 in 049645b

rect = getRectangleFromRange( this.editor.selection.getRng() );

...with:

rect = this.editor.selection.getBoundingClientRect();

...will return the value we expect. Unsure if we can/want to be able to leverage this directly from our generic DOM utilities here.

[ellatrix]

Yeah, this is what TinyMCE does:

https://github.com/tinymce/tinymce/blob/5f1e352a539359b351573d0621693e4580aa81d1/src/core/main/ts/caret/CaretPosition.ts#L126-L224

Maybe we can use this through tinyMCE.dom.Selection.

[ellatrix]

We're also using TinyMCE's isEmpty check here.

[ellatrix]

Starting to wonder if we could do something like:

function getDirectedRangeObject() {
	return {
		anchorNode: startContainer,
		anchorOffset: startOffset,
		focusNode: endContainer,
		focusOffset: endOffset,
	} = window.getSelection();

	return {
		startContainer,
		startOffset,
		endContainer,
		endOffset,
	};
}

[ellatrix]

Or even replace start and end with anchor and focus.

[ellatrix]

(And just look at the selection object, not care about ranges inside.)

[ellatrix]

See https://github.com/WordPress/gutenberg/compare/update/writing-flow-selection-direction-iseulde-2 for the record.

[ellatrix]

Would this also need to invert? I haven't checked.

[aduth]

Would this also need to invert? I haven't checked.

I don't think it should need to, since this is for traversing through parents at the extent of the node, where selection direction doesn't matter.

[ellatrix]

yeah, came to the same conclusion in my branch :)

[ellatrix]

Do we need the same logic as above here?

[aduth]

Which logic are you referring to? Since isVerticalEdge is working primarily off the rect, I don't think we'll need similar logic with focusNode and such.

[aduth]

Using focus offset appears to work well. Cherry-picked those commits into this branch.

[aduth]

I became a bit overzealous with end-to-end tests here and came up with an (admittedly not the most elegant but workable) solution for #5095 in 291c19c.

[ellatrix]

Just curious: what prompted this change?

[aduth]

When pressing up from an uncollapsed selection, we want the caret to move directly above where the start of the selection occurs. Without this change, it would place it above the half-way point of the selection.

[aduth]

I've spent the better part of the last day and a half hacking away at these behaviors, finding myself stumbling into several rabbit holes along the way. I'd been tempted to merge in its previous approved state, as it was largely functional, though I've noticed a few intermittent test failures which have prompted me to commit my local changes.

Some of them are only tangentially related to the goal, and given the number of them, it may be wise to fragment a few into separate pull requests against which to rebase. Or, we could bite the bullet and merge it as one.

Specifically, I noted several issues with our handling of inline boundaries, of TinyMCE's bogus nodes, and of lingering zero-width space characters.

A few of which can be reproduced on current master:

Unable to horizontally traverse in a paragraph containing only inline boundary node:

1. Add paragraph
2. Add a new paragraph before the one created in Step 1
3. Cmd+B to bold
4. Write something
5. Press ArrowRight repeatedly
6. Get stuck at edge

Issue: TinyMCE leaves a bogus br node†, and our current logic to check horizontal edge doesn't account for this. In fact, our current test for tinymce.DOM.isEmpty is symptomatic of this larger problem, where in a contenteditable it's not possible for the caret to be traversed to certain nodes (empty nodes, br).

Resolution: We should check if the intended caret navigation will land in a reachable node. After a few iterations here, I finally landed on one similar to the original proposal, except not considering nodes and instead the effective caret position within text.

† It's my understanding this could be a bug within TinyMCE itself. In TinyMCE Fiddle, usually the bogus br only exists so long as the field is empty.

Demo: https://codepen.io/aduth/pen/qYxPEM

Held horizontal navigation from within inline boundary forces selection restore:

1. Add paragraph
2. Add a new paragraph before the one created in Step 1
3. Cmd+B to bold
4. Write something
5. Press ArrowRight, ArrowLeft, ArrowRight (the fact you need to do Left,Right is another bug, one in TinyMCE I think)
6. Type something (should be unbolded)
7. Save and reload
8. Place caret at start of first paragraph, hold ArrowRight
9. Notice the selection starts looping from the beginning of the selection

Issue: I haven't nailed this one down precisely, but I believe it has something to do with TinyMCE's selection restoration.

Zero-width space character from inline boundaries saved to post content:

1. Add paragraph
2. Cmd+B to bold
3. Write something
4. Press Enter to create a new paragraph
5. Press ArrowLeft
6. Save the post
7. The post is saved with a zero-width space character before the closing </strong> tag

Issue: We don't filter empty nodes in the getContent implementation, so these are saved. The filterEmptyNodes function is an option here, but it occurs shallowly. Since TinyMCE performs this already in its own (HTML) serialization, it's unfortunately we'd need to recreate this. In newer versions we can use { format: 'tree' } to receive a tree node which is cleaned of empty nodes. This works okay, with a few caveats:

• We still need to filter empty nodes in our splitContent function, and there's not an obvious way to leverage TinyMCE's internal cleanup logic for this, so we may still have need for the reimplemented node filtering.
• Since we're not converting from DOM to React and using dom-react's attribute mapping, attributes aren't guaranteed to be in the canonical form for React. Fortunately our new serializer (Framework: Use custom serializer for texturize compatibility #5897) doesn't care about this and will add them correctly. But as long as it's rendered in the editor, warnings could occur (keys and incorrect props properties).

TinyMCE's inline boundary zero-width spaces linger after blurring the field, causing inconsistent edge traversal behavior:

1. Add paragraph
2. Add a new paragraph
3. Cmd+B to bold
4. Write something
5. Press up to return to first paragraph
6. Press ArrowRight until you're in second paragraph
7. Note that the caret is within the inline boundary
8. Press ArrowLeft to return to first paragraph
9. Press ArrowRight
10. Note that the caret is before the inline boundary

Issue: TinyMCE's inline boundary zero-width character delineater remains when blurring the field, and the variation of its presence has an impact on where our placeCaretAtHorizontalEdge results in user behavior.

Resolution: Not implemented here, but there is some "clean-up" that TinyMCE performs on NodeChange. Naively adding this to the RichText's focusout had other regressing behavior. Will need to investigate. Ideally this would occur automatically on focusout by default in TinyMCE.

Inline boundary zero-width space occurs at beginning of node for new inline node:

1. Add paragraph
2. Cmd+B to bold
3. Write something
4. In Developer Tools Console, enter window.getSelection().focusNode
5. Note something like "Bold" (ZWSP at beginning)
6. Blur field
7. Click at end of the paragraph
8. In Developer Tools Console, enter window.getSelection().focusNode
9. Note something like "Bold" (ZWSP at end, where caret is)

Issue: This is mainly problematic in detecting where the caret will land on a horizontal arrow press. It would be most convenient to check in a forward movement that the next character is a ZWSP, but in the above edge case this is not correct.

Resolution: A workaround has been implemented to check for ZWSP at the beginning for forward arrow movement.

[aduth]

cc @spocke as well re: confirming potential bugs noted above.

I've unfortunately needed to introduce a few "wait for" promises in the end-to-end tests, as it appears there can be some race conditions with TinyMCE readiness, or at least this appears to be the case.

[ellatrix]

Issue: TinyMCE leaves a bogus br node†, and our current logic to check horizontal edge doesn't account for this. In fact, our current test for tinymce.DOM.isEmpty is symptomatic of this larger problem, where in a contenteditable it's not possible for the caret to be traversed to certain nodes (empty nodes, br).

The same issue would appear if there is a trailing BR node without the bogus attribute, so wouldn't it be better to ignore trailing BR nodes in general?

I'd prefer if we merge the previously approved state because I'm a bit confused with all the changes. Would be great if there's (a) PR(s) exactly fixing these cases where we can discuss the best solution and make sure there is no further performance degradation.

[aduth]

The same issue would appear if there is a trailing BR node without the bogus attribute, so wouldn't it be better to ignore trailing BR nodes in general?

Yes, the current implementation does this.

I've started fragmenting this pull request out into a few separate ones: #6710, #6711, #6712

[aduth]

I've been looking at this again between Friday and today, and in addition to the aforementioned pull requests, there's a few separate tasks which could be tackled independently:

• Ensuring ZWSP are removed from text nodes
• Removing collapse consideration from isHorizontalEdge
• Caret vertical movement on uncollapsed selection

The ZWSP handling in particular has kept me thinking. While it would be nice to take advantage of TinyMCE's tree form, since we can't use it consistently (for filtering split content), I think we'll still want to have support for removing empty nodes from a DOM tree. I think there's an opportunity to consolidate many like behaviors:

• Cleaning ZWSP after focus leaves node
  • Use instead of removeZwsp as implemented in this pull request
• Removing bogus nodes from element value
  • Use instead of filterEmptyNodes
  • Use instead of createTinyMCEElement
  • Use instead of this.editor.dom.isEmpty check in getContent (return static reference after filtering, if filtered value is empty)
  • Use instead of domToString aggregating of fragment

Work-in-progress looks something like:

export function domToElement( node ) {
	return nodeListToReact( node.childNodes );
}

export function domToString( node, editor ) {
	return editor ? editor.serializer.serialize( node ) : node.innerHTML;
}

export function removeEmptyChildNodes( node ) {
	for ( let i = 0; i < node.childNodes.length; i++ ) {
		const child = node.childNodes[ i ];

		switch ( child.nodeType ) {
			case Node.TEXT_NODE:
				if ( REGEXP_TINYMCE_ZWSP.test( child.nodeValue ) ) {
					child.nodeValue = child.nodeValue.replace( REGEXP_TINYMCE_ZWSP, '' );

					if ( ! child.nodeValue ) {
						node.removeChild( child );
					}
				}
				break;

			case Node.ELEMENT_NODE:
				if ( child.hasAttribute( 'data-mce-bogus' ) ) {
					if ( child.getAttribute( 'data-mce-bogus' ) !== 'all' ) {
						while ( child.firstChild ) {
							node.insertBefore( child.firstChild, child );
						}
					}

					node.removeChild( child );
				}
				break;
		}

		if ( child.nodeType === Node.TEXT_NODE && REGEXP_TINYMCE_ZWSP.test( child.nodeValue ) ) {
			child.nodeValue = child.nodeValue.replace( REGEXP_TINYMCE_ZWSP, '' );

			if ( ! child.nodeValue ) {
				node.removeChild( child );
			}
		}

		removeEmptyChildNodes( child );
	}
}

export function domToFormat( nodes, format, editor ) {
	const fragment = document.createDocumentFragment();
	for ( let i = 0; i < nodes.length; i++ ) {
		fragment.appendChild( nodes[ i ].cloneNode( true ) );
	}

	removeEmptyChildNodes( fragment );

	switch ( format ) {
		case 'string':
			return domToString( fragment, editor );
		default:
			return domToElement( fragment );
	}
}

Needs to be particularly conscious of performance impact of creating / cloning nodes. Not sure if copying node attributes to an object form might be faster than cloneNode.

There's more "clean-up" which needs to occur to strip TinyMCE attributes and placeholders in the filtering function. Ideally we could leverage some of TinyMCE's internals to avoid recreating this, or at least it should be based on a like implementation.

[aduth]

This has stagnated and is beyond saving at this point. Many of the ideas have inspired other pull requests, and yet more ideas included here are still valid and could be pulled out into their own separate pull requests, which I'll explore.