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

VirtualScroll takes over arrow keys #156

Closed
jlongster opened this issue Mar 16, 2016 · 16 comments · Fixed by #159
Closed

VirtualScroll takes over arrow keys #156

jlongster opened this issue Mar 16, 2016 · 16 comments · Fixed by #159
Labels

Comments

@jlongster
Copy link

I am rendering an input in an item, and the cursor inside the input cannot be controlled with the left/right arrows keys. What is taking over this behavior? Not much more to say about it, I'm sure you know what I'm talking about :) If not I can post a small test case.

Is there a way to turn it off?

@jlongster
Copy link
Author

Sorry for the sucky bug report. I see in #121 that other components support keyboard navigation, but I don't see what that has to do with VirtualScroll which is not a specific grid/table component. I looked through the docs but I don't see anything concerning keyboard controls (for VirtualScroll).

I will post a test case tomorrow; it's late tonight and I thought you might know exactly where to look.

@bvaughn
Copy link
Owner

bvaughn commented Mar 16, 2016

Hey @jlongster! Hope you're well.

No need to post a test case. I know what's causing the issue you report. (It's this method in Grid.)

I see in #121 that other components support keyboard navigation, but I don't see what that has to do with VirtualScroll which is not a specific grid/table component. I looked through the docs but I don't see anything concerning keyboard controls (for VirtualScroll).

Both VirtualScroll and FlexTable components use a Grid internally. Grid supports arrow-key navigation and so the others do as well.

However, as you point out in this example...it does make inner <input>s awkward. Even if I disabled the left/right arrow keys for Grids with only 1 cell (which would resolve the issue you're mentioning for VirtualScroll and FlexTable) it would still potentially cause problems for multi-line <textarea> tags.

Now I am questioning this behavior as a default. As you mention the discussion on #121 mentions potentially refactoring the keyboard navigation out of Grid and into a HOC. That would allow it to be opt-in. I'm thinking... maybe that's a more desirable solution.

@bvaughn bvaughn added the bug label Mar 16, 2016
@bvaughn
Copy link
Owner

bvaughn commented Mar 16, 2016

I just published version 5.5.4 which should improve the behavior you're talking about. I'm going to leave this issue open for more consideration though. It's not clear to me whether I should...

  1. Make arrow-key navigation opt-in (disabled by default).
  2. Allow users to disable arrow-key navigation via a Grid property.
  3. Require users to intercept key-down events (and prevent-default) if they want to disable arrow key navigation. (I could update my key-down handler to ignore events where defaultPrevented is true.)
  4. Ignore arrow key events anytime event.target is an input, textarea, or other type of form control.

I'm leaning toward option 4.

Edit: One awkward edge-case about approach #4 is that for Safari and Chrome, the browser's built-in arrow-key-scrolling will take over if I ignore the event and the cursor is at the edge of the input. The resulting scroll doesn't snap to grid cells. That makes the interaction kind of weird.

@Guria
Copy link

Guria commented Mar 16, 2016

4 is not best option since it wouldn't fit a case with control "selected" row from arrow keys.

@cgestes
Copy link

cgestes commented Mar 16, 2016

4 would have to take contenteditable into consideration also.

On Wed, 16 Mar 2016 07:40 Aleksey Guryanov, [email protected]
wrote:

4 is not best option since it wouldn't fit a case with control "selected"
row from arrow keys.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#156 (comment)

@jlongster
Copy link
Author

I'm leaning towards #1. Why does VirtualScroll have this at all? It seems a little strange that VirtualScroll inherits from Grid, because the type of content that VirtualScroll has it up to the user. I'm sure it's just a naming thing and there's a lot technically shared, but I don't think keyboard navigation belongs in the base class. I never want to override the default browser behaviors, and I want up/down to scroll normally, not go to the next item. I can scroll with my trackpad as well, and VirtualScroll should act like a normal big div being scrolled. (I can see FlexTable being different)

In fact, now that I'm looking, there is strange interaction with up/down. If I scroll with the trackpad so that half of the top item is cut off, pressing "down" jumps down items but the top item is still cut off because there's an offset. I'd very much prefer all of this to be opt-in.

@jlongster
Copy link
Author

Thanks for a quick release also! That makes it a little better.

@jlongster
Copy link
Author

FWIW in the latest release, pressing up/down scrolls the container even though the input is focused. As far as I know that shouldn't happen: if I'm in an input the browser doesn't scroll it's container. I know you did a quickfix but just wanted to explain other side effects.

No rush on this!

@bvaughn
Copy link
Owner

bvaughn commented Mar 16, 2016

Thanks for the feedback folks. After sleeping on it, I'm leaning towards creating a HOC that manages arrow-key "snapping" behavior (so you scroll by exactly the width/height of a cell) and just leaving that off by default entirely.

Busy week for me but I'll try to see if I can't put together a release by the weekend.

@bvaughn
Copy link
Owner

bvaughn commented Mar 16, 2016

Why does VirtualScroll have this at all? It seems a little strange that VirtualScroll inherits from Grid, because the type of content that VirtualScroll has it up to the user.

@jlongster VirtualScroll and FlexTable compose a single-column Grid (rather than inherit). They're just decorator components that expose convenient alternative interfaces. This dramatically reduces my code duplication. :)

@jlongster
Copy link
Author

I figured they share a lot of code, I think the Grid name is slightly confusing though. But that makes sense. I knew they didn't literally inherit, but rather compose, which is good :)

@bvaughn
Copy link
Owner

bvaughn commented Mar 16, 2016

Really! Confusing? :) I thought grid, datagrid, etc. was a pretty standard name but... naming things is hard! I'd be curious what you would find a more appropriate name if you have any suggestions.

@jlongster
Copy link
Author

I have no suggestions :) My initial impression of grid is the layout of multiple things both horizontally and vertically, while the VirtualScroll just optimizes a big list. It doesn't really matter, I think grid is an appropriate name if interpreted a different way! My confusion was short and it's easy to learn what each component does.

@bvaughn
Copy link
Owner

bvaughn commented Mar 17, 2016

I believe I am starting to lean toward option 5: remove this functionality entirely. :)

I've been working on option 1 in a branch- removing the arrow-key handling from Grid and into a HOC and it ends up being kind of complicated/awkward.

@bvaughn bvaughn mentioned this issue Mar 17, 2016
@bvaughn
Copy link
Owner

bvaughn commented Mar 17, 2016

I've put together PR #159 to illustrate my thoughts on the direction for option 1. Feedback is welcome. :)

bvaughn added a commit that referenced this issue Mar 19, 2016
@bvaughn
Copy link
Owner

bvaughn commented Mar 19, 2016

Btw @jlongster, version 6 was released just a moment ago. You should be able to completely opt-out of key-arrow handling now (it is the default in fact). Thanks for raising this issue. :)

bvaughn added a commit that referenced this issue Mar 20, 2016
Replaced react-pure-render with react-addons-shallow-compare
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 a pull request may close this issue.

4 participants