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

Changed unnatural checkbox behaviour after double click #10748

Merged
merged 4 commits into from Jan 26, 2024

Conversation

wszymanski
Copy link
Contributor

@wszymanski wszymanski commented Jan 24, 2024

Context

I can see two problems.

  1. Unnatural checkbox behaviour after double click.

After a double click, a value has been inverted. I can't see a reason for such behaviour. It was introduced within 1c7a93a (2013 year), and it's very hard to find a reason for that feature. I can't see similar behaviour in Google Sheets or the Excel desktop. I propose to get rid of this solution. This PR did it.

So far, double-clicking on a cell (TD element) has been inverting the checkbox value. However, it had side effects when a click was performed directly on the input[type=checkbox] element. This PR fixes that.

  1. Big lag.

The lag problem shown within the issue is related to the setDataAtCell method. It's very clearly visible after clicking on the checkbox, as it is the easiest way of changing value. A lag can also be seen in changing value using fast editing or from API. The longest time is spent executing the instance.view.render method, which also takes much time when displaying the Handsontable instance with a big dataset for the first time. We don't have a method which re-renders only part of the cells visible in the viewport. Probably, it should be done within another, bigger task.

How has this been tested?

I tested a checkbox using clicks, double clicks, and space presses.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature or improvement (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Additional language file or change to the existing one (translations)

Related issue(s):

  1. https://github.com/handsontable/dev-handsontable/issues/1540

Affected project(s):

  • handsontable
  • @handsontable/angular
  • @handsontable/react
  • @handsontable/vue
  • @handsontable/vue3

Checklist:

  • I have reviewed the guidelines about Contributing to Handsontable and I confirm that my code follows the code style of this project.
  • My change requires a change to the documentation.

@wszymanski wszymanski self-assigned this Jan 24, 2024
@wszymanski
Copy link
Contributor Author

wszymanski commented Jan 24, 2024

@evanSe @adrianszymanski89 If the purpose of the task was to repair the lag, it would be a more complicated process, which, in my opinion, may take a lot of time and should be solved as a separate task. What's more developed change within this PR can, in my opinion, be treated as a breaking change.

@wszymanski wszymanski marked this pull request as ready for review January 24, 2024 13:27
Copy link
Member

@jansiegel jansiegel left a comment

Choose a reason for hiding this comment

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

We could potentially remove this feature (if we don't want to keep it, that's a question for @evanSe), but I have an alternative solution:

I think the main problem with this task is the fact that when you double-click the actual checkbox, something like this happens:

  1. Cell contains an unchecked checkbox
  2. User double-clicks the checkbox element
  3. Checkbox gets checked
  4. Checkbox gets unchecked
  5. (pause) Checkbox gets checked again

I think that 5. (pause) Checkbox gets checked again is the main problem here.

My suggestion would be to leave the double-click functionality when interacting with a cell, but remove it for the checkbox. This way double-clicking in the general cell area would flip the checkbox state, but double-clicking on the checkbox would be synonymous to two separate clicks (so the chebkox would get checked -> unchecked).

I think this approach would require a really minor change here:

if (event && event.type === 'mouseup') {

Basically, we'd need to change this line to

if (event && event.type === 'mouseup' && event.target.nodeName === 'TD') {

WDYT, @wszymanski?

@wszymanski
Copy link
Contributor Author

wszymanski commented Jan 25, 2024

My suggestion would be to leave the double-click functionality when interacting with a cell, but remove it for the checkbox. This way double-clicking in the general cell area would flip the checkbox state, but double-clicking on the checkbox would be synonymous to two separate clicks (so the chebkox would get checked -> unchecked).

I think this approach would require a really minor change here:

if (event && event.type === 'mouseup') {

Basically, we'd need to change this line to

if (event && event.type === 'mouseup' && event.target.nodeName === 'TD') {

WDYT, @wszymanski?

You are correct. I didn't notice that this feature mainly concerns clicking on TD. As a consequence, your solution is definitely better, and consequently, there is no point in talking about breaking change.

@evanSe
Copy link
Member

evanSe commented Jan 25, 2024

We could potentially remove this feature (if we don't want to keep it, that's a question for @evanSe), but I have an alternative solution:

I think the main problem with this task is the fact that when you double-click the actual checkbox, something like this happens:

  1. Cell contains an unchecked checkbox
  2. User double-clicks the checkbox element
  3. Checkbox gets checked
  4. Checkbox gets unchecked
  5. (pause) Checkbox gets checked again

I think that 5. (pause) Checkbox gets checked again is the main problem here.

My suggestion would be to leave the double-click functionality when interacting with a cell, but remove it for the checkbox. This way double-clicking in the general cell area would flip the checkbox state, but double-clicking on the checkbox would be synonymous to two separate clicks (so the chebkox would get checked -> unchecked).

I think this approach would require a really minor change here:

if (event && event.type === 'mouseup') {

Basically, we'd need to change this line to

if (event && event.type === 'mouseup' && event.target.nodeName === 'TD') {

WDYT, @wszymanski?

With this solution will the user see it checked then unchecked? if after double-clicking a unchecked box its state is still unchecked I am for this solution

@jansiegel
Copy link
Member

jansiegel commented Jan 25, 2024

With this solution will the user see it checked then unchecked? if after double-clicking a unchecked box its state is still unchecked I am for this solution

@evanSe it would work like this:

  1. Double-clicking the TD element:
  • Checkbox get checked
  1. Double-clicking the input[type=checkbox] element:
  • Checkbox get checked after the first click
  • Checkbox gets unchecked after the second click

@wszymanski wszymanski merged commit c1ee222 into develop Jan 26, 2024
24 checks passed
@wszymanski wszymanski deleted the feature/issue-dev-1540 branch January 26, 2024 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants