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

[BUG] When using down or up arrow, the mat-select scrollTop isn't calculated correctly #119

Closed
Mabiro opened this issue Mar 14, 2019 · 7 comments · Fixed by #120
Closed
Labels
bug Something isn't working minor

Comments

@Mabiro
Copy link

Mabiro commented Mar 14, 2019

Thanks for this component!

I realize this might not be possible to fix without a dirty hack like I provided in the "Additionnal Context" section, but I thought maybe you'd be able to give a better approach.

Describe the bug
It looks like the height of the ngx-filter and the fact that it is above the other mat-options makes it so the mat-select can't calculate correctly where the scrolltop of its panel should be.

When using the component inside a mat-option, the issue is when going "up" with the up arrow.
When using the component outside of a mat-option, the issue is when going "down" with the down arrow.

To Reproduce

  1. Go to the demo https://stackblitz.com/github/bithost-gmbh/ngx-mat-select-search-example?file=src%2Fapp%2Fapp.component.html
  2. Open the single selection example
  3. Go to the bottom of the list using the down arrow
  4. Go back to the top of the list with the up arrow
  5. You should see that once the scroll bar starts going up, the active item is not visible, and that the first option is not visible. (It requires a manual scroll up)

The same can be observed with the component outside of a mat-option, but it's the other way around.

Expected behavior
When navigating up or down, the active item should correctly be displayed in the mat-select panel.

Screenshots
image

Desktop (please complete the following information):

  • OS: Win10
  • Browser Chrome
  • Version Latest

Additional context
I was able to "fix" the issue by overriding private methods of MatSelect, but I would rather stay away from this:

this.matSelectElement['_scrollActiveOptionIntoView'] = (): void => {
            let activeOptionIndex = this.matSelectElement['_keyManager'].activeItemIndex || 0;
            let labelCount = _countGroupLabelsBeforeOption(activeOptionIndex, this.matSelectElement.options, this.matSelectElement.optionGroups);

            this.matSelectElement.panel.nativeElement.scrollTop = _getOptionScrollPosition(activeOptionIndex + labelCount,
                this.matSelectElement['_getItemHeight'](),
                this.matSelectElement.panel.nativeElement.scrollTop,
                SELECT_PANEL_MAX_HEIGHT - 51
            );
        };

It's the same implementation, but the height of the filter is substracted from "max height". I tested it with my ngx-search outside of the mat-option, but not inside. So it might not work for both.. maybe its +51 in that case? not sure..

@Mabiro Mabiro added the bug Something isn't working label Mar 14, 2019
@macjohnny
Copy link
Member

@Mabiro thanks for reporting this issue. it seems this is not so easy to fix, and as you stated, overwriting the _scrollActiveOptionIntoView method is not a sustainable option.
However, I will see if we can find a better way to achieve this.

@macjohnny
Copy link
Member

one way to achieve this could be something similar to
https://github.com/angular/material2/blob/876727dbfa5b4a4bf43edded91c6ee84eb2db7e1/src/lib/select/select.ts#L905-L907

where we listen for keymanager changes and if the option is covered by the search input, shift the scrolltop

@macjohnny
Copy link
Member

@Mabiro would you like to try to implement the fix I suggest?

@Mabiro
Copy link
Author

Mabiro commented Mar 18, 2019

Sure, I'll take a look at this. I am not sure how to determine whether the option is covered by the search input in clean manner, but I'll give it a shot.

Thanks!

@macjohnny
Copy link
Member

macjohnny commented Mar 18, 2019

"covered" would be if scrollTop == 51 * option index or something like that

@Mabiro
Copy link
Author

Mabiro commented Mar 18, 2019

@macjohnny : Gave it a try, let me know what you think! #120

macjohnny added a commit that referenced this issue Mar 21, 2019
…er-search-input

#119 - Issue with scrollTop and keyboard selection
@macjohnny
Copy link
Member

this is release in 1.7.0. thanks @Mabiro for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working minor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants