afcapel / stimulus-autocomplete

Stimulus autocomplete component
MIT License
482 stars 63 forks source link

Turn off scrollIntoView(false) by default. #58

Closed drnic closed 2 years ago

drnic commented 3 years ago

Opt in with data-autocomplete-scroll-into-view-align-to-top-value="false" or data-autocomplete-scroll-into-view-align-to-top-value="true"

See https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoView

Personally, I don't like the behaviour of scrollIntoView(false) and don't want it on by default for all autocomplete dropdowns.

Perhaps we can find something nicer in future? Or options?

Available as stimulus-autocomplete@2.0.1-rc.4

@phylor you ok with this? re #46

drnic commented 3 years ago

Yes data-autocomplete-scroll-into-view-align-to-top-value="false" is long. Perhaps could be data-autocomplete-scroll-align-to-top-value="false" or just data-autocomplete-align-to-top-value="false"?

phylor commented 3 years ago

Sure! May I ask for the use case though? Or more specifically, what don't you like about the scrolling behavior? I guess you don't set a fixed height, so this is more useful for lists with a small amount of results?

drnic commented 3 years ago

Yeah I have many autocompletes that start tiny

image

Here's a little video of what I see:

https://share.getcloudapp.com/Jru1yvQp

My results field has both overflow-y scrolling and max-height: (some duplicated css via tailwind

<ul data-autocomplete-target="results" tabindex="-1" class="stimulus-autocomplete max-h-60 rounded-md py-1 text-base shadow-xs overflow-x-auto overflow-y-scroll focus:outline-none sm:text-sm sm:leading-5" style: "max-height: 10rem; overflow-y: scroll;">
</ul>

I've changed the diff max height/overflow-y-scroll between tailwindcss classes max-h-60 and overflow-y-scroll and the style="..." but still see what I see in the video.

Ideas?

phylor commented 3 years ago

That is certainly bad. I would need to investigate tonight.

I'll probably look at how JQuery UI solves it. Don't remember to have seen that behavior when using them.

It is probably a good idea to have an option anyways. If the behavior is fixed and thoroughly tested, maybe we can think about changing the default again?

phylor commented 3 years ago

I think this is the relevant part from JQuery UI:

var borderTop, paddingTop, offset, scroll, elementHeight, itemHeight;
if ( this._hasScroll() ) {
    borderTop = parseFloat( $.css( this.activeMenu[ 0 ], "borderTopWidth" ) ) || 0;
    paddingTop = parseFloat( $.css( this.activeMenu[ 0 ], "paddingTop" ) ) || 0;
    offset = item.offset().top - this.activeMenu.offset().top - borderTop - paddingTop;
    scroll = this.activeMenu.scrollTop();
    elementHeight = this.activeMenu.height();
    itemHeight = item.outerHeight();

    if ( offset < 0 ) {
        this.activeMenu.scrollTop( scroll + offset );
    } else if ( offset + itemHeight > elementHeight ) {
        this.activeMenu.scrollTop( scroll + offset - elementHeight + itemHeight );
    }
}

It's a bit more elaborate than scrollIntoView and probably does not affect the scroll state of the whole page. I'll see if I can work this into a prototype tonight.

phylor commented 3 years ago

I rewrote the scrolling in https://github.com/phylor/stimulus-autocomplete/tree/fixes/scroll-into-view

This looks as below. As now only the scrollTop attribute is changed, the scroll position of the page should not change anymore.

Would you mind checking whether this fixes the issues for you? Do you find any new issues? 🙂

screengrab-20210225-2217

drnic commented 3 years ago

Damn, not working for me -- will try to figure out why.

I cut your branch as release 2.0.1-phylor-6095f2a9

Demo of the cursor scrolling out of view:

https://share.getcloudapp.com/5zuAj6Bq

My <ul> has tailwind css classes max-h-48 overflow-y-scroll

afcapel commented 3 years ago

I think it makes sense to turn it off by default. We can raise an event autocomplete.option-selected that would make it trivial for anyone to scroll to the selected option if they need to:

document.addEventListener('autocomplete.option-selected', (e) => e.target.scrollIntoView(false))

I feel it's more sustainable to provide easy extension points rather than try to handle all possible use cases inside the autocomplete code.

phylor commented 3 years ago

@afcapel I'm not sure we talk about the same thing. I believe that navigation of the search results should be a part of the library. The problem of the current behavior is that when using the keyboard navigation to scroll the search results, the correct item is selected, but the search results do not scroll into the viewport. So the user does not see which item is selected. In my opinion, this is an incorrect behavior, because keyboard navigation already is a feature of this library.

Of course, this behavior is only relevant if your viewport on your website (or the space below your select field) is smaller than the height of the search results. I believe that happens quite often though when you use autocomplete. If you don't have many search results, you probably don't need an autocomplete in the first place.

Please also note, that we tried to use scrollIntoView, but that has erratic behavior in some cases. You can find a more verbose implementation which works for me in https://github.com/phylor/stimulus-autocomplete/commit/6095f2a96b4abe9b79a8adf4ec8e5c03c689ddd5. IMHO it would be nice if we could spare others from fiddling around with that scrolling behavior.

afcapel commented 3 years ago

@phylor I see your point. The ideal solution would be something that handles scrolling but doesn't cause issues like the one Nic is seeing. I'll investigate and see if we can find a solution that works for everyone.

phylor commented 3 years ago

@phylor I see your point. The ideal solution would something that handles scrolling but doesn't cause issues like the one Nic is seeing. I'll investigate and see if we can find a solution that works for everyone.

Great, thank you very much! Let me know if I can help in any way.

jduff commented 2 years ago

FYI the changes in https://github.com/phylor/stimulus-autocomplete/commit/6095f2a96b4abe9b79a8adf4ec8e5c03c689ddd5 work great for me and would like to add a 👍 to having them merged and released.

@drnic I noticed that when I have position:relative on the results I see the behaviour from your video, but without that it works as expected.

afcapel commented 2 years ago

I'm tackling this in #91

Fix preview at https://deploy-preview-91--stimulus-autocomplete.netlify.app/long.html