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

The `getCell` method should not return `undefined` #5608

Closed
jansiegel opened this Issue Nov 22, 2018 · 7 comments

Comments

Projects
None yet
4 participants
@jansiegel
Copy link
Member

jansiegel commented Nov 22, 2018

Description

The getCell method (according to our docs) should return a HTMLTableCellElement if the requested cell was rendered or null otherwise. In reality, it can also return undefined, which is not a desired behaviour.

This situation occurs when we're trying to get a cell from a row which is rendered, but in a column which is not.

Steps to reproduce

  1. Go to https://jsfiddle.net/js_ziggle/zn3fLt5m/,
  2. Open the developer console,
  3. Click the buttons below the table and notice the returned values in the console.

Demo

https://jsfiddle.net/js_ziggle/zn3fLt5m/

jansiegel referenced this issue Nov 22, 2018

Fixes uncaught TypeError while selecting invalid cell #5119 #5258 #5128
… (#5381)

If the cell wasn't rendered then it would return back null and would try and remove the class of a null object which would throw an exception. Added a basic null check.
@aaronbeall

This comment has been minimized.

Copy link
Contributor

aaronbeall commented Jan 26, 2019

I've run into this and sadly its causing a lot of issues in our app.

In my app I can consistently reproduce it by:

  • Enter an invalid value into a dropdown cell (I have allowInvalid: false)
  • Press enter to commit
  • Results in JS error like #5128:
handsontable.js:1169 Uncaught TypeError: Cannot read property 'classList' of undefined
    at _removeClass (handsontable.js:1169)
    at removeClass (handsontable.js:1266)
    at handsontable.js:14716
    at done (handsontable.js:14851)
    at handsontable.js:14871
    at handsontable.js:43592
    at ColumnSettings.autocompleteValidator [as validator] (handsontable.js:43565)
    at handsontable.js:14867

img

After that point I can't close the dropdown or enter values in any cells. 😞 It happens sometimes when entering valid values as well, just not 100% of the time.

Debugging this almost all day today with another dev (not exaggeration, sadly) I found that:

  • getCell() is returning undefined which leads to the strict null check added in #5128 to let it through (null !== undefined)

    handsontable/src/core.js

    Lines 923 to 925 in c2ba2dc

    const cell = instance.getCell(cellPropertiesReference.visualRow, cellPropertiesReference.visualCol);
    if (cell !== null) {
    removeClass(cell, instance.getSettings().invalidCellClassName);

    Which leads to the JS type error in removeClass.
  • getCell() returns undefined because the call to sourceColumnToVisibleRowHeadedColumn here:
    if (TR) {
    return TR.childNodes[this.columnFilter.sourceColumnToVisibleRowHeadedColumn(column)];
    }

    returns a negative number which results in a lookup like childNodes[-1] which is undefined.
  • sourceColumnToVisibleRowHeadedColumn returns a negative number because the offset value in ColumnFilter is a larger negative number than the index, ie in this method:
    unOffsetted(index) {
    return index - this.offset;
    }

    index=3 but offset=4 (or other numbers, once saw the result as -7). I'm a little lost what those numbers are supposed to be, so I haven't gone deeper than this.

The above error could be fixed by:

  • Changing if (cell !== null) from #5128 to if (cell !== null && cell !== undefined), but that seems wrong since getCell() is not supposed to return undefined. Maybe if (cell) would be ok, but doesn't answer the fundamental question why getCell() is returning undefined.
  • Changing getCell() to account for being unable to find the element and returning another "error code" negative number, like -3, which leads to conversion to null higher up
    const td = this.wt.getCell(coords, topmost);
    if (td < 0) { // there was an exit code (cell is out of bounds)
    return null;
    and avoiding the above JS error. But I don't know what places need to then account for -3 return value, and it still doesn't answer the fundamental "why" question.

Other info:

  • I found that sourceColumnToVisibleRowHeadedColumn() returns negative numbers a lot (you see them in console in the GIF), but in many cases it never leads to a JS error, and getCell() also frequently return undefined but doesn't always result in the JS error, either.
  • I have one table where this happens frequently, and one table where it never seems to happen. They are both very similar. I've tried stripping out lots of stuff (including nestedRows which I immediately suspected) but it still happens a lot in the one table. Oddly, removing colWidths seems to fix the above JS issue and makes it happen less, but not completely gone away.
  • I haven't been able to reproduce it in a demo yet. :(
@aaronbeall

This comment has been minimized.

Copy link
Contributor

aaronbeall commented Jan 26, 2019

Alright, I have a reproducible demo. I just needed enough columns, and the error only happens with changes in fixed left columns. Here's the demo: https://jsfiddle.net/kfwg7jeL/2/

  1. Scroll far to the right
  2. Click in one of the fixed left columns
  3. Enter an invalid value and press enter

img

@AMBudnik

This comment has been minimized.

Copy link
Contributor

AMBudnik commented Jan 28, 2019

Wow, thank you for doing the investigation for you and the colleague.
You're right. In this case, dropdown is totally unusable.

@aaronbeall

This comment has been minimized.

Copy link
Contributor

aaronbeall commented Feb 4, 2019

So it seems like I came full circle to what was described in the original comment:

This situation occurs when we're trying to get a cell from a row which is rendered, but in a column which is not.

The question is: what's the correct fix? It seems that maybe the function should return -3 and -4 for column outside viewport:

    } else if (this.isColumnBeforeViewport(column)) {
      // column before viewport
      return -3;
      
    } else if (this.isColumnAfterViewport(column)) {
      // column after viewport
      return -4;
    }

And a few places may need to be updated to expect this return. Is that about right? Unless someone disagrees I can put in a PR. This would fix a nasty bug we've encountered in our app during testing!

@wojciechczerniak wojciechczerniak added this to the February 2019 milestone Feb 5, 2019

@jansiegel

This comment has been minimized.

Copy link
Member Author

jansiegel commented Feb 11, 2019

@aaronbeall Once again, thanks for the investigation!

Your -3/-4 fix seems to resolve the null/undefined problem.
In the future it would probably be best if walkontable's getCell would return an array or object with this kind of information, as:

  • negative numbers as error codes are not too intuitive, they require looking into documentation or deeper into the code,
  • after your fix, if the requested cell is placed before the rendered rows and before rendered columns, we'll only get -1 and the -3 information would be lost.

But this would require some more thought to make it consistent with other methods, making sure it works with everything that depends on getCell etc.

tl;dr: It's definitely good enough for now! Thanks a lot!

@aaronbeall

This comment has been minimized.

Copy link
Contributor

aaronbeall commented Feb 11, 2019

@jansiegel Yeah, I completely agree about the return value. My thought was to return null for out-of-bounds and an optional second object argument which will have the individual flags assigned, that way the common case (and the only one I could find in actual usage) doesn't require unwrapping a return object, but you could still get at the out-of-bounds flags if you needed to for some reason.

@AMBudnik

This comment has been minimized.

Copy link
Contributor

AMBudnik commented Mar 6, 2019

Hey @aaronbeall

I'm happy to announce that we've updated the Handsontable to 7.0.0 and fixed this issue

@AMBudnik AMBudnik closed this Mar 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.