Fixed #3249

[YoshiWalsh]

Only draw non-visible items once when they are loaded, instead of continuously every frame

I spent quite a lot of time changing this one line and making sure that everything still works, but I'm still nervous that I may have inadvertently broken something and not noticed. If anyone can think of anything that's likely to be broken by this fix please let me know and I'll be happy to help resolve it.

[yotamberk]

@YMIndustries what issue does this come to solve?

[YoshiWalsh]

Hi @yotamberk , this PR solves this issue: #3249

I've tried put as many details as I can in the issue and also included an example that detects the issue. Due to the fact that the performance issues only manifest with a complex template I haven't been able to create a demo that actually demonstrates the lag, but I'm hoping that the example that I did include is sufficient to show that a problem exists.

[yotamberk]

I have a problem then with the item.displayed property. I would expect it to indicate when the item should be drawn. Why is this solving the problem? Is the displayed property incorrect? I think we have a deeper issue here.

[YoshiWalsh]

I agree that there's a deeper issue. The displayed property is false for items off-screen (and therefore not rendered). When Group.prototype.redraw is called and a restack is triggered, the 'displayed' property is false for items that are not on the screen. We need to know the height of these items, so we call 'redraw' on them (e.g. RangeItem.prototype.redraw) and this function creates a DOM element (if one does not already exist) and appends the item to the DOM. (If it's not already attached) At this point the displayed property is set to true. Then the width and height of the item are measured and stored.

At the end of Group.prototype.redraw, any items off the screen have their 'hide' method called (RangeItem.prototype.hide). This removes the item from the DOM and sets the displayed property back to false. This means as soon as the next restack event occurs (possibly the next frame if the user is scrolling along the timeline) then the element will be redrawn and re-appended to the DOM.

The expensive operation here is attaching the element to the DOM, as this causes a reflow event for the item's element and all its children. My items have fairly complex templates so they have many children, and this reflow event is butchering my application's performance. The way that the items are redrawn every frame is also not consistent with the documentation, as I pointed out in the issue I reported.

By changing it to only call redraw if the 'dom' property is falsey, I make it so it only redraws items that have not yet been drawn. Technically this could be wrong if a stacked item has changed size while out of the view, but in this case the size will be updated as soon as the element enters the view and is re-attached to the DOM. I don't see any real way that we can avoid this issue though, unless we simply kept the items permanently attached to the DOM.

I'm not sure at what point this behaviour was broken (I suspect the 'displayed' property may have been repurposed at some point) but I believe that the fix I've contributed is an effective way of improving the performance with no real drawbacks (or at least none that wouldn't also be present in any other approach to solving the issue). I do think there's a deeper issue with the displayed property, but that property is used in quite a few places and changes to how it works feel likely to cause other bugs to arise.

[yotamberk]

Thanks for the elaborate explanation. I agree, there is something fishy about this property and should be refactored. I think you've explained it very well. I accept this PR since the benefits are worth it.
I'll note myself to check this out in-depth