Vertical arrow key navigation #2988

ellatrix · 2017-10-11T13:38:30Z

Description

This is a rebase of #2296.

It allows the caret in a block to be vertically repositioned across focusable elements.

How Has This Been Tested?

In e.g. the demo content, place the caret in an editable container. Use the UP and DOWN arrow keys to navigate across editable areas and blocks.

Checklist:

My code is tested. (No unit test because it uses layout.)
My code follows the WordPress code style.
My code follows has proper inline documentation.

ellatrix · 2017-10-11T14:32:36Z

~~There's an issue with positioning the caret when you try to move it up vertically and the toolbar is in the way... Then it will try to focus a button in the toolbar. 😄~~ Fixed.

ellatrix · 2017-10-11T19:02:22Z

~~So far it works perfectly in Chrome and Safari. Firefox is a complete mess again.~~ ~~Being addressed in #2986.~~ Done.

ellatrix · 2017-10-12T22:24:45Z

Getting quite happy with this. Squashed down a few other bugs. On rare occasions the browser fails to move position, have yet to figure out these few edge cases.

ellatrix · 2017-10-12T22:43:56Z

Fixed an issue where the new range is not within the target container.
Fixed an issue where vertical arrow keys wouldn't work in an empty container.

ellatrix · 2017-10-13T13:32:18Z

Encountered one more bug with the button block. It will attempt to put the caret next to it but fail. :) Will fix in a bit.

jasmussen · 2017-10-13T13:48:44Z

This seems to be working QUITE well. Impressive. Lovely.

As you get near the end of the demo content, the page seems to scroll in jumps, so to speak. This can make it a little easy to lose focus of where the caret is. This is very possibly just normal browser behavior, in which case ignore crazy old Joen. But otherwise — what could this be and is there a way to make it smoother, perhaps that typewriter approach?

jasmussen · 2017-10-13T13:49:37Z

Inside the code block, the range stuff doesn't work. Which is totally fine, it doesn't have to — but figured I'd mention it in case it's like a one line code change.

ellatrix · 2017-10-13T14:34:49Z

I think that might be the bug I mentioned. I know you'd like that typewriter feel in general, but let's attempt that in a separate PR. 😄
I'll look at the code block. We have to put at the start or end there, there's no way around that for inputs and text areas. :/

jasmussen · 2017-10-13T14:41:45Z

I know you'd like that typewriter feel in general, but let's attempt that in a separate PR. 😄

Totally, no worries.

I'll look at the code block. We have to put at the start or end there, there's no way around that for inputs and text areas. :/

No worries — this enhancement is way more useful for the editables. Doesn't have to work elsewhere imo.

mcsf

Here's a first review. I think we should tackle the matter of firstVerticalRect.

I will now switch context for a bit so that I can come back to this later with a clearer head and see if I can help.

mcsf · 2017-10-13T13:58:25Z

editor/writing-flow/index.js

+					return null;
+				}
+
+				return node;


I'd noticed this usage of reduce just the other day in master, and my question still stands in this PR. The above method call can be modelled as:

xs.reduce( ( result, x, i ) => result || ( pred( x, i ) ? x : null )

which to me looks like Array#find in disguise (and more expensive). Is it not?

// import { find } from 'lodash' find( xs, pred )

Feel free to correct me if I missed something. :)

mcsf · 2017-10-13T14:08:01Z

editor/writing-flow/index.js

 		const focusableNodes = this.getVisibleTabbables();
