fix(autocomplete): don't scroll panel when option is visible #4905
Conversation
googlebot
added
the
cla: yes
willshowell
force-pushed
the
fix/autocomplete-key-scroll
branch
from
f04e716
to
7d4f46e
Compare
|
@jelbourn could I get this assigned? 😄 |
willshowell
force-pushed
the
fix/autocomplete-key-scroll
branch
from
7d4f46e
to
b693409
Compare
|
@kara I would really appreciate if you could review this! I think it makes keyboard scrolling feel more natural. |
| @@ -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 - | |||
There was a problem hiding this comment.
Typo: if below the fold -> is below the fold
| // Scroll up to reveal selected option above the panel | ||
| if (optionOffset < panelTop) { | ||
| this.autocomplete._setScrollTop(optionOffset); | ||
| return; |
There was a problem hiding this comment.
If you used an if-else conditional, you could avoid using the early return here and on line 340. I think it might make it clearer that the two cases are mutually exclusive with fewer lines.
| const newScrollTop = | ||
| const panelTop = this.autocomplete._getScrollTop(); | ||
|
|
||
| // Scroll up to reveal selected option above the panel |
There was a problem hiding this comment.
Nit: above the panel -> scrolled above the panel top
When first reading the comment, I thought you meant literally above the panel element (not scrolled out of view).
| return; | ||
| } | ||
|
|
||
| // Scroll down to reveal selection option below the panel |
There was a problem hiding this comment.
Typo: selection option below the panel -> selected option scrolled below the panel bottom
| * 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 |
There was a problem hiding this comment.
Nit: above the top of the panel -> above the top of the visible 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 |
There was a problem hiding this comment.
Nit: visible within the panel -> visible within the panel already.
|
|
||
| // Expect no scrolling to have occurred. Still showing bottom of 6th option. | ||
| expect(scrollContainer.scrollTop) | ||
| .toEqual(32, `Expected panel to not scroll back.`); |
There was a problem hiding this comment.
Nit: The failure message is a bit vague. It seems like you should move the guidance in the comment to the failure message.
|
@kara changes made! |
kara
added
action: merge
…#4905) * fix(autocomplete): don't scroll panel if option is visible * Clean up comments & indent by 4 on line continuations
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
When scrolling down to an option well below the fold and then scrolling back up a couple of options, it feels jarring to have the selected option "stick" to the bottom of the panel. This fix keeps the panel from scrolling when the selected option is fully visible.
cc @kara
Before
After
