report: fix sticky header toggling too late #13279
Conversation
|
did you test w/ |
This reverts commit a81200a.
|
I changed this a bunch and I think all previous comments are mostly irrelevant at this point. Hoping it's more clear whats happening. |
report/renderer/topbar-features.js
Outdated
| } catch (e) {} | ||
|
|
||
| if (!this.stickyHeaderEl) return; |
There was a problem hiding this comment.
Just curious, does sticking return in the catch block narrow the type correctly?
There was a problem hiding this comment.
good idea. done. (also moved it a bit)
report/renderer/topbar-features.js
Outdated
| this.highlightEl = this._dom.createChildOf(this.stickyHeaderEl, 'div', 'lh-highlighter'); | ||
|
|
||
| // Defer behind rAF to avoid forcing layout | ||
| requestAnimationFrame(() => requestAnimationFrame(() => { |
There was a problem hiding this comment.
Why double requestAnimationFrame?
There was a problem hiding this comment.
getScrollParent forces layout. and since it currently happens during report creation, we have TWO 50ms blocks of style recalc (one forced and one normal async).
since this code can run a little later without consequence.... i figured deferring it was nice :)
doubleRaf specifically because w/ singleRaf it can still happen in the same frame (and thus still have double the rendering costs).
this would be easier to communicate had i kept the traces when i was doing this.. but alas they gone. :p
report/renderer/topbar-features.js
Outdated
| const scrollParent = this._getScrollParent(this.stickyHeaderEl); | ||
| scrollParent.addEventListener('scroll', e => this._updateStickyHeader(e)); | ||
| // 'scroll' handler must be Element | Document, but resizeObserve can't be Document. | ||
| new window.ResizeObserver(e => this._updateStickyHeader(e)) |
There was a problem hiding this comment.
nit: Can we bind this._updateStickyHeader instead? Also, looks tike there is no need to pass the e through.
There was a problem hiding this comment.
nit: Can we bind
this._updateStickyHeaderinstead?
we could. these two lines would look like this (or something close):
scrollParent.addEventListener('scroll', this._updateStickyHeader.bind(this));
new window.ResizeObserver(this._updateStickyHeader.bind(this))i personally dont like the binding style because you create a new function object which has a separate identity from the actual implementation.
so ever since arrow functions have been a thing, i think it makes for a cleaner look without such indirection.
Also, looks tike there is no need to pass the
ethrough.
true! good call. coulda just done a () => this.USH(). thats def better
report/renderer/topbar-features.js
Outdated
| const showStickyHeader = topbarBottom >= scoreScaleTop; | ||
| const categoriesTop = this.categoriesEl.getBoundingClientRect().top; | ||
| const showStickyHeader = topbarBottom >= categoriesTop; | ||
| console.log('showstickyheader', showStickyHeader, topbarBottom, categoriesTop); |
| document = window.document; | ||
|
|
||
| global.window = global.self = window; | ||
| global.window.requestAnimationFrame = fn => fn(); |
There was a problem hiding this comment.
Can we use the JSDOM implementation?
| }; | ||
| }; | ||
|
|
||
|
|
| global.HTMLElement = document.window.HTMLElement; | ||
| global.HTMLInputElement = document.window.HTMLInputElement; | ||
| global.HTMLInputElement = document.window.HTMLInputElement; | ||
|
|
4 participants
it currently (on master) toggles too late because its setup to flip when the scorescale has cleared the topbar.
now that the scorescale moved... that is wrong.
i changed it to be when the top of the categories clears the topbar.
Also I added a double RAF to remove a forced layout and drop like 50ms of rendering cost. 🎉