[Perf] Run scroll and frame handlers outside of angular zone

[tstirrat]

To improve perf, attach the scroll/frame handlers outside of the zone. Only go back into the zone if there is an actual change to be emitted.

• There is also a slight fix here where we don't attach the host element scroll listener if [parentScroll] is supplied.

To highlight the difference in the demo app, I scrolled just a tiny bit so that no actual changes need emitting:

Before:

After:

Once a change is emitted, we must do this in the angular zone and incurr the change detection, so the difference is only a small reduction in the initial scroll handler time:

Before:

After:

Fixes #66

/cc @ghetolay

[ghetolay]

Use Renderer2 instead of direct DOM manipulation.

[tstirrat]

Will do

[tstirrat]

Done.

[ghetolay]

this.zone.run() is nested inside an if, meaning that on the else case we won't run the zone. In such case, I think scrollHeight and topPadding won't be updated on the template because no CD will run. Using zone.run() outside the if would kill the perf so the best solution is probably to remove the bindings and use Renderer2 to set the value programmatically.

[tstirrat]

this.scrollHeight and this.topPadding only change to new values if the start and end change, so I think it's okay. When the there is an actual change, the zone.run will cause CD which will detect the property changes.

Correct me if I am wrong here.

[tstirrat]

They might also change if the items array changes, which would already cause a CD cycle.

[ghetolay]

I'm don't fully understand your computations I just saw that you're setting this.scrollHeight and this.topPadding before the if and their values seemed unrelated to start and end.
Here is the use case I'm concerned about :
You can scroll a bit and keep the same items displayed (so same start and end) while you still need to show that scroll by moving the items a little (which seems to be done with topPadding).

If this.scrollHeight and this.topPadding are not important on the else case it's even better for perf to not sync their value with the DOM so all good then.

[johaven]

To optimize a little more the scroll, it would be interesting to add the deletion of the hover during the scroll. Applicable for those like me who use the hover on content.

https://css-tricks.com/turn-hovers-scroll/

An example:

private scrollHovering = () => {
        clearTimeout(this.scrollTimer)
        if (!this.contentElementRef.nativeElement.classList.contains('table-disable-hover')) {
            this.contentElementRef.nativeElement.classList.add('table-disable-hover')
        }
        this.scrollTimer = setTimeout(() => this.contentElementRef.nativeElement.classList.remove('table-disable-hover'), 200)
        this.refresh()
    }

    private addParentEventHandlers(parentScroll: Element | Window) {
        if (parentScroll) {
            this.zone.runOutsideAngular(() => {
                parentScroll.addEventListener('scroll', this.scrollHovering)
                if (parentScroll instanceof Window) {
                    parentScroll.addEventListener('resize', this.refreshHandler)
                }
            })
        }
    }

[tstirrat]

@johaven the scroll class is out of scope for this PR, maybe add it as an issue, after this PR goes in you could use Renderer2 to toggle a .scrolling class on the container element.

[tstirrat]

@ghetolay I've updated it to use Renderer2 for the height and transform styles. What do you think?

[ghetolay]

Looks fine to me now.

Maybe you could avoid manipulating the DOM is it's to set the same value.
Dunno how often this could happen and how much we would gain.

[tstirrat]

Yeah, we could save some cycles. Apparently, setting CSS to the same value does incurr a small cost

[johaven]

Just an idea that i applied in my code : it seems to be lighter to execute calculateDimensions function only on resize events to optimize the refresh function speed.

[tstirrat]

@johaven I think calculateDimensions also needs to be run when items.length changes.

[tstirrat]

Without unit tests, I don't want to make too many change in a single PR.

The other perf things could be done in another PR.

Also, I started working on some unit tests, but I don't have a heap of free time at the moment :)

[johaven]

My code:

refresh(updateDimensions: boolean = false) {
        this.zone.runOutsideAngular(() => {
            if (updateDimensions || !this.dimensionsView) {
                this.calculateDimensions()
            }
            requestAnimationFrame(() => this.calculateItems())
        })
    }
...
private refreshWithDimensions = () => {
        this.refresh(true)
    }
...
this.eventResizeHandler = this.renderer.listen('window', 'resize', this.refreshWithDimensions)
...
if (start !== this.previousStart || end !== this.previousEnd) {
            ...
                if (this.startupLoop === true) {
                    this.refresh()
                } else {
                    this.change.emit({start, end})
                }
            })

        } else if (this.startupLoop === true) {
            this.startupLoop = false
            this.refresh(true)
        }
...
ngOnChanges(changes: SimpleChanges) {
        this.previousStart = undefined
        this.previousEnd = undefined
        const items = (changes as any).items || {}
        if ((changes as any).items !== undefined && items.previousValue === undefined
            || (items.previousValue !== undefined && items.previousValue.length === 0)) {
            this.startupLoop = true
        }
        this.refresh(true)
    }

[tstirrat]

@ghetolay updated, now it only updates styles when their respective value has changed

[tstirrat]

@rintoj over to you, I think this is ready to merge.

[rintoj]

@tstirrat, Thanks for your contribution. While testing this PR, I found that the component is breaking when items array is an empty array. The scroll is indicating that we have more items in the list.

[rintoj]

@tstirrat, found the root cause. I will fix.