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

Fix/arrow key into textarea #3193

Merged
merged 5 commits into from
Oct 30, 2017
Merged

Fix/arrow key into textarea #3193

merged 5 commits into from
Oct 30, 2017

Conversation

BoardJames
Copy link

Description

Arrow keying up or down through the document would jump to the opposite cursor position to the expected for textarea elements (for example the code block).

For example pressing down arrow when above a code block would jump straight to the end of the code block and when pressing up arrow while below the code block would jump to the start.

I have inverted the condition so that the behaviour of the method placeCaretAtHorizontalEdge matches the comment on the method for the parameter isReverse and it now behaves consistently with the contenteditable sections.

I also fixed a test which was erroneously conflating isReverse to mean the same thing in two different contexts.

How Has This Been Tested?

Manually and with the already existing automated tests.

Screenshots (jpeg or gifs if applicable):

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.

…g in two different contexts

Esentially when placing the caret into a new section while moving
through the document in reverse we expect to put it at the end of the
section.

However when detecting if we are at the edge of the section going in
reverse we expect the caret to be at the start of the section.

The test was erronously expecting the two to be the same thing because
they both used the flag "isReverse".
@codecov
Copy link

codecov bot commented Oct 27, 2017

Codecov Report

Merging #3193 into master will not change coverage.
The diff coverage is 50%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3193   +/-   ##
=======================================
  Coverage   31.36%   31.36%           
=======================================
  Files         223      223           
  Lines        6393     6393           
  Branches     1136     1136           
=======================================
  Hits         2005     2005           
  Misses       3685     3685           
  Partials      703      703
Impacted Files Coverage Δ
blocks/editable/tinymce.js 0% <0%> (ø) ⬆️
editor/utils/dom.js 15.07% <100%> (ø) ⬆️

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 1b2c770...39c17b6. Read the comment docs.

@BoardJames BoardJames self-assigned this Oct 27, 2017
Copy link
Member

@ellatrix ellatrix left a comment

Choose a reason for hiding this comment

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

Works great. Sorry I didn't catch this.

@@ -150,7 +150,7 @@ export function placeCaretAtHorizontalEdge( container, isReverse ) {

if ( includes( [ 'INPUT', 'TEXTAREA' ], container.tagName ) ) {
container.focus();
if ( isReverse ) {
if ( ! isReverse ) {
Copy link
Member

Choose a reason for hiding this comment

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

Minor: When negating, I usually prefer to swap the blocks for readability. E.g.

! isSomething ? a() : b()

to

isSomething ? b() : a()

Copy link
Author

Choose a reason for hiding this comment

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

Ok I will make that minor change and then merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants