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

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

Open
CSEHoangV opened this issue Sep 19, 2024 · 4 comments · May be fixed by #12517
Open

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

CSEHoangV opened this issue Sep 19, 2024 · 4 comments · May be fixed by #12517
Assignees
Labels
ng15 Angular 15 support

Comments

@CSEHoangV
Copy link

CSEHoangV commented Sep 19, 2024

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.

busyindicator_focus_switch.mp4

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;

  • Create platform table using custom loading indicator input.
  • Create a button to use fetch functionality from table and input [loading].
  • Scroll the table to certain position.
  • Click to focus on certain cell.
  • Trigger table fetch.

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.

@droshev droshev added the ng15 Angular 15 support label Sep 19, 2024
@droshev droshev added this to the Sprint 138 - September 2024 milestone Sep 19, 2024
@mikerodonnell89
Copy link
Member

mikerodonnell89 commented Sep 26, 2024

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:

  • when scrolling quickly with the scrollbar (rather than mousewheel or trackpad)
  • within the first few rows of the table
  • sometimes behaves oddly when scrolling the table with the keyboard arrow keys - though this was explicitly addressed for the scrollWholeRows feature and should be working fine there

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
Copy link
Author

CSEHoangV commented Sep 26, 2024

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.

2024-09-26_12-53-16.mp4

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

@CSEHoangV
Copy link
Author

CSEHoangV commented Sep 27, 2024

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
Copy link
Author

CSEHoangV commented Oct 7, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ng15 Angular 15 support
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants