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.

Sign up for GitHub

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

Jump to bottom

Input Interaction: better horizontal edge detection #14462

Merged
merged 9 commits into from
Mar 28, 2019
Merged

Input Interaction: better horizontal edge detection #14462

merged 9 commits into from
Mar 28, 2019

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Mar 15, 2019 •
edited
Loading

Description

Fixes #12114.

Fixes horizontal arrow navigation in empty containers. To reproduce, create an empty paragraph block and try to use the left/right arrow keys.

This completely rewrites isHorizontalEdge to check visual position instead of DOM position, just like isVerticalEdge.

How has this been tested?

Screenshots

Types of changes

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.

@ellatrix ellatrix added the [Status] In Progress Tracking issues with work in progress label Mar 15, 2019
@oandregal oandregal added the [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... label Mar 20, 2019
@ellatrix ellatrix force-pushed the try/alt-horizontal-edge branch from 1d13528 to cdbe050 Compare March 20, 2019 12:48
@ellatrix ellatrix added this to the 5.4 (Gutenberg) milestone Mar 20, 2019
@ellatrix ellatrix removed the [Status] In Progress Tracking issues with work in progress label Mar 20, 2019
@oandregal
Copy link
Member

Tried this patch on an empty paragraph and a table (as in the original issue):

  • on empty paragraph:

    • Chrome: works as expected.
    • Firefox: arrow up doesn't work after the first time.

  • on the table block:

    • Chrome: left/right work as expected; down works as right, up works as left.
    • Firefox: left/right work as expected; down works as right, up doesn't work on empty cells (in cell with content works as left).

As for the down/up keys: I've tested tables in office applications (writer, spreadsheets). There, the arrow down moves to the cell below instead of to the right cell (and the arrow up to the cell above instead of the left cell). Although I think that would be the desirable behavior, this is great progress and should be fine to track that in a different issue if you think it's better.

Tested on Firefox 66 and Chrome 71, Linux OS.

@ellatrix
Copy link
Member Author

@nosolosw Thanks for testing. It's definitely not easy to fix it for Firefox, as it totally mispositions the caret rectangle sometimes. I'll see if I can make a more reliable calculation.

Regarding the up and down arrows working differently in a table, I think this is a separate issue, but worth fixing in a separate PR. The problem is that we first look in the DOM for a tabbable element, then calculate the new caret position.

@ellatrix
Copy link
Member Author

But that confirms again that we shouldn't be looking in the DOM when repositioning the caret with arrow up/down. ;)

@ellatrix
Copy link
Member Author

Maybe these functions are also in the wrong package... :/

@ellatrix ellatrix force-pushed the try/alt-horizontal-edge branch from 3d6ff67 to dc9694c Compare March 26, 2019 09:51
@ellatrix
Copy link
Member Author

@nosolosw I fixed vertical edge as well, and also merged the logic.

oandregal
oandregal reviewed Mar 26, 2019
View reviewed changes
packages/dom/src/dom.js Outdated Show resolved Hide resolved
oandregal
oandregal reviewed Mar 26, 2019
View reviewed changes
packages/dom/src/dom.js Outdated Show resolved Hide resolved
oandregal
oandregal reviewed Mar 26, 2019
View reviewed changes
packages/dom/src/dom.js Outdated Show resolved Hide resolved
oandregal
oandregal reviewed Mar 26, 2019
View reviewed changes
packages/dom/src/dom.js Outdated Show resolved Hide resolved
@ellatrix
Copy link
Member Author

Thanks for reviewing @nosolosw! I addressed your feedback.

@ellatrix ellatrix requested a review from oandregal March 28, 2019 08:13
@ellatrix ellatrix force-pushed the try/alt-horizontal-edge branch from eafa076 to 78480a1 Compare March 28, 2019 08:15
oandregal
oandregal reviewed Mar 28, 2019
View reviewed changes
packages/dom/README.md
@@ -148,7 +148,7 @@ _Parameters_

_Returns_

- `boolean`: True if at the edge, false if not.
- `boolean`: True if at the vertical edge, false if not.
Copy link
Member

Choose a reason for hiding this comment

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

isn't it lovely that we don't have to remember to update this? :)

