Skip to content
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

WIP: Try vertical arrow key navigation in WritingFlow #17084

Closed
wants to merge 6 commits into from

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Aug 19, 2019

Description

Iteration on #16957, which attempts to bring vertical arrow key navigation to the WritingFlow component.

When an up/down arrow key is pressed searches in the relevant direction from the caret until a tabbable text field is found. If none is found, tries searching from the right and left of the caret upwards or downwards until a tabbable is found.

How has this been tested?

Screenshots

Types of changes

New feature (non-breaking change which adds functionality)

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.

@talldan talldan added [Type] Enhancement A suggestion for improvement. [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... labels Aug 19, 2019
@talldan talldan self-assigned this Aug 19, 2019
@talldan talldan force-pushed the try/multidirectional-arrow-key-nav branch 2 times, most recently from e2d0e66 to 90b2818 Compare August 19, 2019 11:37
}

const element = document.elementFromPoint( xPosition, yPosition );
if ( element && isTabbableTextField( element ) ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works ok for now, but would fail if the element at the point was some formatting. A solution would be to check parent elements as well.

There is also elementsFromPoint, which although marked on MDN as experimental, seems to be pretty widely supported:
https://developer.mozilla.org/en-US/docs/Web/API/DocumentOrShadowRoot/elementsFromPoint

@talldan
Copy link
Contributor Author

talldan commented Aug 19, 2019

@ellatrix This is a very early attempt at what we discussed in https://github.com/WordPress/gutenberg/pull/16957—vertical keyboard nav based on element screen positioning.

It seems to work ok so far. I'm not sure how I'd handle something like Cmd+Up to get straight to the top of a range, but for single arrow key presses it seems surprisingly ok!

@talldan talldan changed the title WIP: Try vertical arrow key in WritingFlow WIP: Try vertical arrow key navigation in WritingFlow Aug 20, 2019
@talldan
Copy link
Contributor Author

talldan commented Aug 20, 2019

I thought of a drawback of this approach—there'll be issues with some elements like toolbars covering nearby inputs. Ideally there would be a way to filter out those elements from the search for a tabbable element.

@ellatrix
Copy link
Member

I thought of a drawback of this approach—there'll be issues with some elements like toolbars covering nearby inputs. Ideally there would be a way to filter out those elements from the search for a tabbable element.

This should not happen with hiddenCaretRangeFromPoint.

@ellatrix ellatrix self-requested a review August 20, 2019 09:51
@talldan talldan force-pushed the try/multidirectional-arrow-key-nav branch from 502e4a2 to d15949d Compare August 29, 2019 10:08
@talldan
Copy link
Contributor Author

talldan commented Aug 29, 2019

I've switched to using elementsFromPoint which also solves the issue of overlapping elements.

This does seem to work pretty well in my testing so far. Not sure if there are any additional things I need to think about.

Potentially I should constrain the functionality to the bounds of the WritingFlow container.

Another feature that I'm thinking about implementing is the use of Cmd/Ctrl with arrow keys keys to move straight to a table or grid's boundaries (as I had in my table block PR #16957).

I think that could be considered on a separate PR and work on this incrementally.

@talldan
Copy link
Contributor Author

talldan commented Sep 11, 2019

Ah, I've realised this little experiment of using elementsFromPoint doesn't work as it's unable to navigate to elements that are off-screen. I'll have to rework the PR. I think a good approach would be getting a list of focusableNodes, filtering out those that aren't above/below the caret, and then sorting based on vector distance.

@ellatrix
Copy link
Member

ellatrix commented Oct 9, 2019

Ah, I've realised this little experiment of using elementsFromPoint doesn't work as it's unable to navigate to elements that are off-screen. I'll have to rework the PR.

Have you had a look at how we approach this vertically? I believe we try to scroll, then look again etc., but I'd have to check. Ideally the implementation is very similar so logic can be reused. This can be found in the DOM package.

Otherwise I think the work done here is very promising! Would it be possible to write some e2e test, similar to the ones we have for vertical navigation?

@talldan talldan force-pushed the try/multidirectional-arrow-key-nav branch from 8e2faca to bfe2233 Compare October 17, 2019 09:26
@talldan
Copy link
Contributor Author

talldan commented Oct 17, 2019

@ellatrix I've taken a different approach now, selecting the tabbable that's closest in distance. It seems to work better than before, and I don't have to worry if the element is off-screen.

I need to consider how it'll work when navigating between blocks, but so far so good.

I'll have a look at writing some e2e tests.

@talldan talldan added the [Status] In Progress Tracking issues with work in progress label Dec 5, 2019
@talldan talldan closed this May 12, 2020
@talldan talldan deleted the try/multidirectional-arrow-key-nav branch May 12, 2020 02:07
@talldan
Copy link
Contributor Author

talldan commented May 12, 2020

Superceded by #22105

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants