Skip to content

Improve performance of tabindex module#228

Merged
hippotastic merged 7 commits intoexpressive-code:mainfrom
delucis:chris/tabindex-perf
Aug 1, 2024
Merged

Improve performance of tabindex module#228
hippotastic merged 7 commits intoexpressive-code:mainfrom
delucis:chris/tabindex-perf

Conversation

@delucis
Copy link
Copy Markdown
Collaborator

@delucis delucis commented Jul 26, 2024

This PR aims to improve performance of the script which applies/removes tabindex attributes to code blocks depending on whether or not they are scrollable.

While looking into some performance details on lower-powered devices, I realised that because this script reads element dimensions (scrollWidth and clientWidth), it could cause forced style recalculation/layout work if running while this information is not yet fully resolved (for example during initial page load).

This PR wraps the tab index update logic in a requestIdleCallback(), to defer setting tabindex to a moment when the main thread is idle. In general this means that any required style/layout work the browser has planned will be complete before Expressive Code tries to measure anything.

I also removed the initial call to updateTabIndex() made at the same time as adding an element to the resize observer. In my testing, ResizeObserver already fires its callback immediately for every newly observed element, so manually calling updateTabIndex() was redundant.

I tested this alongside another change in withastro/starlight#2155 which allowed the DOMContentLoaded event to fire quite a bit earlier on a throttled CPU.

@netlify
Copy link
Copy Markdown

netlify bot commented Jul 26, 2024

Deploy Preview for expressive-code ready!

Name Link
🔨 Latest commit 0fe0421
🔍 Latest deploy log https://app.netlify.com/sites/expressive-code/deploys/66ab469198af0f0008fb8f5c
😎 Deploy Preview https://deploy-preview-228--expressive-code.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@hippotastic
Copy link
Copy Markdown
Collaborator

Thank you for this PR!

If I remember correctly, one of the reasons why I used setTimeout with the value 250 in my original code was that horizontally resizing a browser window containing a page with many code snippets felt sluggish without a reasonable debounce timeout value. Have you checked the subjective performance of your new code vs. the original code in such a scenario? I'm curious to see if window.requestIdleCallback also satisfies this and does not get called too often to slow resizing down.

@delucis
Copy link
Copy Markdown
Collaborator Author

delucis commented Jul 27, 2024

Have you checked the subjective performance of your new code vs. the original code in such a scenario?

Good question. No, I didn’t think to test that — I will. We actually have similar logic for resizes for something in Starlight where I ended up with setTimeout(() => requestIdleCallback(...), 200), so it could be still needed. Let me confirm.

@delucis
Copy link
Copy Markdown
Collaborator Author

delucis commented Jul 27, 2024

Did some testing, and it does look like with only the requestIdleCallback, the resize logic can run a bit too often, so I have reintroduced the setTimeout as well.

I also noticed that running tests updates the minified version which appears to be committed to the repo so I’ve included that update too. Is that correct?

@delucis
Copy link
Copy Markdown
Collaborator Author

delucis commented Jul 27, 2024

Hmm, hold on, just tested this in withastro/starlight#2155 and seeing the old behaviour again. I think I’ll need to investigate a bit further.

EDIT: I may just have messed up my patch in that PR. Testing again now.

@delucis
Copy link
Copy Markdown
Collaborator Author

delucis commented Jul 27, 2024

Ah yeah, seems to be something to do with Netlify’s caching of the ec.js in between builds. Testing locally, I can confirm this is looking good 👍

@delucis
Copy link
Copy Markdown
Collaborator Author

delucis commented Aug 1, 2024

@hippotastic Do you need anything else from me here? I think this PR should be good to go!

@hippotastic hippotastic merged commit 876d24c into expressive-code:main Aug 1, 2024
@hippotastic
Copy link
Copy Markdown
Collaborator

No, I also think it's fine! Thank you :)

@delucis delucis deleted the chris/tabindex-perf branch August 1, 2024 08:52
@hippotastic
Copy link
Copy Markdown
Collaborator

I've just released your fix as v0.35.4. Thanks again!

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.

2 participants