From fbb80fcaad09ea22cb75c1a67382e98bb29ab631 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Tue, 13 Jul 2021 13:47:55 +0200 Subject: [PATCH] fix(material/select): scroll to top on last option before option group (#23147) We have some logic in `mat-autocomplete` that scrolls the list to the top when the user moves to the first option in the first option group. This is slightly better UX, because it shows the group label, rather than stopping just below it. These changes port over the same logic to `mat-select`. --- .../mdc-select/select.spec.ts | 24 +++++++++++++++++++ .../mdc-select/select.ts | 21 +++++++++++----- src/material/select/select.spec.ts | 24 +++++++++++++++++++ src/material/select/select.ts | 19 ++++++++++----- 4 files changed, 76 insertions(+), 12 deletions(-) diff --git a/src/material-experimental/mdc-select/select.spec.ts b/src/material-experimental/mdc-select/select.spec.ts index 01a7c7020d39..748f9161e32b 100644 --- a/src/material-experimental/mdc-select/select.spec.ts +++ b/src/material-experimental/mdc-select/select.spec.ts @@ -2134,6 +2134,30 @@ describe('MDC-based MatSelect', () => { expect(panel.scrollTop).toBe(520, 'Expected scroll to be at the 16th option.'); })); + it('should scroll to top when going to first option in top group', fakeAsync(() => { + fixture.destroy(); + const groupFixture = TestBed.createComponent(SelectWithGroups); + groupFixture.detectChanges(); + groupFixture.componentInstance.select.open(); + groupFixture.detectChanges(); + flush(); + + host = groupFixture.debugElement.query(By.css('mat-select'))!.nativeElement; + panel = overlayContainerElement.querySelector('.mat-mdc-select-panel')! as HTMLElement; + + for (let i = 0; i < 5; i++) { + dispatchKeyboardEvent(host, 'keydown', DOWN_ARROW); + } + + expect(panel.scrollTop).toBeGreaterThan(0); + + for (let i = 0; i < 5; i++) { + dispatchKeyboardEvent(host, 'keydown', UP_ARROW); + } + + expect(panel.scrollTop).toBe(0); + })); + }); }); diff --git a/src/material-experimental/mdc-select/select.ts b/src/material-experimental/mdc-select/select.ts index b6ee8237749b..a7dcbd654b68 100644 --- a/src/material-experimental/mdc-select/select.ts +++ b/src/material-experimental/mdc-select/select.ts @@ -23,6 +23,7 @@ import { MatOption, MAT_OPTGROUP, MAT_OPTION_PARENT_COMPONENT, + _countGroupLabelsBeforeOption, _getOptionScrollPosition, } from '@angular/material-experimental/mdc-core'; import {CdkOverlayOrigin, ConnectedPosition} from '@angular/cdk/overlay'; @@ -163,14 +164,22 @@ export class MatSelect extends _MatSelectBase implements OnInit if (option) { const panel: HTMLElement = this.panel.nativeElement; + const labelCount = _countGroupLabelsBeforeOption(index, this.options, this.optionGroups); const element = option._getHostElement(); - panel.scrollTop = _getOptionScrollPosition( - element.offsetTop, - element.offsetHeight, - panel.scrollTop, - panel.offsetHeight - ); + if (index === 0 && labelCount === 1) { + // If we've got one group label before the option and we're at the top option, + // scroll the list to the top. This is better UX than scrolling the list to the + // top of the option, because it allows the user to read the top group's label. + panel.scrollTop = 0; + } else { + panel.scrollTop = _getOptionScrollPosition( + element.offsetTop, + element.offsetHeight, + panel.scrollTop, + panel.offsetHeight + ); + } } } diff --git a/src/material/select/select.spec.ts b/src/material/select/select.spec.ts index c90b31dbdb8d..40de1a61fe95 100644 --- a/src/material/select/select.spec.ts +++ b/src/material/select/select.spec.ts @@ -2177,6 +2177,30 @@ describe('MatSelect', () => { expect(panel.scrollTop).toBe(512, 'Expected scroll to be at the 16th option.'); })); + it('should scroll to top when going to first option in top group', fakeAsync(() => { + fixture.destroy(); + const groupFixture = TestBed.createComponent(SelectWithGroups); + groupFixture.detectChanges(); + groupFixture.componentInstance.select.open(); + groupFixture.detectChanges(); + flush(); + + host = groupFixture.debugElement.query(By.css('mat-select'))!.nativeElement; + panel = overlayContainerElement.querySelector('.mat-select-panel')! as HTMLElement; + + for (let i = 0; i < 5; i++) { + dispatchKeyboardEvent(host, 'keydown', DOWN_ARROW); + } + + expect(panel.scrollTop).toBeGreaterThan(0); + + for (let i = 0; i < 5; i++) { + dispatchKeyboardEvent(host, 'keydown', UP_ARROW); + } + + expect(panel.scrollTop).toBe(0); + })); + }); }); diff --git a/src/material/select/select.ts b/src/material/select/select.ts index 838ade767bcd..ecd4a4db7916 100644 --- a/src/material/select/select.ts +++ b/src/material/select/select.ts @@ -1204,12 +1204,19 @@ export class MatSelect extends _MatSelectBase implements OnInit const labelCount = _countGroupLabelsBeforeOption(index, this.options, this.optionGroups); const itemHeight = this._getItemHeight(); - this.panel.nativeElement.scrollTop = _getOptionScrollPosition( - (index + labelCount) * itemHeight, - itemHeight, - this.panel.nativeElement.scrollTop, - SELECT_PANEL_MAX_HEIGHT - ); + if (index === 0 && labelCount === 1) { + // If we've got one group label before the option and we're at the top option, + // scroll the list to the top. This is better UX than scrolling the list to the + // top of the option, because it allows the user to read the top group's label. + this.panel.nativeElement.scrollTop = 0; + } else { + this.panel.nativeElement.scrollTop = _getOptionScrollPosition( + (index + labelCount) * itemHeight, + itemHeight, + this.panel.nativeElement.scrollTop, + SELECT_PANEL_MAX_HEIGHT + ); + } } protected _positioningSettled() {