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

Interaction between range selection/excel copy paste + click to edit is bugged #36

Closed
5 tasks done
Jatastico opened this issue Jan 25, 2023 · 5 comments · Fixed by #52
Closed
5 tasks done

Interaction between range selection/excel copy paste + click to edit is bugged #36

Jatastico opened this issue Jan 25, 2023 · 5 comments · Fixed by #52

Comments

@Jatastico
Copy link

Jatastico commented Jan 25, 2023

Hi, i noticed that if you want a table that allows range selecion/excel style copy paste and editable cells on click that some weird interactions happen, this is noticeable in example 3: https://ghiscoding.github.io/slickgrid-react/#/Example3

if you select a range of cells, copy them and try to paste them elsewhere, it looks like you can't select a new active cell to paste on, the only way to do it is through "range selecting" a single cell, then the paste function will be able to recognise the new active cell and paste there. this only happens if cell editing itself is ON, i tested locally disabling cell edit on click allows the expected copy paste behaviour as seeon on example 7: https://ghiscoding.github.io/slickgrid-react/#/Example7

Reproduction

On example 3:
Double click cell edit: select and copy a range of cells, click on another cell where to paste. the click won't register a new active cell. only if you range select a cell(s) will you be able to paste.

Expectation

clicking on a single cell and pasting should paste cell copied, but its seems stuck on previous selection

Environment Info

https://ghiscoding.github.io/slickgrid-react/#/Example3

Validations

@ghiscoding
Copy link
Owner

Hmm I never use this selection feature so I haven't noticed it before but Angular-Slickgrid Example 3 seem to exhibit the same problem. I think it might be since we removed jQueryUI from SlickGrid 3.0, however if we look at the original SlickGrid Copy & Paste Example, it seems to be working fine. I don't have really have much time to look at this but I assume since Angular-Slickgrid also has the problem then it might be coming from Slickgrid-Universal extensions slickCellExcelCopyManager.ts or slickCellExternalCopyManager.ts

@Jatastico
Copy link
Author

the original Example works because cell editing -autoedit- is not enabled there (i assume since that's how it works here as well, as seen in example 7), so I'd say the problems comes from a conflict of both features at the same time.

I was just looking for grid I'd like to use and noticed this so I thought I'd give a heads up, great job on it overall regardless

@ghiscoding
Copy link
Owner

hey so I took a quick look and I can see that it is actually working if we use the keyboard to change cell position before pasting, it however doesn't work when using the mouse to change the cell position. I'll investigate more later but at this point, I think that what is happening is actually that the cell is not actually active even though it looks active. I know that when we use autoCommitEdit, we had code in place to remove the active cell and that was the thing that doing the edit commit. I'm not sure why the mouse click doesn't change the active cell but it seems to be the origin of the issue

AwnrcmMzbz

@ghiscoding
Copy link
Owner

ghiscoding commented Feb 9, 2023

EDIT

So I found how to make it work, however it requires a SlickGrid PR to be merged and released, it's funny because I just created that PR for something but it turns out that it will be needed to fix this issue (with some other code change to be applied in Slickgrid-Universal lib).

Here's a quick demo of all of the fixes applied from a Slickgrid-Universal demo (React-Slickgrid uses Slickgrid-Universal behind the scene, that is the common lib that I created for all of my other libs like Angular-Slickgrid, Slickgrid-React, etc...). Below is with auto-edit enabled, but it works just the same without it. The issue was really related to the active cell that I mentioned in previous post

brave_WP2kRdSetx

So I'm not sure when that will be out since it requires a new release from Slickgrid-Universal (my lib) and also a release from the SlickGrid lib (not mine, I'm only a contributor there). But I assume that I should have a fix withing the next couple weeks at the max :)

@ghiscoding
Copy link
Owner

fixed and release in v2.5.0 :)

ghiscoding added a commit to ghiscoding/slickgrid-universal that referenced this issue Feb 23, 2023
- fixes a regression caused by previous PR #901 and identified in Angular-Slickgrid issue [1103](ghiscoding/Angular-Slickgrid#1103), the previous PR #901 was put in place to fix cell external copy in a bug reported in Slickgrid-React issue [36](ghiscoding/slickgrid-react#36)
- this bug actually came from a very old patch that was put in place via `suppressActiveCellChangeOnEdit: true` in SlickGrid [PR 243](6pac/SlickGrid#243) and this flag was to avoid trigger an event when the active cell changes, this event was being listened by SlickCellSelectionModel and when triggered was then sending another event with the cell ranges that changed, which then sent another event onSelectedRangesChanged which was itself listened by SlickCellExternalCopyManager and when triggered was calling a `grid.focus()` and when it did that, it was making the editor loses its focus (hence the implementation of `suppressActiveCellChangeOnEdit`) which is required by SlickCellExternalCopyManager to be able to copy & paste cell ranges. After investigating, I was able to remove the use of `suppressActiveCellChangeOnEdit` (now disabled globally) by simply calling `grid.focus()` in each editor prior to itself getting its own focus, so at least our focus remains in the grid and our editor no longer loses its focus and we are also able to finally use Editor + CellExternalCopyManager at the same time without conflict anymore
- hopefully this fixes all regressions and it also removes some old hacks (suppressActiveCellChangeOnEdit) that was put in place, in summary, it should be a lot cleaner now
ghiscoding added a commit to ghiscoding/slickgrid-universal that referenced this issue Feb 23, 2023
…ork (#917)

- fixes a regression caused by previous PR #901 and identified in Angular-Slickgrid issue [1103](ghiscoding/Angular-Slickgrid#1103), the previous PR #901 was put in place to fix cell external copy in a bug reported in Slickgrid-React issue [36](ghiscoding/slickgrid-react#36)
- this bug actually came from a very old patch that was put in place via `suppressActiveCellChangeOnEdit: true` in SlickGrid [PR 243](6pac/SlickGrid#243) and this flag was to avoid trigger an event when the active cell changes, this event was being listened by SlickCellSelectionModel and when triggered was then sending another event with the cell ranges that changed, which then sent another event onSelectedRangesChanged which was itself listened by SlickCellExternalCopyManager and when triggered was calling a `grid.focus()` and when it did that, it was making the editor loses its focus (hence the implementation of `suppressActiveCellChangeOnEdit`) which is required by SlickCellExternalCopyManager to be able to copy & paste cell ranges. After investigating, I was able to remove the use of `suppressActiveCellChangeOnEdit` (now disabled globally) by simply calling `grid.focus()` in each editor prior to itself getting its own focus, so at least our focus remains in the grid and our editor no longer loses its focus and we are also able to finally use Editor + CellExternalCopyManager at the same time without conflict anymore
- hopefully this fixes all regressions and it also removes some old hacks (suppressActiveCellChangeOnEdit) that was put in place, in summary, it should be a lot cleaner now
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 a pull request may close this issue.

2 participants