Framework: Extract arrow navigation to its own component #2424

youknowriad · 2017-08-15T15:04:33Z

related #2421 and transversal to #2296

This doesn't change the way arrow navigation works (like #2296 does) but just extracts it to its own component.

Pros

The VisualEditorBlock component handles way more than it should
More composable approach, the WritingFlow component can wrap anything to allow arrow navigation between tabbables.
This fixes a bug where we were stuck in the PostTitle while still allowing the PostTitle to be used in the text mode without duplicating the navigation behaviour

Notes

Like suggested in How can we improve the writing flow and its stability? #2421 I think we should think more about how to "compose" all these behaviours. I think this component could also handle Enter and Backspace navigation the same way we do for arrow navigation. It could be done separately and probably after merging Improved arrow navigation #2296
Not done in this PR yet, I think the isTyping reducer/actions should be handled by this component (WriningFlow) because the PostTitle should be able to update this property too.

codecov · 2017-08-15T15:10:32Z

Codecov Report

Merging #2424 into master will increase coverage by 2.68%.
The diff coverage is 19.54%.

@@            Coverage Diff             @@
##           master    #2424      +/-   ##
==========================================
+ Coverage   31.57%   34.26%   +2.68%     
==========================================
  Files         175      177       +2     
  Lines        5304     6173     +869     
  Branches      914     1176     +262     
==========================================
+ Hits         1675     2115     +440     
- Misses       3081     3390     +309     
- Partials      548      668     +120

Impacted Files	Coverage Δ
editor/writing-flow/index.js	`0% <0%> (ø)`
blocks/url-input/index.js	`1.63% <0%> (-0.06%)`	⬇️
editor/modes/visual-editor/index.js	`0% <0%> (ø)`	⬆️
editor/modes/visual-editor/block.js	`0% <0%> (ø)`	⬆️
editor/utils/dom.js	`40.47% <40.47%> (ø)`
editor/sidebar/post-author/index.js	`39.53% <0%> (-45.47%)`	⬇️
editor/header/tools/publish-button.js	`53.48% <0%> (-34.98%)`	⬇️
editor/sidebar/post-sticky/index.js	`26.08% <0%> (-28.46%)`	⬇️
blocks/library/image/index.js	`54.16% <0%> (-8.34%)`	⬇️
editor/sidebar/page-attributes/index.js	`69.44% <0%> (-5.56%)`	⬇️
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09380a4...14b7a7f. Read the comment docs.

aduth · 2017-08-16T17:11:48Z

I think this is a good direction and maybe we ought to take it a step further to have WritingFlow act as a coordinator using callbacks from Editable instances directly. It should probably be the responsibility of the Editable component to emit the specific behaviors (i.e. onFocusPrevious, onDeleteToPrevious) rather than inferring these from keyboard events. I'm seeing WritingFlow being the one to render EditableProvider and using context-based function callbacks to respond to these events, transitioning focus between Editables. I played with some ideas earlier today where WritingFlow could track all Editables within it, but it proved to be very difficult to do in a React-y way. Maybe we'll still at least need DOM APIs to find the previous and next focus targets.

I think the general advantage here is in isolating text interactivity, since it's quite complex, and certainly not related to rendering the block's editable form.

I'll plan to do a more thorough review shortly.

youknowriad · 2017-08-16T17:16:50Z

It should probably be the responsibility of the Editable component to emit the specific behaviors (i.e. onFocusPrevious, onDeleteToPrevious) rather than inferring these from keyboard events.

@iseulde Had a similar idea where we provide a HoC to wrap any "input" (editables, textareas, input) to emit these calls.
What concerns me here is that this forces people to use Editable or a special input component if they just want text without formatting or anything on their block.
I see the Editable component as one way among others to make the block content "editable" and not the only way.
The DOM approach has the advantage of the genericity.

aduth · 2017-08-16T17:32:11Z

Another benefit to isolating these behaviors is that it makes them more testable... ahem... AHEM 😉

youknowriad · 2017-08-16T17:44:56Z

@aduth Understood :). Was kind of waiting the direction taken by #2296 first.

…tion instead

youknowriad · 2017-08-24T11:28:27Z

@iseulde @aduth @karmatosed @jasmussen @mtias

