Improved arrow navigation #2296

ellatrix · 2017-08-08T21:06:17Z

This PR changes arrow navigation so that the vertical position is preserved. Still a WIP, but feel free to play with it.

To do:

~~Add editable left/right nav.~~
~~Handle scroll properly.~~
Look into editables with padding.
Preserve second last line x position if the last line is shorter.

mattyrob · 2017-08-09T19:06:49Z

I've been directed here from the WordPress.org forums. Just to point out that arrow cursor navigation is broken on Safari in Gutenberg 0.7.0 and buggy in FF and Chrome on MacOS also.

Basically arrow navigation only works inside a single block in Safari.

In FF, Up and down navigate only within a block, at the start or ned, left or right move the highly to the next block and then you are navigating by block. If you end up in the Title block though arrow navigation stops working.

In Chrome, up and down navigation works but when you are at the top of one block, hitting up takes you to the top of the block above - not the end - so different to FF. Again, if you end up in the Title block though arrow navigation stops working.

ellatrix · 2017-08-10T14:10:11Z

editor/modes/visual-editor/block.js

+						followingEditor.selection.collapse( false );
+					}
+				} else {
+					window.setTimeout( () => {


Here again I'm not sure why exactly it doesn't work without the timeout... @spocke or anyone @tinymce, any idea why?

Ideally we should get rid of this, because it also prevents the positioning if you hold the arrow key.

Also, editor.selection.placeCaretAt doesn't seem to work in Firefox, even though it should support https://developer.mozilla.org/en-US/docs/Web/API/Document/caretPositionFromPoint.

Maybe it does work with caretRangeFromPoint... FF supports both, and Chrome and Safari will fall back to that one.

Okay, so in Firefox it works without a timeout and without focussing first (but we still need to move the focus...) and in Chrome and Safari in doesn't work without focussing first and the timeout. 😖

@spocke For me that doesn't seem to work. When focus is called it will move to it to the last range when there was focus.

In Chrome and Safari, it will work moving down, but not up. In Firefox it still won't work at all.

Can't seem to reproduce that for me the fiddle works on Mac, Windows, Linux various versions of Chrome since I use the dev channel on my dev machine and stable on the windows testing machine.

Make sure you bind that even early since otherwise handlers inside tinymce might want to act on the down key as well.

I just can't get it to work in Gutenberg. To me it seems weird that we'd have to execute that twice anyway. That suggests that it somehow doesn't work correctly.

ellatrix · 2017-08-10T14:14:07Z

This should work already nicely now, albeit with a lot of code... One thing I don't like is how this introduces so much code to this component that feels like it should belong to Editable. @youknowriad do you think I should try the props again? Not sure how to better handle this as what I have now.

codecov · 2017-08-10T14:40:37Z

Codecov Report

Merging #2296 into master will decrease coverage by 0.31%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2296      +/-   ##
==========================================
- Coverage    25.9%   25.58%   -0.32%     
==========================================
  Files         157      157              
  Lines        4853     4921      +68     
  Branches      822      848      +26     
==========================================
+ Hits         1257     1259       +2     
- Misses       3035     3077      +42     
- Partials      561      585      +24

Impacted Files	Coverage Δ
editor/modes/visual-editor/block.js	`0% <0%> (ø)`	⬆️
blocks/color-palette/index.js	`0% <0%> (ø)`	⬆️
blocks/library/cover-text/index.js	`36% <0%> (+1%)`	⬆️

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 77620b2...33557b3. Read the comment docs.

ellatrix · 2017-08-10T15:20:48Z

Merging #2296 into master will increase coverage by 2.8%.

@nylen Huh, why would this increase coverage? :)

nylen · 2017-08-10T18:36:43Z

I'm not sure why, but Codecov has the wrong base commit for this PR:

Let's try this allow_coverage_offset setting from https://docs.codecov.io/v4.3.6/docs/comparing-commits#section-resolution (cbcb14d).

ellatrix · 2017-08-10T19:03:09Z

@aduth Do you think I should the EditableProvider for this similarly to your focus PR?

nylen · 2017-08-10T19:42:53Z

I pushed 2 other branches to try to track down the coverage issue:

try/improved-arrow-nav-original - this PR with a minor change to trigger a full rebuild. The Codecov build failed saying "Missing base report. Unable to compare commits because the base of the compare did not upload a coverage report." which I think is better than reporting incorrect results.
try/improved-arrow-nav-rebased - this PR rebased against master. Coverage appears to be reported correctly.

The base commit of this PR should be d7033d5 which appears to have a passing Codecov build. However, in the list of builds for master, the commit hash for that commit is wrong, and the previous commit still says "Processing reports" after a couple days:

I am not entirely sure what happened but I would suggest that we rebase this PR and separately add that codecov.yml setting (#2354).

aduth

In my testing, this feels very nice, and I think is a good step toward restoring some simple text interactions if we're going to keep with the Editable-per-paragraph direction.

One interaction that doesn't work quite as well as in most native editors is remembering the original caret position even when navigating across an empty block or a block with shorter content.

Expected:

Actual:

@aduth Do you think I should the EditableProvider for this similarly to your focus PR?

Not sure what you have in mind here specifically, but there is a fair amount of complexity here, and it is not very well isolated. Maybe using the EditableProvider could be used here to surface up and isolate the handling. Specifically to try to remedy:

The individual block querying all adjacent blocks in the DOM
Blocks being bound to TinyMCE as the only supported arrow navigation (maybe this ought to be conceded)

Generally this could use some more comments, as some of the logic is quite dense and not self-evident.

aduth · 2017-08-14T14:28:14Z

editor/modes/visual-editor/block.js

+
+		const isVerticalEdge = ( { editor, reverse } ) => {
+			const rangeRect = editor.selection.getBoundingClientRect();
+			const buffer = rangeRect.height / 2;


What is this buffer for, and how did you come to arrive at rangeRect.height / 2?

Rather than trying to place the caret at the very top or bottom (which might be off by 0-1px), it seems better to attempt to place it in the middle of the line.

(I'll comment)

aduth · 2017-08-14T14:33:58Z

editor/modes/visual-editor/block.js

+				} else {
+					window.setTimeout( () => {
+						const buffer = rect.height / 2;
+						const editorRect = followingEditor.getBody().getBoundingClientRect();


Do we know followingEditor still exists after this setTimeout ?

I know you commented previously something similar. In this case I want to get right of the timeout as it doesn't work in Firefox, and it breaks holding the arrow key.

ellatrix · 2017-08-14T15:02:14Z

One interaction that doesn't work quite as well as in most native editors is remembering the original caret position even when navigating across an empty block or a block with shorter content.

I will fix that. We can remember the first position, and discard it when you press any other key.

ellatrix · 2017-08-14T15:49:27Z

@aduth Okay, I fixed that for the down key. For the up key it's harder as we don't manage the position after the last line. We only set it when moving the caret from one editor to another. If we want to fix that we should manage up and down fully... :/

ellatrix · 2017-08-14T15:50:15Z

(Or at least the first, second last, and last line.)

ellatrix · 2017-10-10T20:10:14Z

Hm, not sure why this PR was closed... I'll rebase in a new one.

ellatrix · 2017-10-11T13:37:17Z

Opening a new PR.

ellatrix added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Status] In Progress Tracking issues with work in progress labels Aug 8, 2017

ellatrix added this to the Beta 0.8.0 milestone Aug 8, 2017

ellatrix self-assigned this Aug 8, 2017

ellatrix force-pushed the try/improved-arrow-nav branch from 5c61e54 to ee64866 Compare August 8, 2017 21:15

ellatrix commented Aug 10, 2017

View reviewed changes

ellatrix force-pushed the try/improved-arrow-nav branch from e37f542 to 8ee9e35 Compare August 10, 2017 14:19

ellatrix mentioned this pull request Aug 10, 2017

Try: Manage focus through EditableProvider #2108

Closed

nylen mentioned this pull request Aug 10, 2017

Prevent Codecov from using "approximate" PR base commits #2354

Closed

mtias modified the milestones: Beta 0.9.0, Beta 0.8.0 Aug 14, 2017

ellatrix added the [Component] TinyMCE label Aug 14, 2017

ellatrix added 3 commits August 14, 2017 15:24

Improved arrow nav

9778502

Handle multiple editables in one block

9ccab35

Handle horizontal arrow keys

7c2a9e8

aduth reviewed Aug 14, 2017

View reviewed changes

Attempt to preserve first vertical position

33557b3

ellatrix force-pushed the try/improved-arrow-nav branch from cbcb14d to 33557b3 Compare August 14, 2017 15:46

youknowriad mentioned this pull request Aug 15, 2017

Framework: Extract arrow navigation to its own component #2424

Merged

ellatrix mentioned this pull request Aug 15, 2017

Improve arrow nav (horizontal only) #2431

Closed

mtias modified the milestones: 0.10.0, Beta 0.9.0 Aug 18, 2017

karmatosed modified the milestones: 0.11.0, 0.10.0 Aug 25, 2017

karmatosed closed this Aug 30, 2017

ntwb deleted the try/improved-arrow-nav branch September 4, 2017 08:03

ellatrix restored the try/improved-arrow-nav branch October 10, 2017 20:10

ellatrix reopened this Oct 10, 2017

ellatrix mentioned this pull request Oct 11, 2017

Block navigation with keydown #2979

Merged

3 tasks

ellatrix closed this Oct 11, 2017

ellatrix deleted the try/improved-arrow-nav branch October 11, 2017 13:37

ellatrix mentioned this pull request Oct 11, 2017

Vertical arrow key navigation #2988

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved arrow navigation #2296

Improved arrow navigation #2296

ellatrix commented Aug 8, 2017 •

edited

Loading

mattyrob commented Aug 9, 2017

ellatrix Aug 10, 2017

ellatrix Aug 10, 2017

ellatrix Aug 10, 2017

ellatrix Aug 10, 2017

ellatrix Aug 10, 2017

ellatrix Aug 10, 2017

ellatrix Aug 10, 2017

spocke Aug 10, 2017

spocke Aug 10, 2017

ellatrix Aug 14, 2017

ellatrix commented Aug 10, 2017

codecov bot commented Aug 10, 2017 •

edited

Loading

ellatrix commented Aug 10, 2017

nylen commented Aug 10, 2017

ellatrix commented Aug 10, 2017

nylen commented Aug 10, 2017 •

edited

Loading

aduth left a comment

aduth Aug 14, 2017

ellatrix Aug 14, 2017

ellatrix Aug 14, 2017

aduth Aug 14, 2017

ellatrix Aug 14, 2017

ellatrix commented Aug 14, 2017

ellatrix commented Aug 14, 2017

ellatrix commented Aug 14, 2017

ellatrix commented Oct 10, 2017

ellatrix commented Oct 11, 2017

Improved arrow navigation #2296

Improved arrow navigation #2296

Conversation

ellatrix commented Aug 8, 2017 • edited Loading

mattyrob commented Aug 9, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Aug 10, 2017

codecov bot commented Aug 10, 2017 • edited Loading

Codecov Report

ellatrix commented Aug 10, 2017

nylen commented Aug 10, 2017

ellatrix commented Aug 10, 2017

nylen commented Aug 10, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Aug 14, 2017

ellatrix commented Aug 14, 2017

ellatrix commented Aug 14, 2017

ellatrix commented Oct 10, 2017

ellatrix commented Oct 11, 2017

ellatrix commented Aug 8, 2017 •

edited

Loading

codecov bot commented Aug 10, 2017 •

edited

Loading

nylen commented Aug 10, 2017 •

edited

Loading