-		if ( direction === 'UP' ) {
+
+		if ( reverse ) {


Even though this.getVisibleTabbables will return a new instance on each call and we can see that detail in this very component, I tend to be careful with in-place modifications like Array#reverse. I'll leave it up to you, but I would either use lodash#reverse or even [ ...focusableNodes ].reverse().

I don't really see the problem here? Should I change it?

mcsf · 2017-10-13T16:34:22Z

editor/utils/dom.js

+	const rangeRect =
+		range.startContainer.nodeType === window.Node.ELEMENT_NODE ?
+		range.startContainer.getBoundingClientRect() :
+		range.getClientRects()[ 0 ];


Minor point, but I find that multi-line ternaries read better like:

a ? b : c

but currently our linter has no preference.

Fair enough. I've been used to this way. Maybe we should add a rule so that it's the same everywhere?

My only worry is that the Javascript interpreter of some browser out there will see the condition as the end of the statement if the question mark is not the same line. It may be changed in transpiling though so if Babble gets it right then I suppose I don't need to worry.

That's probably why I've been used to it this way.

I had a whole reply with an excerpt of the ECMA spec and an analysis making the case that only a faulty browser would be too eager to insert a semicolon in this case, but then I found the right Crockford source. :)

The ternary operator can be visually confusing, so ? question mark always begins a line and increase the indentation by 4 spaces, and : colon always begins a line, aligned with the ? question mark. The condition should be wrapped in parens.

My take is that Crockford has historically been a conservative reference on JS (and, apparently, a previous version of the same text had advised against having an operator at the beginning of a line), and thus his OK means that we're totally safe here and that it's ultimately a stylistic decision. How do y'all weigh in?

I don't have a strong opinion. If browsers are a worry, we anyway process with Babel and everything gets minified.

mcsf · 2017-10-13T16:35:30Z

editor/utils/dom.js

+	// Editable was already focussed, it goes back to old range...
+	// This fixes it.
+	selection.removeAllRanges();
+	selection.addRange( range );


Could you provide more detail on the need to repeat this? Is it a quirk?, is it something otherwise preventable?, etc.

I suspect it might be related to TinyMCE's editor.selection.lastFocusBookmark, but I need to investigate. Maybe @ephox-mogran or @EphoxJames know more.

I don't know off the top of my head but I will dig into the code and see if I can find out.

Thanks! Ideally we wouldn't have to touch the TinyMCE editor instance at all here.

mcsf · 2017-10-13T16:45:31Z

editor/utils/dom.js

+
+	const range = caretRangeFromPoint( document, x, y );
+
+	container.style.zIndex = null;


Hm, bummer. :)

Local testing (on this very page) indicates that container.style.pointerEvents = 'none'; is just as effective. Should we prefer that one instead? I wonder if that way we minimize our chances of generating visual artefacts.

How would that be used? On the toolbar element? That would require us to have the toolbar element, which would be less isolated logic. In that case I'd prefer the z-index approach.

Yes, it would be on the toolbar — I was looking at this so closely that I didn't weigh the drawback of having to deal with yet another entity. z-index may be the lesser evil, then.

mcsf · 2017-10-13T16:56:02Z

editor/utils/dom.js

+ *
+ * @type {DOMRect}
+ */
+let firstVerticalRect;


My biggest concern is this global here. It's a concern in general, but aggravated by the fact that we are in a utils module, where the expectation of lingering state is low, and that the main function that sets it is a seemingly innocent predicate, isVerticalEdge.

I see it as a definite code smell that we should tackle, probably before merging. Does it mean that the predicate should become something else? That it should be broken into different pieces? That it should become a function returning a DOMRect or null? Relatedly, onMouseDown={ resetVerticalPosition } on WritingFlow is an implementation detail that shouldn't be leaking.

Any ideas would be welcome! I'll have a think.

Can firstVerticalRect just be passed in, and have its state / value managed in WritingFlow itself? It is essentially part of the state of writing flow, if that's where we are doing the cursor movement.

Okay, I changed this.

mcsf · 2017-10-13T16:57:03Z

editor/utils/dom.js

 */
