[DataGrid] Avoid trying to scroll right when focusing a cell in the last column if 100% in viewport

[flaviendelangle]

Part of #2103 (followed by #2110)

[dtassone]

Not completely fixed.

That said, I'm not sure about the way you fix this issue. I think it is due to a scrollbar width being mistakenly added on the column header.

[flaviendelangle]

@dtassone for me the issue was that renderedColState.rightEmptyWidth === 1 in the example given in #2013 and 3 in the example given in #2011 because of the Math.floor in the flex column width computation.

So we add a GridEmptyCell at the right of the table in GridColumnHeader, which has a 10px padding.
Meaning that we add 20px - 1px => 19px to the header.

My fix reduces the renderedColState.rightEmptyWidth to zero in both examples so their is no GridEmptyCell on the right anymore.

With that being said, if their are valid cases where we want to show these empty cells with more than 0px of width but less that 20px of width, then we should edit their padding because the current behavior do not handle it correctly.

If we just remove the padding, it would be quite clean for #2013 but not on #2011 because of the border on the cells.

But I have probably broken something else on the table you screen, I'll check

EDIT : I'm not reproducing your screenshot. Could you add a video and the link please ?
On my computer the cell is correctly aligned with the header when resizing the 2nd column.

[dtassone]

The fix seems a bit of a hack and doesn't solve the calculation issue. We have an extra 11px on the scrollWidth of the column headers that should not be there.

[m4theushw]

I did a small investigation about this bug in #2103 (comment). I think we can take a different direction to fix it, because the problem is not the empty cell. Not that we shouldn't remove the empty cell, but the problems is not it. 😛

[flaviendelangle]

OK so from what I understand there are several issues here.

I think that Grid with at least 1 flex column should always have a total column width a least equal to the viewport width. Unless there is a better solution to avoid this gap problem : #2011 (comment)
To do so, we can remove the Math.floor but I think it's not great to have non-integer column width.

---

I did reproduce your video @dtassone.
And indeed my conclusion on the empty cell is wrong, I removed its padding and I still have the same behavior.
Do we agree that removing it's padding is still something we want to do though ?

---

I'll implement @m4theushw 's fix from #2103 (comment)

[flaviendelangle]

@m4theushw do you see a difference here https://github.com/mui-org/material-ui-x/pull/2104/files#diff-1c5bf15ff6f5b30287003a45d20a597797db104b7edab8d778f00e96f2dc06c1R97

Between :

if (
    lastColIndex < positions.length - 1 ||
    (positions[positions.length - 1] && totalWidth > availableWidth)
) {
    lastColIndex -= 1; // Last fully visible column
}

and

const lastColumnRightPosition = positions[lastColIndex] + visibleColumns[lastColIndex].width
if (
    lastColIndex < positions.length - 1 ||
    (positions[positions.length - 1] && lastColumnRightPosition > availableWidth)
) {
    lastColIndex -= 1; // Last fully visible column
}

For me they are equivalent (and the 1st is lighter) but I'm not sure.

I did the 1st one and it fixes the video above

[m4theushw]

@m4theushw do you see a difference here https://github.com/mui-org/material-ui-x/pull/2104/files#diff-1c5bf15ff6f5b30287003a45d20a597797db104b7edab8d778f00e96f2dc06c1R97

Yeah, they seem to be equivalent. I think the solution is around improving this while-loop. This last decrement we do doesn't seem right. But I'm OK if we need an extra logic checking if we're at the last column.

https://github.com/mui-org/material-ui-x/blob/2a1aeef4f3d5d5da473e4c3120c49df19657e867/packages/grid/_modules_/grid/hooks/features/virtualization/useGridVirtualColumns.ts#L90-L94

[flaviendelangle]

@m4theushw I reworked the while loop to make it more explicit.

[oliviertassinari]

I have done a quick exploration in #2162 for another issue. It seems to solve this one even if it wasn't the intent. Two birds 🕊 with one stone 🗿.

[oliviertassinari]

It seems to solve this one even if it wasn't the intent. Two birds 🕊 with one stone 🗿.

I was wrong; I have posted a new comment on why in #2103