I worked on this today and tried to resolve the navigation issues we were having #2104 and #2124
I've used a full DOM approach (no redux state), this makes it "generic".
It seems to work fine for me. It still lacks unit tests, but wanted to have your input.

ellatrix · 2017-08-24T19:56:22Z

@youknowriad At first sight, that looks good and makes sense. I'll have a deeper look tonight or tomorrow.

ellatrix · 2017-08-25T12:06:29Z

Merged isEdge from

gutenberg/editor/modes/visual-editor/block.js

Line 62 in 56bbbcf

function isEdge( { editor, reverse } ) {

😊
Feel free to reset of course.

ellatrix · 2017-08-25T12:09:40Z

Another benefit to isolating these behaviors is that it makes them more testable... ahem... AHEM 😉

Some parts may be testable, but not sure how we should test the selection stuff? Manual browser tests? Something à la PhantomJS?

@youknowriad Tested a bit, but when the Editable has been focussed already, the caret moves to the last position.

youknowriad · 2017-08-25T12:10:23Z

@iseulde Thanks for the change, this is better 👍

youknowriad · 2017-08-28T10:15:37Z

@iseulde Improved the caret moves, it should behave better now.

The last problem I see is with the Table block, but I'm not sure how to fix it. I wonder if we could move forward anyway.

ellatrix · 2017-08-28T20:36:45Z

editor/utils/dom.js

+ * @param  {Element} container DOM Element
+ * @param  {Boolean} start     Position: Start or end of the element
+ */
+export function placeCaretAtEdge( container, start = false ) {


IMHO, it would be better to use a hash for the options so it's more readable. (Same for the other functions.)

ellatrix · 2017-08-28T20:42:10Z

@youknowriad I think I'm still having the same problem... To reproduce:

Put the cursor with the pointer in the middle of an editable.
Put the cursor in the next one, again with the pointer.
Now use the arrow keys to move the caret up.
I ends up being in the start position, not at the very end of the block.

youknowriad · 2017-08-29T09:02:22Z

@iseulde Thanks for the steps, this is really hard to get right.

I noticed that when moving up, the keyUp event was causing the caret to be moved to the beginning (or middle of a block) even if the keyDown put the cursor at the right position. So I moved the placeCaretAtEdge to the keyUp event.

mtias · 2017-08-29T10:45:02Z

This is looking good to me.

mtias · 2017-08-29T10:54:27Z

blocks/url-input/index.js

@@ -104,6 +104,10 @@ class UrlInput extends Component {

 	onKeyDown( event ) {
 		const { selectedSuggestion, posts } = this.state;
+		if ( ! this.state.showSuggestions || ! this.state.posts.length ) {


Can we have a comment here to explain what this is for? I don't know what showSuggestions has to do with the key event.

youknowriad · 2017-08-29T11:22:25Z

unfortunately, jsdom doesn't support the selection API, I was only able to test the "input/textarea" use-case.

As suggested by @iseulde This could be tested in potential integration tests.

aduth · 2017-08-29T12:56:54Z

editor/utils/dom.js

+		container.focus();
+		setTimeout( () => {
+			if ( start ) {
+				container.selectionStart = 0;


Do we know that container still exists at this point? To the more pertinent point, do we need the setTimeout ? Can we comment as such to why it is necessary?

Yes, it was necessary at the time of writing (not sure why?), but I made some tweaks later, I'll check again.

Cool, the timeout is not necessary anymore :)

aduth · 2017-08-29T12:57:12Z

editor/utils/dom.js

+		return;
+	}
+
+	function placeCaretInContentEditable( element, atStart ) {


Why do we create a function for this when we could inline it in place? Comments can serve as stand-in to self-documenting functions.

yep probably not necessary

aduth · 2017-08-29T12:59:17Z

editor/writing-flow/index.js

+	}
+
+	getVisibleTabbables() {
+		const tabblablesSelector = [


As in #2323 and #2392, we're increasingly drawn to needing general helpers with this sort of logic, and am tempted to bring in ally.js despite it not being terribly compact. Don't need to make changes here, but curious if you have thoughts.

Using a library or extracting to a general helper seems like a good thing to me. But I'm not sure it will work as expected, if I understand correctly ally.js returns different tabbables depending on the browser. We'll have to test I guess

Should this be tabbablesSelector?

… no timeout

aduth

This works well in Chrome, Safari, Firefox on macOS. The behavior in Edge is not so great. Pressing "Down" when in the middle of the last line of a paragraph does nothing. Sometimes when pressing from end of a paragraph to the next, it moves to the middle of the current paragraph instead (anecdotally, after an italicized text?)

The dance of onKeyDown and onKeyUp looks a bit awkward, and more importantly feels not quite right in usage (especially when not quickly pressing arrow buttons) but I sense there was some difficulty in getting this quite right, so maybe some iterative improvements can be made.

aduth · 2017-08-29T16:32:12Z

blocks/url-input/index.js

@@ -104,6 +104,12 @@ class UrlInput extends Component {

 	onKeyDown( event ) {
 		const { selectedSuggestion, posts } = this.state;
+		// If the suggestions are not shown, we shouldn't handle the arrow keys
+		// We shouldn't preventDefault to allow block arrow keys navigation


Doesn't return false; have the same effect as preventDefault?

I guess not since it fixed the issue (maybe it was the stopPropagation) but any way safer to just return

aduth · 2017-08-29T16:37:46Z

editor/writing-flow/index.js

+			'input',
+		].join( ', ' );
+		const isVisible = ( elem ) => elem.offsetWidth > 0 || elem.offsetHeight > 0 || elem.getClientRects().length > 0;
+		return Array.from( this.container.querySelectorAll( tabbablesSelector ) ).filter( isVisible );


Minor (stylistic): Can be expressed with array spread too:

[ ...this.container.querySelectorAll( tabbablesSelector ) ]

youknowriad · 2017-08-29T16:47:48Z

The dance of onKeyDown and onKeyUp looks a bit awkward, and more importantly feels not quite right in usage (especially when not quickly pressing arrow buttons) but I sense there was some difficulty in getting this quite right, so maybe some iterative improvements can be made.

Yes, it was initially all handled in onKeyDown but #2424 (comment)

youknowriad · 2017-08-29T16:51:52Z

The behavior in Edge is not so great. Pressing "Down" when in the middle of the last line of a paragraph does nothing. Sometimes when pressing from end of a paragraph to the next, it moves to the middle of the current paragraph instead (anecdotally, after an italicized text?)

I suspect the selection API behaves slightly differently in Edge. Unfortunately, I can't fix this right now (no VM and no bandwidth to get it).

youknowriad · 2017-08-29T17:01:13Z

Ok merging. We'll address the remaining issues in other PRs

aduth · 2017-10-02T13:19:04Z

editor/writing-flow/index.js

+
+		if ( ( moveUp || moveDown ) && isEdge( target, moveUp ) ) {
+			event.preventDefault();
+			this.setState( { shouldMove: true } );


Do we need to be using state for this, or could we assign an instance property? Thinking the latter would help to avoid two unnecessary rerenders.

this.shouldMove = true;

You're probably right 👍

aduth · 2020-01-13T17:47:09Z

blocks/url-input/index.js

+		if ( ! this.state.showSuggestions || ! this.state.posts.length ) {
+			return;
+		}


I'm led here via #19462 (comment) , and trying to understand what it was that we were trying to accomplish with this condition. I'm guessing at the time it was when we were first introducing WritingFlow and using arrow keys to navigate between blocks. But in considering a link editor input: Why should we care whether there are suggestions present in deciding whether to prevent the default arrow key behavior? Based on what I describe in #19462 (comment), I sorta expect that arrow keys should always use the native behavior in the input, regardless of suggestions present. I don't really expect to be able to navigate between blocks in this context.

Trying to recall: Did we used to have the link fields inline within the blocks list, and this was how we tried to allow the arrow keys to let the user go to the next/previous focusable field? Most all of those are in popovers now, correct? Maybe we don't want to assume that, but I also don't think we need to optimize for WritingFlow here either.

In some ways it relates to #19488 (#18755), where I talk about what it is about these inputs which make us want them to be ignored by WritingFlow, ObserveTyping, etc. More and more, I think it's that being in a popover (modal) gives it its own context, where it should be treated as detached from the rest of the editor (and thus not inherit any of those typing behaviors).

I sorta expect that arrow keys should always use the native behavior in the input, regardless of suggestions present.

If URLInput is not used in the popover (Button block at that time), it shouldn't be "an arrow navigation trap"

If URLInput is not used in the popover (Button block at that time), it shouldn't be "an arrow navigation trap"

Yeah, thinking about it more, it feels like "popover" is more the exception (like previously mentioned with related #19488, #18755). It has me wondering whether it would be reasonable to treat popovers as entirely separate context, and to (by default) stop all event bubbling propagation from escaping. Maybe that's an extreme approach, but I kinda see it making sense as a totally independent document, almost like an iframe.

youknowriad self-assigned this Aug 15, 2017

youknowriad requested review from aduth and ellatrix August 15, 2017 15:04

youknowriad mentioned this pull request Aug 15, 2017

How can we improve the writing flow and its stability? #2421

Closed

aduth mentioned this pull request Aug 15, 2017

Try/2031 manual focus #2392

Closed

ellatrix mentioned this pull request Aug 16, 2017

Improve arrow nav (horizontal only) #2431

Closed

youknowriad added 3 commits August 24, 2017 12:25

Framework: Extract arrow navigation to its own component

d2aedef

Editor: Fix arrow navigation in Safari and Firefox

630f0e8

Editor: Avoid relying on whether the range changes and check the posi…

291a62a

…tion instead

youknowriad force-pushed the extract/arrow-navigation branch from bdad318 to 291a62a Compare August 24, 2017 11:26

Merge similar logic and make less dense

401c4f1

Wirting Flow: Improve caret positionning

30e13c1

ellatrix reviewed Aug 28, 2017

View reviewed changes

Writing Flow: Move on KeyUp instead

e1475bf

Writing Flow: Exclude tabbables with tabindex=-1

adf550d

mtias reviewed Aug 29, 2017

View reviewed changes

Test: Unit testing dom utils

8b2e9ca

youknowriad force-pushed the extract/arrow-navigation branch from 7e5b96e to 8b2e9ca Compare August 29, 2017 11:17

Writing flow: comment explaining urlinput changes

d89a857

aduth reviewed Aug 29, 2017

View reviewed changes

youknowriad added 2 commits August 29, 2017 14:13

Writing flow: simplify caret placing utility, no wrapped function and…

3216e78

… no timeout

Writing Flow: Fix Typo

f3448c4

aduth reviewed Aug 29, 2017

View reviewed changes

Writing Flow: Small enhancements (avoid return false and style change)

14b7a7f

youknowriad merged commit 2d8584f into master Aug 29, 2017

youknowriad deleted the extract/arrow-navigation branch August 29, 2017 17:01

youknowriad mentioned this pull request Aug 29, 2017

Arrow navigation broken in Firefox #2124

Closed

aduth mentioned this pull request Sep 8, 2017

Visual Mode: Extract content inserter as separate component #2706

Merged

aduth reviewed Oct 2, 2017

View reviewed changes

youknowriad mentioned this pull request Oct 6, 2017

Writing Flow: Avoid using state for tracking arrow key navigation #2894

Merged

This was referenced Oct 10, 2017

Title: Unselect title by blur event #2948

Merged

Allow going down to the editor from the post title using keyboard #2052

Closed

aduth reviewed Jan 13, 2020

View reviewed changes

Framework: Extract arrow navigation to its own component #2424

Framework: Extract arrow navigation to its own component #2424

Conversation

youknowriad commented Aug 15, 2017

codecov bot commented Aug 15, 2017 • edited Loading

Codecov Report

aduth commented Aug 16, 2017

youknowriad commented Aug 16, 2017

aduth commented Aug 16, 2017

youknowriad commented Aug 16, 2017

youknowriad commented Aug 24, 2017

ellatrix commented Aug 24, 2017

ellatrix commented Aug 25, 2017

ellatrix commented Aug 25, 2017

youknowriad commented Aug 25, 2017

youknowriad commented Aug 28, 2017

Choose a reason for hiding this comment

ellatrix commented Aug 28, 2017

youknowriad commented Aug 29, 2017 • edited Loading

mtias commented Aug 29, 2017

Choose a reason for hiding this comment

youknowriad commented Aug 29, 2017

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

aduth left a 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

youknowriad commented Aug 29, 2017

youknowriad commented Aug 29, 2017

youknowriad commented Aug 29, 2017

aduth Oct 2, 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

codecov bot commented Aug 15, 2017 •

edited

Loading

youknowriad commented Aug 29, 2017 •

edited

Loading

aduth Oct 2, 2017 •

edited

Loading