Input Interaction: restore collapsing selection before edge calc #14906

ellatrix · 2019-04-10T13:54:51Z

Description

Fixes #14871.
Regression caused by #14871.

Specifically, the following code should not have been removed:

Lines 89 to 95 in 0563f6f

    
           // Create copy of range for setting selection to find effective offset. 
        
           const range = selection.getRangeAt( 0 ).cloneRange(); 
        
           // Collapse in direction of selection. 
        
           if ( ! selection.isCollapsed ) { 
        
           	range.collapse( ! isSelectionForward( selection ) ); 
        
           }

How has this been tested?

Screenshots

Types of changes

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.

ellatrix · 2019-04-10T14:08:17Z

Hm, I ran the e2e test against master, but it does not fail. Looking again.

ellatrix · 2019-04-10T14:16:34Z

Fixed. Looks like it needed minimum three characters per line.

ellatrix · 2019-04-10T14:29:31Z

I'm not sure why this code change makes 6fa60b2 needed, but this seems the correct behaviour to me now.

ellatrix · 2019-04-10T14:34:04Z

The fixed test was introduced in caaf204, which also introduced the code that was removed. But again, the fixed test in this PR seems like the correct behaviour to me. Twice arrow up from a certain horizontal position should land the caret in the same horizontal position if possible.

ellatrix · 2019-04-10T14:39:39Z

I think the reason is that the range is now cloned, before handing it to getRectangleFromRange.

getRectangleFromRange may unexpectedly modify the live range:

gutenberg/packages/dom/src/dom.js

Lines 205 to 215 in 0bc9067

    
           // If the collapsed range starts (and therefore ends) at an element node, 
        
           // `getClientRects` can be empty in some browsers. This can be resolved 
        
           // by adding a temporary text node with zero-width space to the range. 
        
           // 
        
           // See: https://stackoverflow.com/a/6847328/995445 
        
           if ( ! rect ) { 
        
           	const padNode = document.createTextNode( '\u200b' ); 
        
           	range.insertNode( padNode ); 
        
           	rect = range.getClientRects()[ 0 ]; 
        
           	padNode.parentNode.removeChild( padNode ); 
        
           }

aduth · 2019-04-10T20:27:09Z

Twice arrow up from a certain horizontal position should land the caret in the same horizontal position if possible.

Yes, I compared the results of the test steps on master and this branch before seeing your comment, and it very much seems like the correct / improved behavior (you can see in the comment box here on GitHub that a native textarea behaves the same).

aduth

Makes sense, tests well.

Not so sure about where cloneRange in getRectangleFromRange is necessary, but I don't think it can hurt anyways. I'll leave it to your discretion on my feedback.

packages/dom/src/dom.js

aduth · 2019-04-10T20:33:04Z

packages/dom/src/dom.js

