-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improved arrow navigation #2296
Conversation
5c61e54
to
ee64866
Compare
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. |
editor/modes/visual-editor/block.js
Outdated
followingEditor.selection.collapse( false ); | ||
} | ||
} else { | ||
window.setTimeout( () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we should get rid of this, because it also prevents the positioning if you hold the arrow key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it does work with caretRangeFromPoint
... FF supports both, and Chrome and Safari will fall back to that one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 😖
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Chrome and Safari, it will work moving down, but not up. In Firefox it still won't work at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure you bind that even early since otherwise handlers inside tinymce might want to act on the down key as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
e37f542
to
8ee9e35
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
I'm not sure why, but Codecov has the wrong base commit for this PR: Let's try this |
@aduth Do you think I should the EditableProvider for this similarly to your focus PR? |
I pushed 2 other branches to try to track down the coverage issue:
The base commit of this PR should be d7033d5 which appears to have a passing Codecov build. However, in the list of builds for I am not entirely sure what happened but I would suggest that we rebase this PR and separately add that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
editor/modes/visual-editor/block.js
Outdated
|
||
const isVerticalEdge = ( { editor, reverse } ) => { | ||
const rangeRect = editor.selection.getBoundingClientRect(); | ||
const buffer = rangeRect.height / 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this buffer for, and how did you come to arrive at rangeRect.height / 2
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I'll comment)
editor/modes/visual-editor/block.js
Outdated
} else { | ||
window.setTimeout( () => { | ||
const buffer = rect.height / 2; | ||
const editorRect = followingEditor.getBody().getBoundingClientRect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know followingEditor
still exists after this setTimeout
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I will fix that. We can remember the first position, and discard it when you press any other key. |
cbcb14d
to
33557b3
Compare
@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... :/ |
(Or at least the first, second last, and last line.) |
Hm, not sure why this PR was closed... I'll rebase in a new one. |
Opening a new PR. |
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.