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 Report][3.3.3] VInfiniteScroll does not set root element correctly unless margin prop is explicitly supplied #17583

Closed
eldjh3 opened this issue Jun 12, 2023 · 9 comments · Fixed by #17870 · May be fixed by jonathanestefani/crud_vuetify#2, jonathanestefani/crud_vuetify#5 or aurelienfvre/sae401#1
Assignees
Labels
C: VInfiniteScroll T: bug Functionality that does not work as intended/expected
Milestone

Comments

@eldjh3
Copy link

eldjh3 commented Jun 12, 2023

Environment

Vuetify Version: 3.3.3
Vue Version: 3.3.4
Browsers: Chrome 114.0.0.0
OS: Mac OS 10.15.7

Steps to reproduce

Use a v-infinite-scroll element without the margin prop. The root is not passed to the IntersectionOberserver which therefore uses the viewport as the root for intersection calculations.

More details

In the linked playground run as-is. It will use the viewport for the root element. It will only load one extra set of data (3 more items) [and get stuck - as per issue 17358]. Resize the viewport such that the intersection div leaves and re-enters the viewport. The infinite scroll will be triggered to load more data.
Comment/uncomment the two v-infinite-scroll tags in the playground. Behaviour changes such that the wrapper div is now correctly the root element. But, no amount of scrolling will now get more data to load.

Expected Behavior

Whether the margin prop is specified or not, the v-infinite-scroll wrapper div is used for the root element.

Actual Behavior

With no explicit margin prop the IntersectionObserver uses the viewport as the root for intersection calculations.

Reproduction Link

https://play.vuetifyjs.com/#...

Other comments

Rather than defaulting to undefined, margin should probably default to zero.

Interestingly, this issue allows a workaround for #17358 as the browser can be resized continually to move the intersection div in and out of the viewport until such time as the loaded content fills the container and naturally pushes the intersection div out of the viewport. At this time the behaviour returns to expected and new content can be loaded simply by scrolling.

Thus, fixing this issue before that issue will prevent that workaround.

@KaelWD
Copy link
Member

KaelWD commented Jun 14, 2023

I feel like it makes more sense to always use the viewport as the root, never the actual root element. For example if you don't specify a height it works as expected: Playground
If you enable margin then root becomes rootEl and it no longer works at all.

@eldjh3
Copy link
Author

eldjh3 commented Jun 14, 2023

If you enable margin then root becomes rootEl and it no longer works at all.

By this do you mean it gets stuck after the first load cycle? Or something else? If yes then that's the issue in #17358.

Thinking about always using the viewport for the root, what then happens if you have a relatively small infinitely scrollable area within the viewport and you do set the height explicitly? If the viewport is used as the root then the intersection will never leave the viewport, no?

Or does the intersection observer get notified when the intersection div gets 'unobscured' even if it was always within the browser window/viewport? Replying quickly, I haven't had a chance to experiment further myself.

@KaelWD
Copy link
Member

KaelWD commented Jun 14, 2023

By this do you mean it gets stuck after the first load cycle

Specifically in the playground I posted, which loads enough items to avoid #17358
Actually I think if #17358 is fixed then that playground would keep loading items forever with root = rootEl, because the intersection div is always intersecting rootEl if there's no height specified.

even if it was always within the browser window/viewport

Yes, it takes into account other scrolling elements between root and target. You can see this on https://vuetifyjs.com/en/components/infinite-scroller/, none of the examples have margin so root = null

@eldjh3
Copy link
Author

eldjh3 commented Jun 15, 2023

Interesting. I was under the (seemingly incorrect) assumption that IntersectionObserver wouldn't notify if the intersection target remained within the viewport regardless of visibility of the target element. I haven't seen that documented in the MDN docs other than some fairly vague references to 'visible' when discussing bounding/intersection rectangles. Empirically it definitely does appear to be the case. If this behaviour of IntersectionObserver is documented explicitly anywhere I'd be interested to read it.

