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

feat: add support for pageup and pagedown control keys #734

Conversation

JohannesFischer
Copy link
Contributor

What:
Adding support for PageUp and PageDown control block keys as requested in issue #694

Why:
To support more possible user inputs.

How:
Add PageDown and PageUp to keymap. The behavior equals the of Home and End keys.

Checklist:

  • N/A Documentation
  • Tests
  • Ready to be merged

Keys need to be added to special characters when this gets merged.

@ph-fritsche
Copy link
Member

Thanks for contributing to this library❤️

If I recall correctly, ArrowUp and ArrowDown have not been implemented earlier, because the exact new position of the cursor was hard to determine due to possible line wrapping.
What is the behavior with PageUp / PageDown on long text content in teaxtarea?

@JohannesFischer
Copy link
Contributor Author

JohannesFischer commented Oct 7, 2021

On long text content in texarea the cursor will jump to the very beginning or very end when pressing PageUp / PageDown.

Kapture 2021-10-07 at 11 22 24

I tested this on a Mac using Cmd+ArrowUp / Cmd+ArrowDown and a Linux machine with the actual keys. Additionally I tested the behavior on input[type="text"] on which the keys behave like Home and End.

@JohannesFischer
Copy link
Contributor Author

Just tested it again using a textarea with 100+ lines, in that case it jumps up and down a number of lines but not all the way to the beginning or end. So probably we have the same issue as with the arrow keys.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 7, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ad3fb81:

Sandbox Source
userEvent-PR-template Configuration

@codecov
Copy link

codecov bot commented Oct 7, 2021

Codecov Report

Merging #734 (ad3fb81) into main (2f900ef) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #734   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           50        50           
  Lines          958       965    +7     
  Branches       385       389    +4     
=========================================
+ Hits           958       965    +7     
Impacted Files Coverage Δ
src/keyboard/keyMap.ts 100.00% <ø> (ø)
src/keyboard/specialCharMap.ts 100.00% <ø> (ø)
src/keyboard/plugins/control.ts 100.00% <100.00%> (ø)

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 2f900ef...ad3fb81. Read the comment docs.

@ph-fritsche
Copy link
Member

I think the cursor movement won't be exactly the same on different platforms in a real browser environment either.

Maybe we should not encourage assuming the cursor position after one of these keys is used.
Even a close-to-real estimate how far the cursor might be moved would depend heavily on the effective width of the content-box which is not reliable in a no-layout environment.
@nickmccurdy What do you think?

The added entries in the keyMap are much needed and the changes for input look good to me.

Copy link
Member

@ph-fritsche ph-fritsche left a comment

Choose a reason for hiding this comment

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

@JohannesFischer Could you split up the behaviorPlugin? Keeping the changes for input and keyDef but removing PageUp/PageDown from textarea and contenteditable?
How to handle the cursor movement for the latter needs more discussion while the other parts are ready to be merged.

@JohannesFischer
Copy link
Contributor Author

@ph-fritsche Done. Split up the behavior for the textarea and editable content in behaviorPluginand removed the test case for textarea. Please check out the changes and let me know if that's what you were going for.

@ph-fritsche ph-fritsche merged commit f731f68 into testing-library:main Oct 15, 2021
@ph-fritsche
Copy link
Member

@all-contributors add @JohannesFischer code

@allcontributors
Copy link
Contributor

@ph-fritsche

I've put up a pull request to add @JohannesFischer! 🎉

@github-actions
Copy link

🎉 This PR is included in version 13.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@JohannesFischer
Copy link
Contributor Author

Thanks for the credit @ph-fritsche

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

Successfully merging this pull request may close these issues.

2 participants