[FEATURE occlusion]: Preliminary Vertical collection integration

[alexander-alvarez]

WHY
#12 #435
Especially #12 (comment)

WHAT
This PR introduces experimental integration with https://github.com/html-next/vertical-collection, as it is nearing a 1.0 in terms of stability. By setting the occlusion=true flag and setting a value for estimatedRowHeight, we deactivate ember-scrollable and use vertical-collection instead.

There is a new section in the cookbook cookbook/occlusion-rendering where an example is live

Some compromises with respect to functionality are made with this initial release so that it can get into people's hands and begin testing and playing with it. These are listed below:

CAVEATS (To be turned into issues)

• Must define widths for all columns -- Fixing this involves handling resize events, and making sure column widths stay synchronized across header / body tables
• Infinite render needs some TLC -- Fixing this involves usage/understanding of scrollBuffer property in ELT & vertical collection
• Fixed footer + occlusion rendering -- likely need to account for footer width when determining the width of the body as we did for the header.
• Native scrollbars (and 14px padding) are added -- integrating virtual scrollbars into vertical collection or vertical collection into ember scrollable
• Tests?

[offirgolan]

This is an amazing start @alexander-alvarez. Thanks for putting in the time to make this happen! Im super excited for the next iterations 😄

[markcellus]

Must define widths for all columns -- Fixing this involves handling resize events, and making sure column widths stay synchronized across header / body tables

Will this fix what I attempted to in #298?

[alexander-alvarez]

@mkay581 quite possibly, as the implementation will likely have to inspect the DOM like you do in your PR

[alexander-alvarez]

woopss.. fat finger..
@buschtoens did you want to take a look?

[KeithClinard]

I am having two issues with this branch.

The table header is respecting the column widths that were specified, but the table cells are not.
This causes the columns to be misaligned.

The other issue is that the header appears to be a few pixels taller than it has room for, leading to a scroll bar in the the top right for the header.
This effect becomes more pronounced when more padding is added to the header cells.

Both of these issues can be reproduced easily by using the occluded-table in the dummy app.

Here is a screenshot of what I am seeing. (Borders added for emphasis)

[alexander-alvarez]

thanks @KeithClinard will take a look

[KeithClinard]

I just managed to get this to render properly.
It seems that the vertical-collection and occluded-content html nodes broke the table rendering.

I was able to get it to work by tweaking the display property of these nodes to use table styles.

vertical-collection {
  display: table;
  table-layout: fixed;
}
vertical-collection occluded-content:first-of-type {
  display: table-caption;
}

There is likely a cleaner way to write the css, but I thought I would give you what I have.

[alexander-alvarez]

@KeithClinard thanks for the investigation and the fix, I've incorporated your changes. Check them out.

As to:

The other issue is that the header appears to be a few pixels taller than it has room for, leading to a scroll bar in the the top right for the header.
This effect becomes more pronounced when more padding is added to the header cells.

I think you have to style the height of the header row appropriately so it doesn't scroll (padding & border widths included), but I could be wrong and there could be a way to have this not be a requirement.

[buschtoens]

I finally got around to try this out. First of all: absolutely amazing work! Thank you so much. 🎉 ❤️

I dropped this into a rather complex table that has to display up to 500 rows simultaneously. Previously we incrementally pushed rows into the table to not freeze the main UI thread. I can confirm that I can now just throw in all 500 rows at once and it just works.

I haven't yet discovered any styling issues. Functionality-wise I only discovered that scrollTo and scrollToRow do not work which makes sense since the rely on ember-scrollable.

I'll give this further test drives in the coming days and check back in. But so far, I can't 👍 this enough!

[buschtoens]

I found a minor issue, but I suspect that it's an issue in vertical-collection itself: when the table is created while the browser tab is hidden, this error is thrown, but does no apparent harm:

Uncaught Error: Attempted to remove a handler from an unknown element or an element with no handlers
    at ScrollHandler.removeScrollHandler (-private.js:1756)
    at removeScrollHandler (-private.js:1817)
    at Class.willDestroy [as _super] (component.js:199)
    at Class.willDestroy (debug-mixin.js:82)
    at Class.superWrapper [as willDestroy] (ember-utils.js:426)
    at invoke (backburner.js:274)
    at Queue.flush (backburner.js:153)
    at DeferredActionQueues.flush (backburner.js:345)
    at Backburner.end (backburner.js:455)
    at Backburner.run (backburner.js:539)

// assets/addon-tree-output/modules/vertical-collection/-private.js
  ScrollHandler.prototype.removeScrollHandler = function removeScrollHandler(element, handler) {
    var index = this.elements.indexOf(element);
    var elementCache = this.handlers[index];
    // TODO add explicit test
    if (elementCache && elementCache.handlers) {
      var _index = elementCache.handlers.indexOf(handler);

      if (_index === -1) {
        throw new Error('Attempted to remove an unknown handler');
      }

      elementCache.handlers.splice(_index, 1);

      // cleanup element entirely if needed
      // TODO add explicit test
      if (!elementCache.handlers.length) {
        _index = this.elements.indexOf(element);
        this.handlers.splice(_index, 1);
        this.elements.splice(_index, 1);

        this.length--;
        this.maxLength--;

        if (this.length === 0) {
          this.isPolling = false;
        }

        // TODO add explicit test
        if (this.isUsingPassive) {
          element.removeEventListener('scroll', elementCache.passiveHandler, { capture: true, passive: true });
        }
      }
    } else {
      throw new Error('Attempted to remove a handler from an unknown element or an element with no handlers');
    }
  };

[buschtoens]

Is this intentionally only a devDependency, because not every user will want to try occlusion rendering just yet?

[alexander-alvarez]

I think this was a blunder on my part... will update when I update to beta.9

[alexander-alvarez]

@buschtoens sounds related to html-next/vertical-collection#140

[alexander-alvarez]

Status update... generally well received & thanks to help from @KeithClinard we've gotten to a pretty good place...

I'm waiting for a 1.0.0.beta.9 release of vertical-collection because it has breaking changes, then we should be able to roll this out and get people to experiment with it.

[alexander-alvarez]

https://github.com/html-next/vertical-collection/releases/tag/v1.0.0-beta.9 was just released, so I updated this branch to reflect the API changes.

we may explore moving the vertical collection into lt-scrollable as we're figuring out the optimal mechanics (should we use it as a tag-less component?) and we're connecting full set of APIs offered by vertical collection.

Please take a look

[buschtoens]

I'm super happy with this PR and would love, if we'd merge it. Since it's behind a flag, we can change some finer details later. 😄

If you're okay with it, I'd merge and release, so I can properly test drive it in my apps. 😁 🚀

[alexander-alvarez]

The green button is 👇 😃

[ghost]

thanks for all the work @alexander-alvarez! (and reviewers like @buschtoens ). :) can someone merge this please? I would like to test this in our application that has numerous large tables.

[buschtoens]

Released as v1.12.0 🎉