Also interesting that you say none of the examples at https://vuetifyjs.com/en/components/infinite-scroller/ have the root set, but all work as one might expect. Again, empirically that is the case. In my digging I came across https://blog.webdevsimplified.com/2022-01/intersection-observer/ which has some similar examples for observing content as it scrolls and the author states he had to specify the root in order to get them working correctly. Quote follows, unfortunately he doesn't go into more detail as to what the issues were without the root set.

In order to make all the examples on this page work I actually had to use the root property to set the scrolling container as the root element since otherwise the observer would not work correctly.

Conceptually it feels to me that the rootEl should be the closest ancestor element that has a size restriction and scroll behaviour. But if it is workable always using the viewport then fair enough.

It seems there are a few different use cases in play.

  1. No height specified on the v-infinite-scroll. Visible content restricted only by the viewport. The infinite scroller can grow infinitely (funny that 😆). The infinite scroller itself isn't then really a scrolling element anymore (even if it has overflow: scroll/auto it will never kick in), it's the entire viewport that's scrolling. The v-infinite-scroll is just the trigger/handler to load more content. The root element in this case should conceptually be the viewport.

  2. No height specified on the v-infinite-scroll. Visible content restricted by some parent component with a size restriction and scroll behaviour. Again, the infinite scroller itself isn't a scrolling element, just the trigger mechanism. The root element should conceptually be that parent component.

  3. Height is specified on the v-infinite-scroll and it restricts the visible content. It acts as the scroll container and the trigger for loading. The root element should conceptually be the wrapper div of the v-infinite-scroll.

What would be really confusing and a horrible UX would be if you end up in a situation with scrollbars on both the viewport and the v-infinite-scroller (or some parent of it) and you need to use some combination of scrolling to get it to respond.

All that said, my initial concern was the inconsistency in the configuration, in that the root element was only specified if margin is also set. If consistent behaviour can be obtained using the viewport as the root always then that seems ok.

@eldjh3
Copy link
Author

eldjh3 commented Jun 15, 2023

I guess to put it another way, if the intersection is always calculated taking into account clipping of the target element w.r.t. any of its parent elements between the target and the root then using the viewport as root should always behave correctly.

I've answered my own question re documentation and am reading the w3c spec. Comment near the start.

...intersectionRect... // boundingClientRect, clipped by its containing block ancestors, and intersected with rootBounds

In which case maybe the only reason to specify a root element is for efficiency to avoid performing as many clipping/bounds calculations if it's a complex layout. 🤷‍♂️

@KaelWD
Copy link
Member

KaelWD commented Jun 15, 2023

Even the MDN example responds to both its container and the window and that's in an iframe.

What would be really confusing and a horrible UX would be if you end up in a situation with scrollbars on both the viewport and the v-infinite-scroller (or some parent of it) and you need to use some combination of scrolling to get it to respond.

That's maybe correct behaviour as you wouldn't actually be able to see the bottom of the list anyway. We could work around it by adding observers for both rootEl and window if rootEl is scrollable but having two scrollbars like that is horrible UX in the first place so it's kinda your fault if it ends up broken.

@eldjh3
Copy link
Author

eldjh3 commented Jun 15, 2023

Even the MDN example responds to both its container and the window and that's in an iframe.

Good point, I hadn't tried playing with that example with the container half outside the viewport. It would have been patently obvious very quickly that my assumption was incorrect as it updates regardless of which you scroll.

Long story short, I agree with your initial comment that using the viewport as the root should be fine in all cases.

but having two scrollbars like that is horrible UX in the first place so it's kinda your fault if it ends up broken.

Completely agree. Maybe something could be added to the docs to indicate it's suggested to either set an explicit height, or make the scroller itself non-scrollable to avoid it. Meh. Not sure if that adds any value.

@yuwu9145
Copy link
Member

Actually I think if #17358 is fixed then that playground would keep loading items forever with root = rootEl, because the intersection div is always intersecting rootEl if there's no height specified.

#17358 is fixed and this is indeed happening now: demo

@johnleider johnleider assigned yuwu9145 and unassigned KaelWD Aug 1, 2023
@johnleider johnleider added T: bug Functionality that does not work as intended/expected C: VInfiniteScroll and removed S: triage labels Aug 1, 2023
@johnleider johnleider added this to the v3.3.x milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment