Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache the scrollbar width for the life of the application #373

Merged
merged 2 commits into from
Sep 25, 2019

Conversation

abettadapur
Copy link
Contributor

@abettadapur abettadapur commented Sep 24, 2019

Native scrollbar width should not change over the life of an application.
Cache this value to eliminate some forced reflows

See #205 and #371

@andersk
Copy link
Contributor

andersk commented Sep 24, 2019

Native scrollbar width should not change over the life of an application.

It changes when zooming the browser, since the physical width of the scrollbar remains the same but the physical size of a px changes. We can still cache the width (or better, just share the cache that’s already there across all SimpleBar instances), but the cache needs to be invalidated on zoom.

@andersk
Copy link
Contributor

andersk commented Sep 24, 2019

Although, this occurs to me now: on browsers that support scrollbar-width: none (Firefox ≥ 64) or ::-webkit-scrollbar { display: none; } (Chrome ≥ 2, Safari ≥ 4), we could theoretically just hide the native scrollbar and avoid the need to detect its width at all.

@Grsmto
Copy link
Owner

Grsmto commented Sep 24, 2019

@andersk that's right but how do you detect that support in JS? Is that even possible?

@andersk
Copy link
Contributor

andersk commented Sep 24, 2019

I think we can detect scrollbar-width support with

if ("scrollbarWidth" in someHTMLElement.style)
    

and ::-webkit-scrollbar with

try {
    someStyleSheet.insertRule('::-webkit-scrollbar { }');
} catch (err) {
    
}

@Grsmto
Copy link
Owner

Grsmto commented Sep 25, 2019

Interesting. The stylesheet hack might be as slow as doing the detection itself no? :P
If we go down that route we could directly just check if we are on WebKit if they reliably all support this.

@Grsmto Grsmto merged commit 708e6ca into Grsmto:master Sep 25, 2019
@Grsmto
Copy link
Owner

Grsmto commented Sep 25, 2019

Hey, thanks for your contribution!
I'm merging this without further review because I'll rework it in the next branch with some more things (also right now the code doesn't actually even compile :) )

@andersk
Copy link
Contributor

andersk commented Sep 25, 2019

Interesting. The stylesheet hack might be as slow as doing the detection itself no? :P

Dunno. But here’s a better idea: if we put

.simplebar-content-wrapper::-webkit-scrollbar { display: none; }

in simplebar.css, we can detect whether it worked with

if (window.getComputedStyle(contentWrapperEl, "::-webkit-scrollbar").display === "none")
    

If we go down that route we could directly just check if we are on WebKit if they reliably all support this.

That also seems like fair game for a vendor-prefixed feature, I guess.

@Grsmto
Copy link
Owner

Grsmto commented Sep 25, 2019

Yeah cause thing is getComputedStyle is slow as well! As it's a performance optimization, I suppose we need to avoid any kind of DOM reading otherwise it might not be worth it in the end.
Anyway I'll try something I think you put me on the right track!

@andersk
Copy link
Contributor

andersk commented Sep 25, 2019

getComputedStyle(…).display only forces a style recalc, which should at least be much less work than e.g. getCompuedStyle(…).width, or the getBoundingClientRect() of scrollbarWidth. Also, it need only be tested once and not on every resize event. So I’d still expect it to be well worthwhile.

But if you’re going with user-agent sniffing, that seems fine.

For posterity, I note that if (CSS.supports("selector(::-webkit-scrollbar)")) should work but doesn’t; maybe it will in the future.

@Grsmto
Copy link
Owner

Grsmto commented Sep 26, 2019

Alright. I think best is to do a quick jsperf of all this 😄
I would have supposed that as long as you do getComputedStyle whatever you access in the returned object doesn't change anything to the performance impact as it's a function call returning an object... I'm surprised that it changed based on the property accessed.

@andersk
Copy link
Contributor

andersk commented Sep 26, 2019

Browsers are pretty complicated sometimes…here’s a good reference: https://gist.github.com/paulirish/5d52fb081b3570c81e3a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants