handsontable icon indicating copy to clipboard operation
handsontable copied to clipboard

Fix an issue with dropdownMenu not being displayed on a nestedHeaders-enabled instance with all rows trimmed out.

Open jansiegel opened this issue 2 years ago • 2 comments

Context

The bug described in #9753 was caused by a more broad issue:

When the Handsontable instance does a full redraw from a callback run on afterOnCellMouseDown, all the callbacks for the succeeding click event bound to the rootElement are blocked by the fact that the DOM content of rootElement's children was modified.

  • In the simplest example: https://jsfiddle.net/js_ziggle/ecqg5w9b/
  • In Handsontable context, this can be achieved without nestedHeaders, filters, and dropdownMenu: https://jsfiddle.net/js_ziggle/1mxgrtco/ By passing a custom header-rendering function, we're forcing the full redraw of those. Clicking on them won't trigger the click event callback defined below.

My fix for #9753 pushes the render-triggering function (selecting columns) after all the previously-declared click callbacks were already run by placing it at the end of the click event stack and asynchronously removing the newly-added callback.

Additionally, I corrected some typos in the code I found while searching for this fix.

How has this been tested?

Added a test case for the issue.

Types of changes

  • [x] 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. #9753

Affected project(s):

  • [x] handsontable
  • [ ] @handsontable/angular
  • [ ] @handsontable/react
  • [ ] @handsontable/vue
  • [ ] @handsontable/vue3

Checklist:

jansiegel avatar Aug 30 '22 08:08 jansiegel

Launch the local version of documentation by running:

npm run docs:review 4ec2da9a3875ac37395c46d947cd94931a965715

github-actions[bot] avatar Sep 08 '22 09:09 github-actions[bot]

@budnix In https://github.com/handsontable/handsontable/pull/9846/commits/c7bb49f24378def1ea364f750632df445f679bc0 I took your idea from https://github.com/handsontable/handsontable/commit/08895d41f91294e5de8521b05423c951a0f8cf8d, but modified it to utilize both calculators for fully visible rows/columns and partial ones. It looks like it works fine, but I'm not 100% sure if it doesn't break your initial idea that introduced this issue. Could you verify if it makes sense?

jansiegel avatar Sep 13 '22 11:09 jansiegel

Does it also fix #9930?

warpech avatar Sep 23 '22 08:09 warpech

Does it also fix #9930?

Might be, I'll check that

budnix avatar Sep 23 '22 09:09 budnix

@budnix In c7bb49f I took your idea from 08895d4, but modified it to utilize both calculators for fully visible rows/columns and partial ones. It looks like it works fine, but I'm not 100% sure if it doesn't break your initial idea that introduced this issue. Could you verify if it makes sense?

Initially, the rendering clue seemed like what needed to be fixed. But after playing with that, I saw some issues. The first problem is performance. On every scroll move, there are two more calculators created only for the count value check - that hurts a little bit. And second, for oversized rows, the issue still exists - the dropdown menu can't be opened.

But it seems that a few days between code reviews refreshed my mind 😄 I reinvested the issue and found that the column (and row) for non-nested headers had the same issue. They were fixed 7 years ago by adding DOM caching layer - that was a part of the performance task (https://github.com/handsontable/handsontable/commit/fbfcbb44525461ac83b68c3f27ebbfd17522911b). The NestedHeaders plugin lacks that cache layer implementation that's why I'd propose adding one. I've created a new PR with the proposition. The new PR contains all the relevant changes from this one.

budnix avatar Sep 23 '22 09:09 budnix