From b6934097fb831bb12b7dd6812b9d26786d786b28 Mon Sep 17 00:00:00 2001 From: Will Howell Date: Thu, 1 Jun 2017 09:03:31 -0400 Subject: [PATCH 1/2] fix(autocomplete): don't scroll panel if option is visible --- src/lib/autocomplete/autocomplete-trigger.ts | 23 ++++++-- src/lib/autocomplete/autocomplete.spec.ts | 62 +++++++++++++++++++- src/lib/autocomplete/autocomplete.ts | 9 ++- 3 files changed, 86 insertions(+), 8 deletions(-) diff --git a/src/lib/autocomplete/autocomplete-trigger.ts b/src/lib/autocomplete/autocomplete-trigger.ts index 86cdbfbadd9c..6ec309ecbd40 100644 --- a/src/lib/autocomplete/autocomplete-trigger.ts +++ b/src/lib/autocomplete/autocomplete-trigger.ts @@ -315,15 +315,30 @@ export class MdAutocompleteTrigger implements ControlValueAccessor, OnDestroy { /** * Given that we are not actually focusing active options, we must manually adjust scroll * to reveal options below the fold. First, we find the offset of the option from the top - * of the panel. The new scrollTop will be that offset - the panel height + the option - * height, so the active option will be just visible at the bottom of the panel. + * of the panel. If that offset if below the fold, the new scrollTop will be the offset - + * the panel height + the option height, so the active option will be just visible at the + * bottom of the panel. If that offset is above the top of the panel, the new scrollTop + * will become the offset. If that offset is visible within the panel, the scrollTop is not + * adjusted. */ private _scrollToOption(): void { const optionOffset = this.autocomplete._keyManager.activeItemIndex ? this.autocomplete._keyManager.activeItemIndex * AUTOCOMPLETE_OPTION_HEIGHT : 0; - const newScrollTop = + const panelTop = this.autocomplete._getScrollTop(); + + // Scroll up to reveal selected option above the panel + if (optionOffset < panelTop) { + this.autocomplete._setScrollTop(optionOffset); + return; + } + + // Scroll down to reveal selection option below the panel + if (optionOffset + AUTOCOMPLETE_OPTION_HEIGHT > panelTop + AUTOCOMPLETE_PANEL_HEIGHT) { + const newScrollTop = Math.max(0, optionOffset - AUTOCOMPLETE_PANEL_HEIGHT + AUTOCOMPLETE_OPTION_HEIGHT); - this.autocomplete._setScrollTop(newScrollTop); + this.autocomplete._setScrollTop(newScrollTop); + return; + } } /** diff --git a/src/lib/autocomplete/autocomplete.spec.ts b/src/lib/autocomplete/autocomplete.spec.ts index 422456f40bd4..124dbf2313ad 100644 --- a/src/lib/autocomplete/autocomplete.spec.ts +++ b/src/lib/autocomplete/autocomplete.spec.ts @@ -534,6 +534,7 @@ describe('MdAutocomplete', () => { let fixture: ComponentFixture; let input: HTMLInputElement; let DOWN_ARROW_EVENT: KeyboardEvent; + let UP_ARROW_EVENT: KeyboardEvent; let ENTER_EVENT: KeyboardEvent; beforeEach(() => { @@ -542,6 +543,7 @@ describe('MdAutocomplete', () => { input = fixture.debugElement.query(By.css('input')).nativeElement; DOWN_ARROW_EVENT = createKeyboardEvent('keydown', DOWN_ARROW); + UP_ARROW_EVENT = createKeyboardEvent('keydown', UP_ARROW); ENTER_EVENT = createKeyboardEvent('keydown', ENTER); fixture.componentInstance.trigger.openPanel(); @@ -600,7 +602,6 @@ describe('MdAutocomplete', () => { const optionEls = overlayContainerElement.querySelectorAll('md-option') as NodeListOf; - const UP_ARROW_EVENT = createKeyboardEvent('keydown', UP_ARROW); fixture.componentInstance.trigger._handleKeydown(UP_ARROW_EVENT); tick(); fixture.detectChanges(); @@ -754,7 +755,6 @@ describe('MdAutocomplete', () => { const scrollContainer = document.querySelector('.cdk-overlay-pane .mat-autocomplete-panel')!; - const UP_ARROW_EVENT = createKeyboardEvent('keydown', UP_ARROW); fixture.componentInstance.trigger._handleKeydown(UP_ARROW_EVENT); tick(); fixture.detectChanges(); @@ -763,6 +763,64 @@ describe('MdAutocomplete', () => { expect(scrollContainer.scrollTop).toEqual(272, `Expected panel to reveal last option.`); })); + it('should not scroll to active options that are fully in the panel', fakeAsync(() => { + tick(); + const scrollContainer = + document.querySelector('.cdk-overlay-pane .mat-autocomplete-panel')!; + + fixture.componentInstance.trigger._handleKeydown(DOWN_ARROW_EVENT); + tick(); + fixture.detectChanges(); + expect(scrollContainer.scrollTop).toEqual(0, `Expected panel not to scroll.`); + + // These down arrows will set the 6th option active, below the fold. + [1, 2, 3, 4, 5].forEach(() => { + fixture.componentInstance.trigger._handleKeydown(DOWN_ARROW_EVENT); + tick(); + }); + + // Expect option bottom minus the panel height (288 - 256 = 32) + expect(scrollContainer.scrollTop) + .toEqual(32, `Expected panel to reveal the sixth option.`); + + // These up arrows will set the 2nd option active + [4, 3, 2, 1].forEach(() => { + fixture.componentInstance.trigger._handleKeydown(UP_ARROW_EVENT); + tick(); + }); + + // Expect no scrolling to have occurred. Still showing bottom of 6th option. + expect(scrollContainer.scrollTop) + .toEqual(32, `Expected panel to not scroll back.`); + })); + + it('should scroll to active options that are above the panel', fakeAsync(() => { + tick(); + const scrollContainer = + document.querySelector('.cdk-overlay-pane .mat-autocomplete-panel')!; + + fixture.componentInstance.trigger._handleKeydown(DOWN_ARROW_EVENT); + tick(); + fixture.detectChanges(); + expect(scrollContainer.scrollTop).toEqual(0, `Expected panel not to scroll.`); + + // These down arrows will set the 7th option active, below the fold. + [1, 2, 3, 4, 5, 6].forEach(() => { + fixture.componentInstance.trigger._handleKeydown(DOWN_ARROW_EVENT); + tick(); + }); + + // These up arrows will set the 2nd option active + [5, 4, 3, 2, 1].forEach(() => { + fixture.componentInstance.trigger._handleKeydown(UP_ARROW_EVENT); + tick(); + }); + + // Expect to show the top of the 2nd option at the top of the panel + expect(scrollContainer.scrollTop) + .toEqual(48, `Expected panel to scroll up when option is above panel.`); + })); + it('should close the panel when pressing escape', async(() => { const trigger = fixture.componentInstance.trigger; const escapeEvent = createKeyboardEvent('keydown', ESCAPE); diff --git a/src/lib/autocomplete/autocomplete.ts b/src/lib/autocomplete/autocomplete.ts index 081682eba353..03939f6b7b53 100644 --- a/src/lib/autocomplete/autocomplete.ts +++ b/src/lib/autocomplete/autocomplete.ts @@ -73,8 +73,8 @@ export class MdAutocomplete implements AfterContentInit { } /** - * Sets the panel scrollTop. This allows us to manually scroll to display - * options below the fold, as they are not actually being focused when active. + * Sets the panel scrollTop. This allows us to manually scroll to display options + * above or below the fold, as they are not actually being focused when active. */ _setScrollTop(scrollTop: number): void { if (this.panel) { @@ -82,6 +82,11 @@ export class MdAutocomplete implements AfterContentInit { } } + /** Returns the panel's scrollTop. */ + _getScrollTop(): number { + return this.panel ? this.panel.nativeElement.scrollTop : 0; + } + /** Panel should hide itself when the option list is empty. */ _setVisibility() { Promise.resolve().then(() => { From 2d0a04adf237d074827eefaf9408dff7a42a0e60 Mon Sep 17 00:00:00 2001 From: Will Howell Date: Mon, 26 Jun 2017 21:52:20 -0400 Subject: [PATCH 2/2] Clean up comments & indent by 4 on line continuations --- src/lib/autocomplete/autocomplete-trigger.ts | 20 ++++++++------------ src/lib/autocomplete/autocomplete.spec.ts | 6 +++--- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/src/lib/autocomplete/autocomplete-trigger.ts b/src/lib/autocomplete/autocomplete-trigger.ts index 6ec309ecbd40..a928fe461226 100644 --- a/src/lib/autocomplete/autocomplete-trigger.ts +++ b/src/lib/autocomplete/autocomplete-trigger.ts @@ -315,29 +315,25 @@ export class MdAutocompleteTrigger implements ControlValueAccessor, OnDestroy { /** * Given that we are not actually focusing active options, we must manually adjust scroll * to reveal options below the fold. First, we find the offset of the option from the top - * of the panel. If that offset if below the fold, the new scrollTop will be the offset - + * of the panel. If that offset is below the fold, the new scrollTop will be the offset - * the panel height + the option height, so the active option will be just visible at the - * bottom of the panel. If that offset is above the top of the panel, the new scrollTop - * will become the offset. If that offset is visible within the panel, the scrollTop is not - * adjusted. + * bottom of the panel. If that offset is above the top of the visible panel, the new scrollTop + * will become the offset. If that offset is visible within the panel already, the scrollTop is + * not adjusted. */ private _scrollToOption(): void { const optionOffset = this.autocomplete._keyManager.activeItemIndex ? this.autocomplete._keyManager.activeItemIndex * AUTOCOMPLETE_OPTION_HEIGHT : 0; const panelTop = this.autocomplete._getScrollTop(); - // Scroll up to reveal selected option above the panel if (optionOffset < panelTop) { + // Scroll up to reveal selected option scrolled above the panel top this.autocomplete._setScrollTop(optionOffset); - return; - } - - // Scroll down to reveal selection option below the panel - if (optionOffset + AUTOCOMPLETE_OPTION_HEIGHT > panelTop + AUTOCOMPLETE_PANEL_HEIGHT) { + } else if (optionOffset + AUTOCOMPLETE_OPTION_HEIGHT > panelTop + AUTOCOMPLETE_PANEL_HEIGHT) { + // Scroll down to reveal selected option scrolled below the panel bottom const newScrollTop = - Math.max(0, optionOffset - AUTOCOMPLETE_PANEL_HEIGHT + AUTOCOMPLETE_OPTION_HEIGHT); + Math.max(0, optionOffset - AUTOCOMPLETE_PANEL_HEIGHT + AUTOCOMPLETE_OPTION_HEIGHT); this.autocomplete._setScrollTop(newScrollTop); - return; } } diff --git a/src/lib/autocomplete/autocomplete.spec.ts b/src/lib/autocomplete/autocomplete.spec.ts index 124dbf2313ad..32fd0846b511 100644 --- a/src/lib/autocomplete/autocomplete.spec.ts +++ b/src/lib/autocomplete/autocomplete.spec.ts @@ -781,7 +781,7 @@ describe('MdAutocomplete', () => { // Expect option bottom minus the panel height (288 - 256 = 32) expect(scrollContainer.scrollTop) - .toEqual(32, `Expected panel to reveal the sixth option.`); + .toEqual(32, `Expected panel to reveal the sixth option.`); // These up arrows will set the 2nd option active [4, 3, 2, 1].forEach(() => { @@ -791,7 +791,7 @@ describe('MdAutocomplete', () => { // Expect no scrolling to have occurred. Still showing bottom of 6th option. expect(scrollContainer.scrollTop) - .toEqual(32, `Expected panel to not scroll back.`); + .toEqual(32, `Expected panel not to scroll up since sixth option still fully visible.`); })); it('should scroll to active options that are above the panel', fakeAsync(() => { @@ -818,7 +818,7 @@ describe('MdAutocomplete', () => { // Expect to show the top of the 2nd option at the top of the panel expect(scrollContainer.scrollTop) - .toEqual(48, `Expected panel to scroll up when option is above panel.`); + .toEqual(48, `Expected panel to scroll up when option is above panel.`); })); it('should close the panel when pressing escape', async(() => {