Huge delay when using ally.maintain.disabled

[mikeebee]

I'm using ally.maintain.disabled on multiple off canvas elements, when I open any of them it takes ~3 seconds to open. If I remove Ally they are fine.

This only happens with clear cache and once one is opened they are all fine.

I can't imagine it's related but the network tab shows that a data svg image is missing.

I have a link to a staging site with the issue but I can't publish it. I have sent you it direct.

[rodneyrehm]

Received the link, will have a look on Saturday :)

[rodneyrehm]

I've received the URL of the staging site and taken a quick peek. It seems like we're spending ~3 seconds in layout. But with ~600 DOMElements this site is not exactly huge.

Because this only happens on an empty cache, the reason is likely to be found in the supports tests.

If you were to add the source map (ally.min.js.map) file to the directory where you put ally.min.js, I might be able to figure out what the problem is.

[rodneyrehm]

I've spent some more time looking at the timeline of opening the menu.

When the menu is first opened, ally.js runs it's supports tests. Using detect-focus.js an test like focus-invalid-tabindex will do the following steps:

1. create a container element that's fixed off-screen
2. create the element to test as specified by focus-invalid-tabindex
3. store the current scroll position
4. try to focus the element to test and restore to previously focused element
5. remove container element
6. reset scroll position

This process is repeated for 20+ tests.

Each test will flush the layout buffer (internal browser optimization) 4 - 6 times. That's mainly due to reading and writing .scrollTop, .scrollLeft, as well calling .focus() (based on What forces layout / reflow).

Because one recalculation of style takes around 25ms, we end up with around 2.7 seconds of calculating styles (25ms * 5 flushes * 22 tests).

I've never noticed the impact detect-focus.js has because of its reflows, because in my apps the recalculation of style takes 0.1ms. That still adds up to ~120ms, but is hardly noticeable.

---

Originally I though splitting the tests up into individual files and only running them when required was a good idea. After looking at the timeline's flamechart I'm not so sure anymore. If we run all ~30 tests in a row, we'd reduce the number of style calculations caused. Simply by running the scroll position reset only once and potentially a single .focus() per test, we might get down to ~30 - 40 style invalidations.

I wonder what happens if we run the tests in a temporary iframe. That should decouple all of this from the page's styles and make sure that we can't run into the situation you're facing now.

---

All that said, you should still try to figure out why Chrome needs 25ms to recalculate styles on that site.

[mikeebee]

Thanks for having such a detailed look into this.

Do you still want me to add the ally.min.js.map file?

So is 25ms a lot to recalculate styles?

Edit: I replied before I looked into how to analyse slow recalculation. I'm not expecting an easy fix, there are a lot of styles on the site. I'll have to look into it.

Thanks

[rodneyrehm]

Do you still want me to add the ally.min.js.map file?

If you want to check out the timeline yourself, that would help. I've reproduced the problem outside of your project, so I don't need it anymore.

So is 25ms a lot to recalculate styles?

Yes.

Edit: I replied before I looked into how to analyse slow recalculation. I'm not expecting an easy fix, there are a lot of styles on the site. I'll have to look into it.

Good Luck!

[rodneyrehm]

I've implemented the refactorings (all tests in one go to reduce number of layouts, run tests in iframe to reduce impact of layout) in refactor/142-supports. I've run into an issue with [Intern, BrowserStack, IEDriver] causing a single test to fail on IE10. This failing test is currently keeping me from merging the improvements. The problem is being analyzed by the BrowserStack support. Once they get back to me I'll merge this provide create a beta release.

---

@mikeebee regarding your page I'm inclined to recommend you not use ally.maintain.disabled for the menu. Since the menu fully hides all the page's content, you can simply set your page content to visibility: hidden (or display: none) once the menu is open. That's much more efficient in every regard.

[mikeebee]

Ok, no worries. Thanks for the help!

[rodneyrehm]

I'm sorry for the insane delay. BrowserStack didn't get back to me with a solution to the failing test. I disabled the failing test for IE10 (as this clearly is a WebDriver problem, not an issue of the ally.js code) and merged the refactored code. You can test the results of build 552.