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

<virtual-content> #152

Closed
wants to merge 42 commits into from
Closed

<virtual-content> #152

wants to merge 42 commits into from

Conversation

bicknellr
Copy link
Collaborator

@bicknellr bicknellr commented Jan 22, 2019

The virtual-content element is a different approach to implementing virtual list. This branch assumes that searchable-invisible DOM / display locking is available and necessary for allowing the content of the virtual list to be searchable, work with same-page anchor navigation, etc. This assumption means that all content that the user of the virtual-content is aware of should be inserted into the virtual-content element, which will manage the visibility of that content automatically. Additionally, because all content that the user of the virtual-content is aware of must be in the tree, this means that most of the original API isn't needed by virtual-content. (Concepts like element recycling and lazy item fetch / instantiation should be implemented on top of virtual-content.) See #147 for more info.

For this PR:

  • Update README.md
  • Update DESIGN.md
  • Remove current demos broken by this PR.

For later PRs?

  • Add new demos covering the same topics as those in master.

This PR is only meant for code review of the current content of the branch, without merging it into master (a separate PR).

…le DOM. (...)

This new implementation is based around the assumption that all of the features
enabled by searchable invisible DOM require those elements to actually be in
the main document.
…r. (...)

ChildManager doesn't serve any purpose: MutationObserver is needed to discover
parser-inserted children and MutationObserver callbacks are called before the
next paint, giving overridden Node methods no performance advantage.
…sing scroll event targets. (...)

The target of scroll events isn't always the element that needs to have its
`scrollTop` corrected when height estimates are found to be incorrect during an
update. This can happen if the virtual content is nested in multiple scrollable
elements and the user scrolls more of the virtual content into view using one
of the outer scrollable elements.
…rt intersection with unrendered space. (...)

Before this change, the virtual-content did not update if its nearest scroll
container (possibly the window) resized in a way that made more of the
virtual-content visible. To fix this, sentinel elements sized to fill the space
between visible elements are inserted and monitored with an
IntersectionObserver, using the viewport as the root. This has the convenient
side effect of covering all of the cases that were previously handled by
listening to scroll events. Additionally, this prevents any virtual-content
elements that do not intersect the viewport from updating for scroll events.
…tent's viewport intersection state is unnecessary. (...)

Tracking the state of the virtual-content element's intersection with the
viewport isn't necessary because the callback won't be called if the
virtual-content element's intersection with the viewport didn't change or the
virtual-content element doesn't intersect the viewport (meaning none of its
descendants intersect the viewport).
…es the scroll event listener with an IntersectionObserver.
…ing construction. (...)

Reading `childNodes` at this time violates the "Requirements for custom element
constructors and reactions".

https://html.spec.whatwg.org/multipage/custom-elements.html#custom-element-conformance
@kenchris
Copy link
Contributor

Would be nice with some update to the readme or so, as this is removing all the existing implementation. Also, I assume that means that most of the demos will be broken.

@domenic
Copy link
Collaborator

domenic commented Jan 22, 2019

So, the intent here is to actually have two side-by-side branches, master and virtual-content-2, with virtual-content-2 prototyping a new approach based on searchable invisible DOM. We only opened this pull request because we want to do some initial code review of the virtual-content-2 branch; we won't actually merge it and replace the master branch.

Eventually, the virtual-content-2 branch may have the same capabilities and demos as the master branch, at which point it can replace it. But that's some ways off.

@bicknellr
Copy link
Collaborator Author

Sorry, @kenchris, I should have included a description with this PR!

@bicknellr bicknellr changed the title Virtual content <virtual-content> Jan 28, 2019
Copy link
Collaborator

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Overall looks really solid. A few minor suggestions, and ideally we should split up the giant update method. But besides that the structure is really great; the comments are clarifying; etc.

demo/virtual-content/random.html Outdated Show resolved Hide resolved
demo/virtual-content/random.html Outdated Show resolved Hide resolved
demo/virtual-content/spec.css Outdated Show resolved Hide resolved
src/VirtualContent.js Outdated Show resolved Hide resolved
src/VirtualContent.js Outdated Show resolved Hide resolved
src/VirtualContent.js Outdated Show resolved Hide resolved
src/VirtualContent.js Outdated Show resolved Hide resolved
src/VirtualContent.js Show resolved Hide resolved
src/VirtualContent.js Outdated Show resolved Hide resolved
src/VirtualContent.js Outdated Show resolved Hide resolved
@domenic
Copy link
Collaborator

domenic commented Feb 8, 2019

Review complete! Going to move this to a new clean virtual-content branch with a single commit, then link to that branch from master. Closing this PR!

@domenic domenic closed this Feb 8, 2019
@domenic domenic deleted the virtual-content-2 branch February 8, 2019 19:52
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.

3 participants