SAP / fundamental-ngx

Fundamental Library for Angular is SAP Design System Angular component library
https://sap.github.io/fundamental-ngx
Apache License 2.0
269 stars 130 forks source link

[P2][Sourcing] Platform table switch focus with custom indicator #12450

Closed CSEHoangV closed 4 weeks ago

CSEHoangV commented 1 month ago

Is this a bug, enhancement, or feature request?

bug

Describe your proposal.

When we use custom busy indicator through [loading]="!!(loading | async)" and we want to fetch the table, the content switch focus to different position (or table is scrolled). If we remove this, there is no issue.

https://github.com/user-attachments/assets/61b13582-a285-4c6f-b47d-3d78cbeafeb0

Expectation: when using [loading] and table is fetched, position should stay in place.

Can you handle that on the application side

No

Which versions of Angular and Fundamental Library for Angular are affected? Please, specify the exact version. (If this is a feature request, use current version.)

Angular 15 0.43.43

If this is a bug, please provide steps for reproducing it; the exact components you are using;

Please provide relevant source code (if applicable).

https://stackblitz.com/edit/angular-l9ndgq?file=src%2Fapp%2Fplatform-table-virtual-scroll-example.component.html

Please provide stackblitz example(s).

https://stackblitz.com/edit/angular-l9ndgq?file=src%2Fapp%2Fplatform-table-virtual-scroll-example.component.html

In case this is Accessibility related topic, did you consult with an accessibility expert? If not, please do so and share their recommendations.

Did you check the documentation and the API?

Yes

Did you search for similar issues?

Yes

Is there anything else we should know?

This is important for us to use custom indicator to control when to show indicator especially which we involve API call.

IMPORTANT: Please refrain from providing links or screenshots of SAP's internal information, as this project is open-source, and its contents are accessible to anyone.

mikerodonnell89 commented 1 month ago

This doesn't have anything to do with the custom busy indicator (in the video below I've removed [loading]="!!(loading | async)" and you see the same behavior, it's not happening in your video after removing that line because your fetch does not shift the scroll position of the table). I've gone through some old messages with the UX and accessibility designers around the time we were working on #11491 and it is intentional to keep focus at the previously focused row/column index as the user scrolls the table or the table data changes via fetch, search, filter etc. This is done because it is important to keep focus on a cell within the visible pane of the table. However, on our implementation, it looks like this doesn't work in all cases, including:

I think it is a happy accident for us, that it is working as the designers intended in some cases - likely just a side effect of our virtual scrolling implementation

CSEHoangV commented 1 month ago

This doesn't have anything to do with the custom busy indicator (in the video below I've removed [loading]="!!(loading | async)" and you see the same behavior, it's not happening in your video after removing that line because your fetch does not shift the scroll position of the table).

Hi @mikerodonnell89, I tried to have that [loading] removed and try scroll and fetch multiple times and the position is not shifted even with virtualScroll. Every fetch keeps the same scroll position with internal busy indicator.

https://github.com/user-attachments/assets/ea118530-52f7-42cc-8371-22f1e827e181

The thing is that we constantly observe different result between with and without [loading].

CSEHoangV commented 1 month ago

I found another workaround: Instead of [loading]="!!(loading | async)", I set [loading]="((loading | async) === true) ? true : undefined" and it seems working. The reason for the scroll is from:

Screenshot 2024-09-27 at 10 19 32 AM

When it gets the top from busy indicator rather than the table to do the scroll. It is not supposed to go there since there is a check in the table to do the focus only when loadingState is false, so the root cause comes from the nullish check (??) on loading state:

get loadingState(): boolean {
        return (
            this.loading ??
            (this._dataSourceDirective._internalLoadingState ||
                this._dataSourceDirective._internalChildrenLoadingState ||
                this._dndLoadingState)
        );
    }

The ?? only checks null/undefined so if it has value (even false), it will not consider the second statement and return false and run the focus in setTimeOut with the busy indicator already popped up . This check considers internal loading state but with [loading] it will change this.loading value from table ngOnChanges and becomes problematic since we have multiple sources to check validate loadingState. Since the workaround works, I will close this. But if you have time please refine this logic.

CSEHoangV commented 1 month ago

The workaround is only reduce the timing this issue occurring but it is still triggered depending on when we start triggering busy indicator. It is due to the listener to execute refocus:

private _listenToLoadingAndRefocusCell(): void {
        this._subscriptions.add(
            this._tableService.tableLoading$.pipe(filter((loadingState) => !loadingState)).subscribe(() => {
                setTimeout(() => {
                    if (this._tableHeaderResizer.focusedCellPosition && this._focusableGrid) {
                        this._focusableGrid.focusCell(this._tableHeaderResizer.focusedCellPosition);
                    }
                });
            })
        );
    }

this._focusableGrid.focusCell(this._tableHeaderResizer.focusedCellPosition); should be run only when loadingState is false. However, since it is asynchronously run in setTimeout, before focusCell is triggered, there is a chance that loadingState is set to true (for example, from [loading] in this case while table loading gets triggered by dataSource through fetch) and scrollInToView will get top from busy indicator and gives a wrong position.

I think this should be addressed from library instead of workaround given random async run in this case and it is tedious for application to apply workaround and be mindful when to trigger [loading] or modify data source to prevent this issue.

I will provide a fix on this.