@@ -209,6 +218,8 @@ export function getRectangleFromRange( range ) {
 	// See: https://stackoverflow.com/a/6847328/995445
 	if ( ! rect ) {
 		const padNode = document.createTextNode( '\u200b' );
+		// Do not modify to live range.
+		range = range.cloneRange();


It reminds me of some unrelated testing I'd been doing with Chrome line-ending bugginess, where Range#cloneRange wasn't actually avoiding mutations to the original:

https://codepen.io/aduth/pen/WPjQoW

(See comment: "// WIP: Buggy, collapses spaces (mutates original).")

I never did figure out what I was seeing with my demos. I raise only in case there's still a worry about mutating the cloned value.

I've not tested the Codepen, but when you make this change in master, this test will also fail:

gutenberg/packages/e2e-tests/specs/adding-blocks.test.js

Lines 64 to 71 in 917338c

// Verify vertical traversal at offset. This has been buggy in the past

// where verticality on a blank newline would skip into previous block.

await page.keyboard.type( 'Foo' );

await page.keyboard.press( 'ArrowUp' );

await page.keyboard.press( 'ArrowUp' );

await pressKeyTimes( 'ArrowRight', 3 );

await pressKeyTimes( 'Delete', 6 );

await page.keyboard.type( ' text' );

It is true that cloning the range in isEdge will also prevent this issue, but I think it is safe to clone here is well in case it is called with a live range.

In my testing insertNode doesn't modify the original range.

aduth · 2019-04-10T20:34:31Z

packages/dom/src/dom.js

@@ -209,6 +218,8 @@ export function getRectangleFromRange( range ) {
 	// See: https://stackoverflow.com/a/6847328/995445
 	if ( ! rect ) {
 		const padNode = document.createTextNode( '\u200b' );
+		// Do not modify to live range.


I'd kinda wonder why it matters, i.e. while true that we're mutating the range, we're also following up with a mutation to remove the added node, which means it should be effectively the same. I'm not entirely clear from the conversation of the pull request what impact this change is having.

We do update the DOM again, but not through the range API which may explain why its not restored.

…dPress#14906) * Input Interaction: restore collapsing selection before edge calc * Add e2e test * Fix other e2e test * Clean up * Do not modify live range in getRectangleFromRange

) * Input Interaction: restore collapsing selection before edge calc * Add e2e test * Fix other e2e test * Clean up * Do not modify live range in getRectangleFromRange

ellatrix requested review from aduth, gziolo, oandregal and youknowriad as code owners April 10, 2019 13:54

ellatrix added [Type] Bug An existing feature does not function as intended [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Type] Regression Related to a regression in the latest release labels Apr 10, 2019

ellatrix added this to the 5.5 (Gutenberg) milestone Apr 10, 2019

ellatrix requested review from ajitbohra, nerrad, ntwb and talldan as code owners April 10, 2019 14:03

ellatrix force-pushed the fix/input-interaction-uncollapsed-arrow-selection branch from 4d7e88b to 5d668f3 Compare April 10, 2019 14:15

aduth approved these changes Apr 10, 2019

View reviewed changes

ellatrix added 4 commits April 11, 2019 17:16

Input Interaction: restore collapsing selection before edge calc

6737596

Add e2e test

44dbe67

Fix other e2e test

5267e8b

Clean up

7b7ac4d

ellatrix force-pushed the fix/input-interaction-uncollapsed-arrow-selection branch from 64d33d7 to ba32d64 Compare April 11, 2019 15:17

Do not modify live range in getRectangleFromRange

94c4b6b

ellatrix force-pushed the fix/input-interaction-uncollapsed-arrow-selection branch from ba32d64 to 94c4b6b Compare April 11, 2019 15:18

ellatrix merged commit 8af16de into master Apr 11, 2019

ellatrix deleted the fix/input-interaction-uncollapsed-arrow-selection branch April 11, 2019 16:35

This was referenced Apr 15, 2019

Bump version number to 5.5.0-rc.1 #14981

Merged

Bump/5.5 #15011

Closed

Release/5.5 #15012

Closed

Bump plugin version to 5.5 #15013

Merged

aduth mentioned this pull request Apr 17, 2019

Plugin: Backport bug fixes from Gutenberg 5.5.0 to wp/trunk (WordPress 5.2 RC) (Part 2) #15020

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Input Interaction: restore collapsing selection before edge calc #14906

Input Interaction: restore collapsing selection before edge calc #14906

ellatrix commented Apr 10, 2019 •

edited

Loading

ellatrix commented Apr 10, 2019

ellatrix commented Apr 10, 2019

ellatrix commented Apr 10, 2019

ellatrix commented Apr 10, 2019

ellatrix commented Apr 10, 2019

aduth commented Apr 10, 2019

aduth left a comment

aduth Apr 10, 2019

ellatrix Apr 11, 2019

aduth Apr 10, 2019

ellatrix Apr 11, 2019

	// Create copy of range for setting selection to find effective offset.
	const range = selection.getRangeAt( 0 ).cloneRange();

	// Collapse in direction of selection.
	if ( ! selection.isCollapsed ) {
	range.collapse( ! isSelectionForward( selection ) );
	}

	// Verify vertical traversal at offset. This has been buggy in the past
	// where verticality on a blank newline would skip into previous block.
	await page.keyboard.type( 'Foo' );
	await page.keyboard.press( 'ArrowUp' );
	await page.keyboard.press( 'ArrowUp' );
	await pressKeyTimes( 'ArrowRight', 3 );
	await pressKeyTimes( 'Delete', 6 );
	await page.keyboard.type( ' text' );

Input Interaction: restore collapsing selection before edge calc #14906

Input Interaction: restore collapsing selection before edge calc #14906

Conversation

ellatrix commented Apr 10, 2019 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

ellatrix commented Apr 10, 2019

ellatrix commented Apr 10, 2019

ellatrix commented Apr 10, 2019

ellatrix commented Apr 10, 2019

ellatrix commented Apr 10, 2019

aduth commented Apr 10, 2019

aduth left a comment

Choose a reason for hiding this comment

aduth Apr 10, 2019

Choose a reason for hiding this comment

ellatrix Apr 11, 2019

Choose a reason for hiding this comment

aduth Apr 10, 2019

Choose a reason for hiding this comment

ellatrix Apr 11, 2019

Choose a reason for hiding this comment

ellatrix commented Apr 10, 2019 •

edited

Loading