oandregal
oandregal approved these changes Mar 28, 2019
View reviewed changes
Copy link
Member

@oandregal oandregal left a comment

Choose a reason for hiding this comment

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

This makes keyboard navigation a pleasure! 🌟

@oandregal oandregal mentioned this pull request Mar 28, 2019
Closed
@oandregal
Copy link
Member

Created #14675 for the UP/DOWN navigation within a table block.

@ellatrix
Copy link
Member Author

Thanks for your review! ✨

@ellatrix ellatrix merged commit e4108fc into master Mar 28, 2019
@ellatrix ellatrix deleted the try/alt-horizontal-edge branch March 28, 2019 09:33
@ellatrix ellatrix added the [Type] Bug An existing feature does not function as intended label Mar 28, 2019
This was referenced Apr 1, 2019
Merged
Merged
ellatrix added a commit that referenced this pull request Apr 3, 2019
@ellatrix
Input Interaction: better horizontal edge detection (#14462)
4f70e45 
* Input Interaction: better horizontal edge detection

* Correct BR ranges

* Add e2e test

* Increase buffer for Firefox

* Clean up

* Merge isEdge logic

* Fix typo

* Address feedback

* Build docs
ellatrix added a commit that referenced this pull request Apr 4, 2019
@ellatrix
Bug fixes from 5.4 (#14796)
09ac4a5 
* RichText: improve format boundary style (#14519)

* RichText: improve format boundary style

* rgb => rgba

* Paste: check plain text for gutenberg content (#14536)

* Make ClipboardButton inside a block work correctly in Safari (#7106)

* Make ClipboardButton inside a block work in Safari

* Update changelogs

* Block Editor: Update "Next" to "Unreleased" per guidelines

https://github.com/WordPress/gutenberg/blob/master/packages/README.md#maintaining-changelogs

* Input Interaction: always expand single line selection vertically (#14487)

* Input Interaction: always expand single line selection vertically

* Add e2e test

* Use MenuItem instead of IconButton (#14569)

* Remove id, infoid, label and aria-describedby from MenuItem (#14423)

* Preformatted: save line breaks as characters (#14653)

* Preformatted: save line breaks as characters

* Update e2e test

* Remove negative toolbar position rules from full-aligned blocks. (#14669)

* Fix issue with double scrollbar in Fullscreen Mode (#14677)

This PR fixes an issue where the sidebar would have two scrollbars when in fullscreen mode.

* Fix WordPress embed block resolution (#14658)

* Retry failing embeds with trailing slash (#14705)

* Fix embedding Twitter URLs with a trailing slash (Closes #12664)

* Fix race condition for WordPress URLs that end in slashes, add test

* API Fetch: Fix error on empty OPTIONS preload data (#14714)

* Input Interaction: better horizontal edge detection (#14462)

* Input Interaction: better horizontal edge detection

* Correct BR ranges

* Add e2e test

* Increase buffer for Firefox

* Clean up

* Merge isEdge logic

* Fix typo

* Address feedback

* Build docs

* Fix memize option key typo (#14750)

* RichText: unify active formats, 'selectedFormat' and 'placeholderFormat' (#14411)

* RichText: unify active formats, 'selectedFormat' and 'placeholderFormat'

* Add extra e2e test

* Only should boundary style when focused

* Update docs

* Try to trigger tests with Travis

* Restore Travis config
@ellatrix ellatrix mentioned this pull request Apr 10, 2019
Closed
@aduth aduth mentioned this pull request Jul 31, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oandregal oandregal oandregal approved these changes

@aduth aduth Awaiting requested review from aduth

@gziolo gziolo Awaiting requested review from gziolo

@youknowriad youknowriad Awaiting requested review from youknowriad

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@nerrad nerrad Awaiting requested review from nerrad

@ntwb ntwb Awaiting requested review from ntwb

@talldan talldan Awaiting requested review from talldan

Assignees
No one assigned
Labels
[Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
5.4 (Gutenberg)
Development

Successfully merging this pull request may close these issues.

Tables block - BUG: Arrow keys not working as expected...
2 participants
@ellatrix @oandregal