edenspiekermann / a11y-toggle

A tiny script for accessible content toggles.
https://edenspiekermann.github.io/a11y-toggle/
MIT License
304 stars 21 forks source link

Discussion about JS performance #2

Closed i-like-robots closed 8 years ago

i-like-robots commented 8 years ago

Lovely work on this script =]

I've tested a couple of small variations on JSPerf. Small gains it seems from initial tests and I thought it may be useful to share.

http://jsperf.com/a11y-toggle/2

KittyGiraudel commented 8 years ago

Thank you so much for doing this. Funny how 1 querySelectorAll still is slower than several getElementById.

i-like-robots commented 8 years ago

Yes it's a little surprising. It needs a few more variations, my hunch is that a single loop will be faster but I just threw that together over breakfast!

On Wednesday, 9 March 2016, Hugo Giraudel notifications@github.com wrote:

Thank you so much for doing this. Funny how 1 querySelectorAll still is slower than several getElementById.

— Reply to this email directly or view it on GitHub https://github.com/edenspiekermann/a11y-toggle/issues/2#issuecomment-194191702 .

giuseppeg commented 8 years ago

Another perf thing to keep in mind is that if a target is removed from the dom it will be retained in memory and can't be gc-ed because targetsMap references to it.

KittyGiraudel commented 8 years ago

Ah. Interesting. Anyway to prevent that?

giuseppeg commented 8 years ago

You could cache the target as a custom attribute on the toggle element.

Object.defineProperty( 
  toggle, 
  'a11y-toggle-' + toggle.id, 
  { 
    value: document.querySelector(
      toggle.getAttribute('aria-controls')
    ), 
    configurable: true 
  }
)
KittyGiraudel commented 8 years ago

See https://github.com/edenspiekermann/a11y-toggle/issues/7#issuecomment-196204725.