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

Cell selection minor inconsistency #935

Closed
doornik opened this issue Nov 29, 2023 · 3 comments · Fixed by #940
Closed

Cell selection minor inconsistency #935

doornik opened this issue Nov 29, 2023 · 3 comments · Fixed by #940

Comments

@doornik
Copy link

doornik commented Nov 29, 2023

Home/End keys move horizontally (on same row), while Ctrl-Home/End move vertically (within the same column).

However, in the CellSelectionModel plugin, Home/End select vertically, while the Ctrl versions are not supported.

This can be seen in the spreadsheet examples.

@ghiscoding
Copy link
Collaborator

ghiscoding commented Dec 1, 2023

while the Ctrl versions are not supported.

Making a selection with Ctrl key will never be supported, the Shift Home/End was brought because it was asked in issue #794 and it made sense to support it but Ctrl would be a lot more complex to support it and I don't think we every will (we would have to keep every single cell that was clicked and at the moment we can only do range with SlickRange in slick.core). Supporting Ctrl is out of the question for that reason.

The Home/End only works vertically when using Shift, because of previous point (which was asked in #794).

I the current behavior is coming from the code shown below (without Ctrl is uses this.navigateRowStart() to go Home horizontally) and it was implemented by the original author of SlickGrid, I'm not really sure that I want to change this behavior though. @6pac do you have any thought on the subject? The Example to test with it is example-spreadsheet-dataview.html. Perhaps the behavior made more sense when we didn't have Shift+Home selection introduced in #854 but even then, I don't want to change that implementation, it works great as it is

SlickGrid/src/slick.grid.ts

Lines 5260 to 5264 in 251c91e

if (e.which === keyCode.HOME) {
handled = (e.ctrlKey) ? this.navigateTop() : this.navigateRowStart();
} else if (e.which === keyCode.END) {
handled = (e.ctrlKey) ? this.navigateBottom() : this.navigateRowEnd();
}

EDIT

Testing text selection in VSCode and it looks like instead of Shift+Home to make selection upward, I should have used the Ctrl+Shift+Home key combo instead. Below is what happens in VSCode

  1. Ctrl+Home : moves cursor to top index 0 (we already do that in SlickGrid)
  2. Shift+Home: makes text selections from current cursor position to first index (home) of the same line horizontally
    • in SlickGrid we are selecting all the cell upward vertically (different compared to VSCode)
  3. Ctrl+Shift+Home: makes text selection from current cursor position up to first row first cell, so it does selection horizontally and vertically
    • in SlickGrid this key combo does nothing

So I guess I could change the Shift+Home combo in SlickGrid to Ctrl+Shift+Home so that it better represent the typical applications like VSCode. However the correct behavior of Shift+Home would require extra work to implement and even then the correct behavior of Ctrl+Shift+Home should be to select on both side horizontally+vertically from current cell position and I don't really time to change again the behavior of PR #794 to make it all right. I could certainly change the combo from Shift+Home to Ctrl+Shift+Home but that's half a fix.

ghiscoding pushed a commit that referenced this issue Dec 2, 2023
the previous key combo were incorrect, the new combo is the following
- `Shift+Home` will select from left most (index 0) to current cell position horizontally
- `Shift+End` will select from current cell position until the last cell horizontally
- `Ctrl+Shift+Home` will select everything that is on the left side of current cell position and everything on top current position (horizontally left until first cell & vertically left until first row)
- `Ctrl+Shift+End` will select everything that is on the right side of current cell position and everything on bottom current position (horizontally right until last cell & vertically right until last row)
@ghiscoding
Copy link
Collaborator

Actually it wasn't too complex to modify the code to adapt it and support the correct behavior. I've opened #940 and I made it all work but I have to fix and add some more Cypress E2E tests. The new key combos are shown in the PR with print screens

ghiscoding added a commit that referenced this issue Dec 2, 2023
* fix: revamp all cell selection range with key combos, fixes #935
@ghiscoding
Copy link
Collaborator

ghiscoding commented Dec 2, 2023

fixed and improved by PR #940 and released in v5.5.6

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

Successfully merging a pull request may close this issue.

2 participants