Skip to content
This repository has been archived by the owner on Oct 1, 2021. It is now read-only.

compute <virtual-list> scrollTarget #31

Closed
wants to merge 10 commits into from

Conversation

valdrinkoshi
Copy link
Collaborator

@valdrinkoshi valdrinkoshi commented Apr 25, 2018

Fixes #23.

Introduce scrollTarget property in RepeatsAndScrolls, defaults to window:

  • if scrollTarget === container, we size the container via a sizer element, which is inserted in the container and positioned to give the container the correct size and make it scrollable (see example, inspired from https://developers.google.com/web/updates/2016/07/infinite-scroller). For it to work, it requires the container to have its own stacking context (that's already the case since the stamped children need to be positioned absolutely according to the container)
  • otherwise, container is sized via min-width/height (as it was done before).

This allows <virtual-list> to be the scrolling element, or to be contained in a scrolling element other than the main document.

<!-- scrolls within the given size -->
<virtual-list style="overflow: auto; height: 300px"></virtual-list>

<!-- scrolls within the wrapper size -->
<div id="wrapper" style="overflow: auto; height: 300px">
  <p>Hello world!</p>
  <virtual-list></virtual-list>
</div>

<!-- scrolls within the main document -->
<virtual-list></virtual-list>

<virtual-list> searches for the scroll target when attached. A scroll target is any element that has overflow: auto|scroll.

Added demo:
scrolling

Limitations

<virtual-list> will search for the scrolling element only when attached. Stuff like this will not work:

<virtual-list id=list></virtual-list>
<script>
  list.newChild = () => document.createElement('input');
  list.items = new Array(1000);
  setTimeout(() => {
    list.setAttribute('style', 'overflow: auto; height: 300px');
  }, 1000);
</script>

The user can still detach/attach the element. Not ideal, but gets the job done.

const parent = list.parentNode;
list.remove();
list.setAttribute('style', 'overflow: auto; height: 300px');
parent.append(list);

@valdrinkoshi
Copy link
Collaborator Author

@graynorton @domenic PTAL

@domenic
Copy link
Collaborator

domenic commented May 1, 2018

So, I thought the plan was that we were going to make <virtual-list> have a default width/height:

  • If layout is vertical, width is auto-determined by contents; height defaults to 150
  • If layout is horizontal, width defaults to 300; height is auto-determined by contents
  • If layout is vertical-grid or horizontal-grid, width defaults to 300, height defaults to 150

In all cases the virtual-list always has overflow: auto; you never need to set that yourself.

And you're not able to scroll within the main document with <virtual-list>; for that we'd need to expose a different element, <lazy-rendering-container> or similar.


From the implementation side, I thought the plan was to basically not change anything about the existing implementation, but rename it to <lazy-rendering-viewport>. Then create <virtual-list> as a wrapper around the <lazy-rendering-viewport>. Similar to http://jsbin.com/nagukajega/3/edit?html,output.

Implementation isn't as important as public API for <virtual-list>, but I do think it might be nice to one day expose <lazy-rendering-viewport>, so I kind of liked that implementation strategy.

ALso, this implementation strategy can avoid the limitations section mentioned, which seems pretty important, as those are not great limitations.

@valdrinkoshi
Copy link
Collaborator Author

valdrinkoshi commented May 1, 2018

Defaulting these values should be quite straightforward, as we'd need to add these to the <virtual-list> host styles:

:host {
  display: block;
  /* ... */
  width: 300px;
  height: 150px;
  overflow: auto;
}

Re default width/height: I think we should just have a default height, as display: block already takes care of the width. Correction: we'd need both width and height

If layout is horizontal, width defaults to 300; height is auto-determined by contents

We cannot do this, as we position all the children via position: absolute, and we're not rendering all the elements - we cannot compute the height of the container w/o rendering them all. The user will have to provide this info via style.

From the implementation side, I thought the plan was to basically not change anything about the existing implementation, but rename it to <lazy-rendering-viewport>. Then create <virtual-list> as a wrapper around the <lazy-rendering-viewport>. Similar to http://jsbin.com/nagukajega/3/edit?html,output.

The suggested configuration has issues regarding distribution, as from the RepeatsAndScrolls mixin POV, stamped children will be appended to the container element.

<lazy-rendering-viewport> would be the one that instantiates an internal VirtualList, and set its container to itself - meaning the stamped children will be hosted in the <lazy-rendering-viewport> light dom.

<lazy-rendering-viewport>
  #shadowRoot
    <style> /*...*/ </style>
    <slot></slot>
  #/shadowRoot
  <!-- stamped in the light dom -->
  <div>item 0</div>
  <div>item 1</div>
  ...
</lazy-rendering-viewport>

Now, if <virtual-list> hosts <lazy-rendering-viewport> in its shadow dom, styles will be scoped

<style> div { color: red } </style>
<virtual-list>
  #shadowRoot
    <lazy-rendering-viewport>
      <slot></slot>
      <!-- these are not red! -->
      <div>item 0</div>
      <div>item 1</div>
      ...
    </lazy-rendering-viewport>
  #/shadowRoot
</virtual-list>

If <virtual-list> hosts <lazy-rendering-viewport> in its light dom, it leaks implementation details.

lazy-rendering-viewport would have to expose the container property, so that virtual-list can use it, but that feels weird.

Alternatively, we could add logic in RepeatsAndScrolls to handle the case when container is a <slot>, and from it find the actual container - slot.getRootNode().host, but that feels like overkill right now (e.g. handle named slots, handle slots distributing slots, etc)

Also, we cannot avoid exposing <lazy-rendering-viewport> as it would be accessible via customElements.get('lazy-rendering-viewport').

ALso, this implementation strategy can avoid the limitations section mentioned, which seems pretty important, as those are not great limitations.

The example I mentioned in the description is specifically when the user wants to switch from "lazy-rendering-viewport" mode to "virtual-list" mode, which is very edge case and would be better supported by exposing a property.

For more common cases like updating the viewport height (e.g. virtualList.style.height = '300px'), the user would have to invoke virtualList.requestReset(). This will not be necessary if we use ResizeObservers (to be handled by #16).

@domenic
Copy link
Collaborator

domenic commented May 1, 2018

I don't understand the issue about not being able to auto-determine the height or width from the contents. Here is an example of a virtual-list that auto-sizes to its contents width: http://jsbin.com/nagukajega/3/edit?html,output. You can do the same thing flipped for auto-sizing to height.

The suggested configuration has issues regarding distribution, as from the RepeatsAndScrolls mixin POV, stamped children will be appended to the container element.

Again, please take a look at the jsbin for how this is possible.

It sounds like a lot of your issues are with the current architecture, e.g. RepeatsAndScrolls, VirtualList, container, etc. Maybe it's time to abandon that architecture if it doesn't help us achieve our goals. I've given a very simple example of the desired result in that jsbin; perhaps we should use that as the basis, instead of the current internals?

The example I mentioned in the description is specifically when the user wants to switch from "lazy-rendering-viewport" mode to "virtual-list" mode, which is very edge case and would be better supported by exposing a property.

The problem is actually that the list has two modes at all. It should not. The lazy-rendering-viewport mode should not be exposed to developers in the same API surface as virtual-list. We can consider building a separate component in v2, but the idea is that virtual-list is only one thing, and lazy-rendering-viewport is a separate thing.

@valdrinkoshi
Copy link
Collaborator Author

We cannot auto-determine width/height because we're positioning the children with position: absolute + transform: translate - this is much more performant than relying on auto-sizing, and allows containment via contain.

The only difference between the 2 modes is in the default style, one has an explicit sizing and overflow: auto (virtual-list mode), the other sizes itself and makes a scrolling parent scrollable (lazy-rendering-viewport mode).

The drawback of requiring an explicitly sized <virtual-list> is that it's more setup do be done by the user. I see an analogy with a <div>: by default, it autosizes its height according to the content, and the developer can make it scrollable via <div style="overflow: auto; height: 300px">.

With the current defaults, the developer would be able to just replace the div with a virtual-list for it to "just work".

@valdrinkoshi
Copy link
Collaborator Author

valdrinkoshi commented May 1, 2018

Regarding the implementation, we currently support both modes with the minimal number of DOM nodes, so I'd keep it that way. The only difference between the 2 modes would be these styles:

:host {
  display: block;
  position: relative;
  contain: strict;
}

:host(.explicitly-sized-mode) {
  overflow: auto;
  width: 300px;
  height: 150px;
}

@domenic
Copy link
Collaborator

domenic commented May 1, 2018

this is much more performant than relying on auto-sizing, and allows containment via contain.

I'd like to see some benchmarks here? Contain in particular makes things slower today. But, we can start with explicitly requiring width/height for now.

With the current defaults, the developer would be able to just replace the div with a virtual-list for it to "just work".

But, as we discussed previously, this is an explicit non-goal for the virtual-list project. It is instead a goal for the lazy-rendering-viewport project, which is separate. We should not conflate the two into a single element just because it's easy inside your implementation. It makes for a confusing developer experience and bad API.

The only difference between the 2 modes would be these styles:

That's not the whole story though. The fact that these modes are incompatible, and that you're using styles as a mechanism for switching between them, causes problems of the sort listed in your OP's "limitations", where now the style attribute behaves differently on this element than on any other element in the platform.

This two modes need to be separated into two elements, which you can not even attempt to switch between. Otherwise the behavior for the developer is too confusing. It's like <input>, but worse, because at least <input> automatically updates when you change its type=""; it doesn't need to be removed and re-attached to the DOM.

@valdrinkoshi
Copy link
Collaborator Author

I don't have any handy benchmark, but in general doing positioning ourselves allows for zero paint/layout cost for items not being recycled during scroll, or when other items are being recycled. In any static/flow/flex layout we'd invalidate the container box and cause unnecessary layout/paint.
This is done also in WinJS, <iron-list> (study notes) and in the Infinite Scroller example (see Layout section)

The two modes can be compatible, we'd have to just expose a scrollTarget property which would handle the style update accordingly. This is similar to the scroll target property of <iron-list> - see https://github.com/domenic/infinite-list-study-group/blob/master/studies/Polymer-iron-list.md#configurable-scroll-target

With scrollTarget, the user could update <virtual-list> via property/attribute, in a similar fashion as <input> with type.

In <iron-list> we did the same choice - adopt the "explicit sizing" mode and require the user to set width/height - but that also caused user confusion, e.g see these issues, or this request in particular PolymerElements/iron-list#498. The types of problems users face are:

  • list doesn't render because it wasn't sized
  • list renders all items because it was wrongly sized

In this PR, I was trying to avoid exposing scrollTarget to the user, and infer it from the computed style. Switching between the two modes is a negligible feature, but if we intend to support it, it's fairly easy to just expose scrollTarget property and handle it.

@domenic
Copy link
Collaborator

domenic commented May 1, 2018

The types of problems users face are:

Having a default size fixes that.

I continue to believe we shouldn't have two modes in a single element.

@valdrinkoshi valdrinkoshi changed the title handle scrolling container compute <virtual-list> scrollTarget May 1, 2018
@valdrinkoshi
Copy link
Collaborator Author

Opened #42 which makes <virtual-list> the default scroll target.

@valdrinkoshi
Copy link
Collaborator Author

Closing this in favor of #42

@valdrinkoshi valdrinkoshi deleted the fix-23-scroll-element branch June 12, 2018 22:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants