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

feat(autocomplete): add updatePosition() method to MatAutocompleteTrigger #11495

Merged

Conversation

benelliott
Copy link
Contributor

Ensures that changes to the autocomplete options will cause the component to check that its current
overlay position is still within the viewport so that the overlay can fall back to the above
position, if necessary, while the panel is still open.

Fixes #11492

@benelliott benelliott requested a review from crisbeto as a code owner May 24, 2018 17:04
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 24, 2018
tap(() => this._positionStrategy.reapplyLastPosition()),
// Adding/removing options may mean the overlay now exceeds the viewport,
// so update its position
tap(() => this._overlayRef!.updatePosition()),
Copy link
Member

@crisbeto crisbeto May 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old behavior was deliberate (it's also the reason why we introduced reapplyLastPosition in the first place). We decided not to flip the position further, because it can be distracting for the user. E.g. if you type something, you get the dropdown on the bottom, then you type another character and the dropdown could flip to the top, then you type again and it could end up on the bottom again.

Copy link
Contributor Author

@benelliott benelliott May 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. In my use case the behaviour is undesirable because the panel is always outside the viewport (since the results are loaded dynamically after the user focuses the input). Maybe it could be an option?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general we try to avoid adding APIs for such specific use cases, because they tend to pile up and make things hard to refactor. Would making the panel flexible (having it become scrollable once it hits the viewport edge) be a potential solution?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would partially help, but still wouldn't completely fix it in some cases (e.g. when the panel height is less than the height of one option).

To be honest I don't think it's a hugely unusual use case. You can see in this demo that it's trivial to reproduce - all it requires is that the autocomplete is near the bottom of the viewport, and that the options are loaded dynamically when the user types.

Specifically it is a problem when the options are "additive", in that options are only populated once the user starts typing in the autocomplete, rather than the alternate use case of the user's input filtering a fixed list of options.

There are examples of this use case all over the web, one prominent example being the main search bar on google.com - if you were to implement this using MatAutocomplete with the component placed further down the viewport, it is likely that in the current implementation the UX would be broken.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean, but I'm not sure that either of the solutions (always updating the position or having an opt-out option) would be optimal. We may be able to do something more intuitive where we don't determine the position until the first time a set of options comes in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did consider this, but it still wouldn't really work for typeaheads where the number of results can increase past the initial count. This is actually true for my use case, where the user is tagging an entity and the list of options are (list of existing tags that match the input value) + "Create new tag: [input value]". When the user starts typing, they immediately have the "Create new tag" option, but after some time the API call completes and the relevant existing tags are added to the list of options.

When you say opt-out, do you mean that the current behaviour would be default? Because that's what I was imagining, maybe a matAutocompleteRepositionPanel option that is false by default.

Copy link

@dpalita dpalita Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do have a similar use case : my options are added dynamically. If none is in cache when the panel initially opens, it contains a "search in progress" message. This is to inform the user that this field will have assistance soon.
If the field is in the bottom of the page, it is almost always outside of the viewport since it opens with a small initial panel.
You could maybe expose a reposition() method in the matAutocomplete API so that we could trigger it programmatically (instead of a configuration option).
Better still, the position strategy could choose to change position only if the content doesn't fit in the current one : in your example were the options change back and forth with the user input it would only jump once.

I understand the need for the smallest possible API surface and the consideration of the majority where several consecutive jumps would ruin the user experience, but a piece of UI that is inaccessible because it is outside of the viewport is a bug in my opinion (you wouldn't write in the docs that an autocomplete cannot be placed less than its maximum height from the bottom of the viewport when its content changes dynamically).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@crisbeto How would you feel about a PR which instead added a reposition() method as described above?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be ok with exposing a method for triggering the reposition yourself. I'd call it updatePosition so it aligns with the OverlayRef.

@benelliott benelliott force-pushed the autocomplete-overlay-position branch from 577548c to 2263c32 Compare June 22, 2018 14:04
@benelliott benelliott changed the title fix(autocomplete): update overlay position when options are changed fix(autocomplete): add updatePosition() method to MatAutocompleteTrigger Jun 22, 2018
@benelliott benelliott changed the title fix(autocomplete): add updatePosition() method to MatAutocompleteTrigger feat(autocomplete): add updatePosition() method to MatAutocompleteTrigger Jun 22, 2018
@benelliott
Copy link
Contributor Author

Hi @crisbeto, I've changed the PR to add an updatePosition() method to MatAutocompleteTrigger instead.

The usage would be that you call updatePosition() one CD tick after updating the autocomplete options while the panel is open, if you want it to re-evaluate its position.

@@ -224,6 +224,16 @@ export class MatAutocompleteTrigger implements ControlValueAccessor, OnDestroy {
}
}

/**
* Updates the position of the autocomplete suggestion panel to ensure that its position allows
* for all suggestions to be displayed within the viewport.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"to ensure that its positions allows for all suggestions to be displayed within the viewport" -> "to ensure that it fits all options within the viewport"

fixture.detectChanges();
tick();

Promise.resolve().then(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having promises in tests either leads to unreliable tests since the callback doesn't end up being executed, or flaky tests since some browsers end up throttling them while they're minimized. You can take a look at how we've handled similar cases in the other tests. My guess is that you need a zone.simulateZoneExit() call in there.

Copy link
Contributor Author

@benelliott benelliott Jun 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@crisbeto Thanks, I actually based this test off the other one that uses AutocompleteWithOnPushDelay (L1973). That also uses a promise, but I'll try to avoid it in this spec.

…gger

Add an updatePosition() method to MatAutocompleteTrigger that re-evaluates the position of the
autocomplete panel overlay and falls back to the above position if necessary. This is useful if
enough options have been added to the panel while it is open to cause it to exceed the viewport.

Fixes angular#11492
@benelliott benelliott force-pushed the autocomplete-overlay-position branch from 2263c32 to 6286ecc Compare June 22, 2018 17:04
@benelliott
Copy link
Contributor Author

@crisbeto Added your suggestions.

Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@crisbeto crisbeto added pr: lgtm action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release labels Jun 22, 2018
@benelliott
Copy link
Contributor Author

Hi @crisbeto, it looks like my PR was actually a duplicate of your own one, which was recently merged:

#4469

That PR seems to have fixed my problem (because it was effectively doing the same thing as the initial PR), so I can close this one now unless you still want it.

@crisbeto
Copy link
Member

crisbeto commented Jul 6, 2018

Sorry, I had forgotten about that PR since it was more than a year old. I think it's fine to keep this one since it's relatively low maintenance and it can be useful.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: minor This PR is targeted for the next minor release
Projects
None yet
5 participants