fix: add and remove focused-cell part when scrolling grid #7659
Conversation
FrediWa
changed the title
FrediWa
requested review from
web-padawan and
vursen
and removed request for
web-padawan and
vursen
| @@ -855,6 +856,16 @@ export const GridMixin = (superClass) => | |||
| this._getItem(index, row); | |||
| } | |||
|
|
|||
| /** @private */ | |||
| _updateFocusedItem(row) { | |||
There was a problem hiding this comment.
| _updateFocusedItem(row) { | |
| __updateFocusedItem(row) { |
We wanted to start using double underscores for private methods.
There was a problem hiding this comment.
| const focusedItemInRow = row.__virtualIndex === this._focusedItemIndex; | ||
| let isFocusedCell; | ||
| iterateChildren(row, (cell) => { | ||
| isFocusedCell = cell === this._focusedCell; |
There was a problem hiding this comment.
I'm not sure if this comparison is safe to use with the virtual scrolling mechanism. If you scroll down and up again, is the cell in the focused row guaranteed to reuse the same cell instance that was previously stored as _focusedCell?
Maybe @tomivirkki can provide some insight?
There was a problem hiding this comment.
From some testing it seems that this is not the case:
Bildschirmaufnahme.2024-08-29.um.08.57.58.mov
There was a problem hiding this comment.
How did you do that? And did you get focus just with a click? With 2 finger scrolling, it works for me as expected and I lose focus as soon as I press the scrollbar's handle
|
|
Based on Sasha's finding, it appears that the current solution is not fully complete and will require some additional work, so I'll mark the PR as draft. |
|
Closing the PR based on @vursen 's comment. Keeping the branch for future reference when fixing the issue. |
Description
The part
focused-cellisn't affected by the virtual scrolling of the grid. This PR makes the grid update thefocused-cellwhenever items are updated.When keyboard navigating, an outline was applied to the (DOM) cell even though the focused item was scrolled out of view. As the part name is removed when scrolling, the part name could not be used to target a style
outline: none. So I addedoutline: noneto all cells (the focus ring is implemented with an inset box-shadow anyway).Fixes #7614
Type of change