-export function placeCaretAtEdge( container, start = false ) {
+export function isVerticalEdge( container, reverse ) {
+	if ( [ 'INPUT', 'TEXTAREA' ].indexOf( container.tagName ) !== -1 ) {


Minor: naming this, as is done in other parts of the diff, would be nice: const isInputOrTextarea =

I almost changed the other one. :) What makes you prefer it?

That indexOf isn't very descriptive and the variable assignment counters that. I would also go, instead, for lodash#includes.

mcsf · 2017-10-13T17:03:01Z

editor/writing-flow/index.js

+		const right = keyCode === RIGHT;
+		const reverse = up || left;
+		const horizontal = left || right;
+		const vertical = up || down;


As some of the functions here and in dom/utils grow, keeping track of boolean variables becomes harder. Aside from a [modal]+[main verb] nomenclature (with modal taking values in { is, should, can, may, will, … }), I'd love to see more descriptive names throughout. Examples:

up → isUpKey

reverse → isAscending

noScroll → logic reversal, becoming mayUseScroll

ephox-mogran · 2017-10-16T06:43:09Z

@iseulde I've found one issue that might be making it a little inconsistent. It's most visible when using the table block. If you setup a paragraph below the table and after the table, and try to navigate from below the table up, or above the table down, then the caret doesn't stay in the same horizontal position when it is entering the table.

This is because of two reasons:

There is padding on the table cells which interferes with y positions of nodes. This directly impacts (2)
The caretRangeFromPosition method can return a range that has a bounding rectangle that does not include the (x, y) coordinate passed in. It is a 'close-enough' type of API that is designed for finding the nearest text. The problem is that when it finds a text node, but the (x, y) is not actually inside that text node, it gets the offset wrong and just chooses either the start of the node or the end of the node, which is not what you want.

In order to offset this, you can run the caretRangeFromPosition method again, but this time, use an (x, y) that is bounded within the getBoundingClientRect of the range it returned initially. In this case, just use the same x and use a y that is either the rect.top or rect.bottom (or potentially offset that number by one or two). This then means that it is looking within the text itself (and not inside the padding), and it will give you an accurate offset.

Now, you may get a problem where it will just keep recursing indefinitely, because it can't find a text position that is any better than it's first guess. Therefore, you should quit after some number of retries. It shouldn't be very many.

Note, the cursor definitely doesn't keep the same position when moving through the table itself, but that should be logged against TinyMCE.

mcsf · 2017-10-20T12:59:19Z

@iseulde: a224863 fixes an occasional bug I was seeing, which can be reproduced consistently this way:

Open the Gutenberg Demo
Place the cursor at Of mountains & →|←printing presses
Keep hitting the down arrow
An error should be thrown when switching into block "... like this one, which is right aligned."

ephox-mogran · 2017-10-23T00:58:03Z

I'm looking into seeing why

In the master branch, as well as in this branch, reverse horizontal navigation breaks if there is formatting. Press the left arrow key at the start of a paragraph, and it will end of at the end of a formatting tag in the previous editable. As far as I see, this seems to be caused by range.collapse().

is happening.

ephox-mogran · 2017-10-23T04:04:18Z

@iseulde , it looks like what you are seeing is a bug in tinymce. I've recreated it in a fiddle here (http://fiddle.tinymce.com/Scgaab/1). As soon as you remove the tinymce.init call, it works again.

mcsf · 2017-10-23T17:37:21Z

@iseulde & @ephox-mogran, drive-by comment before I have to run: breaking on selectionchange yields the following:

mcsf · 2017-10-24T11:22:23Z

editor/writing-flow/index.js

+			}
+
+			return true;
+		} );
 	}

 	onKeyDown( event ) {
 		const { keyCode, target } = event;


I missed this until now, but FYI KeyboardEvent.keyCode has been deprecated and KeyboardEvent.key is to be preferred.

Something to revise in the future?

Probably. We've been using it in Gutenberg quite a lot, so does TinyMCE. Maybe better to discuss separately.

Previously I'd avoided key because of browser support; I don't recall if this is specifically an older IE thing, or due to the non-standard behavior described in the "Notes" at:

http://caniuse.com/#feat=keyboardevent-key

ellatrix · 2017-10-24T11:52:25Z

@ephox-mogran Yeah, the fiddle illustrates this well. Only breaks in Chrome for me, not Safari.

mcsf · 2017-10-24T17:18:57Z

Re

In the master branch, as well as in this branch, reverse horizontal navigation breaks if there is formatting. Press the left arrow key at the start of a paragraph, and it will end of at the end of a formatting tag in the previous editable. As far as I see, this seems to be caused by range.collapse().

Culprit appears to be a fix for browser quirks:

https://github.com/tinymce/tinymce/blob/d2c9a9a03e3e2adabcd01a46d65640e49f5beb75/src/core/src/main/js/util/Quirks.js#L529-L544

Fixes selection issues where the caret can be placed between two inline elements like <b>a</b>|<b>b</b> this fix will lean the caret right into the closest inline element.

I didn't spot any obvious override mechanism to get around this quirk fix. I played a bit with event interception and tinymce.init({ setup }), to no avail. @ephox-mogran, short of pushing an upstream change, is there any sort of TinyMCE-fu that could help us here? :)

ephox-mogran · 2017-10-24T17:32:34Z

@mcsf the fix in normalisation of ranges for this is in code review in the TinyMCE development team. Outside of waiting for that fix for core to be merged, there's not much you can do. It's a high priority.

ephox-mogran · 2017-10-24T17:47:10Z

@mcsf , @iseulde , how many blocking issues are there still with this PR? Are there any we have replication cases for (aside from the normalisation range bug in TinyMCE above?)

Specifically, is this still happening?

Sometimes ranges are not set (no idea what the reason is), or they are set at the last focussed range. The latter is fixed with a hack for vertical navigation, so either we add this hack for horizontal too, or we find a better way to fix this.

mcsf · 2017-10-24T18:33:04Z

the fix in normalisation of ranges for this is in code review in the TinyMCE development team.

Great to know! Can you keep us posted? Seems like we can hold off this PR till EOW, perhaps?

Specifically, is this still happening?

I don’t think I ever ran into that one.

how many blocking issues are there still with this PR?

None on my part. How about you, @iseulde?

ephox-mogran · 2017-10-24T20:08:55Z

Great to know! Can you keep us posted? Seems like we can hold off this PR till EOW, perhaps?

No problem. I'll keep you posted.

Also, given that this bug is already in master, and has to do with horizontal navigation and not vertical navigation, it probably shouldn't hold up this PR which is focusing on vertical navigation. Especially, as it's an internal TinyMCE bug. Would you agree @mcsf ?

mcsf · 2017-10-24T20:10:48Z

Good point. We should also rebase and retest.

- Split `getVerticalEdge` into two separate helpers: `computeCaretRect` and `isVerticalEdge`. - Rearrange the logic in `WritingFlow#keyDown` to make the symmetries between horizontal- and vertical-navigation handlings more apparent, as well as the lifecycle of `this.verticalRect`.

ephox-mogran

Aside from my issue with using textContent for non text nodes when dealing with offsets (which is already in master, so not related to this PR), it looks good to me.

ephox-mogran · 2017-10-24T20:56:23Z

@mcsf should I rebase #3038 now based on this and update the PR? Or do you still have another approach in mind that you want to try first?

mcsf · 2017-10-24T21:16:24Z

You can go ahead and rebase the original approach, thanks!

aduth · 2017-10-25T12:48:20Z

editor/writing-flow/index.js

@@ -71,7 +103,9 @@ class WritingFlow extends Component {
 		return (
 			<div
 				ref={ this.bindContainer }
-				onKeyDown={ this.onKeyDown }>
+				onKeyDown={ this.onKeyDown }
+				onMouseDown={ () => this.verticalRect = null }


It would have been good to assign this to a bound instance method so that it's not creating a new function reference (and thereby applying a change diff) on every render.

Darn, I'm usually stringent about those.

ephox-mogran · 2017-11-08T04:38:11Z

@iseulde @mcsf

In the master branch, as well as in this branch, reverse horizontal navigation breaks if there is formatting. Press the left arrow key at the start of a paragraph, and it will end of at the end of a formatting tag in the previous editable. As far as I see, this seems to be caused by range.collapse().

This has been fixed as of TinyMCE 4.7.2.

aduth · 2018-02-26T20:02:49Z

editor/writing-flow/index.js

@@ -20,6 +26,8 @@ class WritingFlow extends Component {

 		this.onKeyDown = this.onKeyDown.bind( this );
 		this.bindContainer = this.bindContainer.bind( this );
+
+		this.verticalRect = null;


Arriving at this pull request hopefully confused about what this variable is. Can we add some inline comments?

Might be good to make an issue :)

Comment coming.

ellatrix added the [Status] In Progress Tracking issues with work in progress label Oct 11, 2017

ellatrix changed the title ~~Improved arrow key navigation~~ Vertical arrow key navigation Oct 11, 2017

ellatrix force-pushed the try/improved-arrow-nav branch from 8c5ac78 to 204884e Compare October 11, 2017 14:26

ellatrix force-pushed the try/improved-arrow-nav branch 4 times, most recently from 71002ed to 2a19bfc Compare October 11, 2017 17:30

mcsf mentioned this pull request Oct 11, 2017

Improve keyboard navigation across blocks #2990

Closed

ellatrix mentioned this pull request Oct 11, 2017

Focusable Utility: Use tabindex attribute for tabbable test #2986

Merged

ellatrix force-pushed the try/improved-arrow-nav branch 2 times, most recently from 3190dcc to cd6d238 Compare October 12, 2017 22:18

ellatrix force-pushed the try/improved-arrow-nav branch from cd6d238 to 0b41ee3 Compare October 12, 2017 22:41

ellatrix requested review from mcsf, youknowriad and ephox-mogran October 12, 2017 22:47

ellatrix force-pushed the try/improved-arrow-nav branch from 0b41ee3 to af6d188 Compare October 13, 2017 12:47

mcsf requested changes Oct 13, 2017

View reviewed changes

ephox-mogran mentioned this pull request Oct 17, 2017

Managing multi-block selection with the keyboard #3038

Merged

3 tasks

ellatrix force-pushed the try/improved-arrow-nav branch 2 times, most recently from 55ebf3c to 5f731cd Compare October 18, 2017 17:51

mcsf force-pushed the try/improved-arrow-nav branch from 673c0ac to f4d2700 Compare October 20, 2017 12:51

mcsf mentioned this pull request Oct 24, 2017

Toolbar Arrow Navigation (just adding ESC behaviour) #2996

Closed

3 tasks

mcsf reviewed Oct 24, 2017

View reviewed changes

ellatrix and others added 3 commits October 24, 2017 21:37

Add vertical arrow navigation for blocks

881810f

placeCaretAtVerticalEdge: fix TypeError when target out of view

c519d58

mcsf force-pushed the try/improved-arrow-nav branch from a224863 to c519d58 Compare October 24, 2017 20:43

ephox-mogran approved these changes Oct 24, 2017

View reviewed changes

mcsf approved these changes Oct 24, 2017

View reviewed changes

mcsf merged commit 6e9b429 into master Oct 24, 2017

mcsf deleted the try/improved-arrow-nav branch October 24, 2017 20:50

aduth reviewed Oct 25, 2017

View reviewed changes

mtias mentioned this pull request Oct 25, 2017

Try alternate style to indicate selected blocks #3068

Merged

3 tasks

mcsf mentioned this pull request Oct 25, 2017

WritingFlow: Avoid function instantiation in render #3153

Merged

aduth reviewed Feb 26, 2018

View reviewed changes

aduth mentioned this pull request May 13, 2019

Writing Flow: Caret position not always preserved in vertical traversal #15604

Closed


		const range = caretRangeFromPoint( document, x, y );

		container.style.zIndex = null;

Vertical arrow key navigation #2988

Vertical arrow key navigation #2988

Conversation

ellatrix commented Oct 11, 2017 • edited Loading

Description

How Has This Been Tested?

Checklist:

ellatrix commented Oct 11, 2017 • edited Loading

ellatrix commented Oct 11, 2017 • edited Loading

ellatrix commented Oct 12, 2017

ellatrix commented Oct 12, 2017

ellatrix commented Oct 13, 2017

jasmussen commented Oct 13, 2017

jasmussen commented Oct 13, 2017

ellatrix commented Oct 13, 2017 • edited Loading

jasmussen commented Oct 13, 2017

mcsf left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ephox-mogran commented Oct 16, 2017

mcsf commented Oct 20, 2017

ephox-mogran commented Oct 23, 2017

ephox-mogran commented Oct 23, 2017 • edited Loading

mcsf commented Oct 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Oct 24, 2017

mcsf commented Oct 24, 2017

ephox-mogran commented Oct 24, 2017 • edited Loading

ephox-mogran commented Oct 24, 2017

mcsf commented Oct 24, 2017

ephox-mogran commented Oct 24, 2017 • edited Loading

mcsf commented Oct 24, 2017

ephox-mogran left a comment

Choose a reason for hiding this comment

ephox-mogran commented Oct 24, 2017

mcsf commented Oct 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ephox-mogran commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Oct 11, 2017 •

edited

Loading

ellatrix commented Oct 11, 2017 •

edited

Loading

ellatrix commented Oct 11, 2017 •

edited

Loading

ellatrix commented Oct 13, 2017 •

edited

Loading

mcsf left a comment •

edited

Loading

ellatrix Oct 13, 2017 •

edited

Loading

ephox-mogran commented Oct 23, 2017 •

edited

Loading

ephox-mogran commented Oct 24, 2017 •

edited

Loading

ephox-mogran commented Oct 24, 2017 •

edited

Loading

mcsf commented Oct 24, 2017 •

edited

Loading