[table] fix: vertical scroll bug when ghostCells enabled attempt 2

[mattskrobola]

Fixes #4765 (again)

Checklist

• Includes tests
• Update documentation

Changes proposed in this pull request:

Adds a parameter rectHeightCorrection to getRowIndicesInRect. This is utilized in shouldDisableVerticalScroll to account for the columnHeaderHeight when checking for which cells are visible to determine whether or not to disable vertical scrolling.

Also reverts the table change made in #5113 that attempted to fix this issue (which it did for the case when the table used the default column header) but didn't address the root cause of the problem - that header heights weren't being accounted for when determining which rows were currently visible.

Added a smoke test table in the table-app features

Reviewers should focus on:

Screenshot

before:

after:

[mattskrobola]

used || vs ?? bc this.columnHeaderHeight was inconsistently being 0 vs the actual height when running tests

[adidahiya]

ok, that's fair, but I think I'd rather not make an exception to the strict-boolean-expressions rule here (we don't actually lint for it, but I try to enforce it manually). I find the || hard to read, how about something like this instead:

Suggested change

	this.columnHeaderHeight || Grid.MIN_COLUMN_HEADER_HEIGHT,
	this.columnHeaderHeight > 0 ? this.columnHeaderHeight : Grid.MIN_COLUMN_HEADER_HEIGHT,

[adidahiya]

actually, on second thought... we never really want this.columnHeaderHeight to be 0.

we could change the ref handler to prevent that from happening, and keep the min-height logic scoped to that part of this component's code:

private refHandlers = {
        columnHeader: (ref: HTMLElement | null) => {
            this.columnHeaderElement = ref;
            if (ref != null) {
                this.columnHeaderHeight = Math.max(ref.clientHeight, Grid.MIN_COLUMN_HEADER_HEIGHT);
            }

[adidahiya]

PR preview links: table-dev-app, docs-app

[adidahiya]

functionality looks good in the preview build, just left some comments for code style

[adidahiya]

this args list is getting a little unwieldy, could we refactor it to use an options object?

also, when you do, can you document this new argument/option and leave a comment about why we need it?

[adidahiya]

ok, that's fair, but I think I'd rather not make an exception to the strict-boolean-expressions rule here (we don't actually lint for it, but I try to enforce it manually). I find the || hard to read, how about something like this instead:

Suggested change

	this.columnHeaderHeight || Grid.MIN_COLUMN_HEADER_HEIGHT,
	this.columnHeaderHeight > 0 ? this.columnHeaderHeight : Grid.MIN_COLUMN_HEADER_HEIGHT,

[adidahiya]

I came to the same conclusion a little while ago when I was playing around with this code – that the - 1 is not strictly necessary – so this change lgtm 👍🏽

[adidahiya]

actually, on second thought... we never really want this.columnHeaderHeight to be 0.

we could change the ref handler to prevent that from happening, and keep the min-height logic scoped to that part of this component's code:

private refHandlers = {
        columnHeader: (ref: HTMLElement | null) => {
            this.columnHeaderElement = ref;
            if (ref != null) {
                this.columnHeaderHeight = Math.max(ref.clientHeight, Grid.MIN_COLUMN_HEADER_HEIGHT);
            }

[adidahiya]

I think naming this columHeaderHeight would be simpler & more explicit

[mattskrobola]

Makes sense, there are some other functions using getRowIndicesInRect like invokeOnVisibleCellsChangeCallback that aren't using this arg but maybe they should be? since a row is technically not visible after it goes behind the header column but we still consider it visible since it's in the rect

[adidahiya]

yeah, I guess invokeOnVisibleCellsChangeCallback should know about columnHeaderHeight as well...

if we're finding that the Grid needs to know the column header height so often, then maybe we should try to pass it columnHeaderHeight from Table / Table2 when we construct/validate the grid? hopefully this value won't get out of sync 🤞🏽, and that change might reduce some code complexity. before you do this, however, can you verify that invokeOnVisibleCellsChangeCallback is buggy right now? i.e. that it returns the wrong range of indices when the column header is larger than the default height.

[mattskrobola]

It's hard to test the accuracy of invokeOnVisibleCellsChangeCallback since its using a bleed buffer on both sides when ghost cells are enabled, but it does seem to be a bit off without accounting for the header height.
without accounting for header height:

with accounting for header height:

[adidahiya]

It still seems off after accounting for header height... the "start" should be closer to 5 or 6 when you scroll all the way down, no? we can address this in a separate PR, but it looks like it's not a high priority for this particular callback since we haven't gotten any bug reports for onVisibleCellsChange accuracy

[adidahiya]

@mattskrobola please try to avoid force-pushing; I like to see all the changes in a PR branch, and sometimes a force-push will make the CI build confused (it got canceled and had to be manually re-triggered here)