Handler violations when using SL with SvelteKit

[brgrz]

Describe the bug

I'm running into handler violations where event handlers took too much time to return (for instance focusin of modals and drawers taking seconds to process).

To Reproduce
I made repro repo available here https://github.com/brgrz/sl-bugs

Steps to reproduce the behavior:

1. Clone the repo
2. Run npm i
3. Run npm run dev, it will run the SvelteKit app at http://localhost:3000
4. Open Chrome + devtools, in Devtools->Console tab->select All issues (or Verbose)
5. Open the app, click the Open drawer button
6. Inside the drawer try clicking around on the whitespace, try clicking into the input and then onto the whitespace and so forth
7. You should be seeing instances of [Violation] 'focusin' handler took 6519ms in the Console (while this happens, the app will freeze preventing you to click on anything else)

Expected behavior
No freezes, no violations/timeouts

Desktop (please complete the following information):

• OS: Windows 10
• Browser Chrome, Edge, both latest BUT was unable to reproduce this in Firefox

[claviska]

I ran the repro locally and saw the lag you reported (although I didn't see that specific focusin error). I decided to reduce it further by removing Svelte and I'm no longer seeing that behavior:

https://jsfiddle.net/claviska/fxLj85zo/

IMO, it feels like the dev server is hanging for some reason. You may want to post a bug to Svelte for investigation, because I'm not sure how to troubleshoot it at that level. Perhaps somebody with a deeper knowledge of Svelte's internals could identify what's happening.

I tried linking a local copy of Shoelace to the repro but didn't have much luck (it linked but didn't seem to use the rebuilt files). I'm out of time for today, but if anyone wants to dive in deeper, here are the two files that would be suspect should this be a problem with Shoelace:

https://github.com/shoelace-style/shoelace/blob/next/src/internal/modal.ts
https://github.com/shoelace-style/shoelace/blob/next/src/internal/tabbable.ts

The first one handles modal components such as dialog and drawer. When a dialog/drawer is shown, the modal is activated. This adds a couple listeners to the document and acts as a focus trap. When focus leaves the modal, the tabbable utility is called to identify elements that should receive focus. Focus is then returned to the appropriate element.

The tabbable utility is called only when focus leaves the modal. It should be reasonably performant, but there is a call to getComputedStyles that could be expensive if it were to be called repeatedly.

Just a guess, but perhaps something is repeatedly causing focus to leave the drawer, resulting in the tabbable utility to be called over and over again.

[brgrz]

IMO, it feels like the dev server is hanging for some reason. You may want to post a bug to Svelte for investigation, because I'm not sure how to troubleshoot it at that level. Perhaps somebody with a deeper knowledge of Svelte's internals could identify what's happening.

I actually built the sample with SvelteKit's static adapter and deployed it to IIS and the issue is still here so it's not a dev server thing.

Just a guess, but perhaps something is repeatedly causing focus to leave the drawer, resulting in the tabbable utility to be called over and over again.

What could that be? The repro sample is minimal, there is nothing else running.

[brgrz]

@claviska So just out of curiosity I switched back to the last Stencil version of SL (beta.27), still on latest SvelteKit and there were no delays happening.

Wrt to the beta.43 version, the delays only happen when clicking inside the sl-tab-panel, for example in between the two inputs that are inside the panel. It does not happen when I click on the modal/drawer itself.

UPDATE: I ran performance profiling in Chrome and Firefox. Chrome identifies the focus handling as being the issue, specifically getTabbableElements function as the most time consuming task, it actually get exponentially larger each time is fired.

Otoh, FF performance log shows completely different call stack and no delays actually happen.

[claviska]

What could that be? The repro sample is minimal, there is nothing else running.

Not sure. I don't know enough about how Svelte works, but maybe something in their binding/rendering logic affects focus. Totally guessing here, but it's strange to me that it only happens when Svelte is involved. It's also strange that it only happens in Chrome.

[claviska]

The getTabbableElements function is recursive. Given an element, it checks for tabbable elements in its light DOM, slots, and inside a shadow root if one exists. It would make sense to get hung up here if the logic were incorrect, but it should also fail when you remove Svelte from the equation. 😕

[brgrz]

What could that be? The repro sample is minimal, there is nothing else running.

Not sure. I don't know enough about how Svelte works, but maybe something in their binding/rendering logic affects focus. Totally guessing here, but it's strange to me that it only happens when Svelte is involved. It's also strange that it only happens in Chrome.

It is strange indeed. It doesn't happen in "regular" Svelte, only with SvelteKit. I did open an issue there though.

I'm sorry to say I'm seeing more issues now that you migrated to LitElement, little things here and there. General feeling is the Stencil version was more robust.

[claviska]

There have been a handful of things that I didn't translate well as the project migrated from one to the other from both misunderstanding specific APIs and simply not catching every detail. That fault should be attributed to me, not the libraries.

It doesn't happen in "regular" Svelte, only with SvelteKit. I did open an issue there though.

I look forward to seeing what the response is from that.

[geoffrich]

I was able to repro this outside of SvelteKit: https://jsfiddle.net/geoffrich/pub0orke/

I reduced the example a bit, but the key difference is one line: document.body.tabIndex = -1. SvelteKit sets tabIndex on the body so it can reset focus on client-side navigation. This repros the issue in the JSFiddle you linked. Interestingly, this only happens in Chrome/Edge, not in Firefox (I'm on Windows).

The issue is in these lines:

shoelace/src/internal/modal.ts

Lines 36 to 40 in 1c010ff

	if (this.isActive() && !path.includes(this.element)) {
	const tabbableElements = getTabbableElements(this.element);
	const index = this.tabDirection === 'backward' ? tabbableElements.length - 1 : 0;
	tabbableElements[index].focus({ preventScroll: true });
	}

For whatever reason, when tabindex is set on the body and you click inside the panel but not on one of the nested inputs, path is equal to [body, html, document, window], which does not include this.element. This causes getTabbableElements to be run. When a sl-select is also in the drawer, it finds ~68k elements, causing the page to hang.

To make it easier to repro, I set a border on the tab panel. Clicking the white space inside the border in Chrome will cause the issue.

This is really a "perfect storm" issue, since the following need to be true:
EDIT: not entirely accurate, see my next comment.

• tabindex has to be set on the body
• browser has to be Chromium-based
• tab panel has be inside the drawer
• something with a lot of nested elements like sl-select also has to be inside the drawer

[brgrz]

Great work! Do you know what Chromium code does different than FF so that this becomes an issue?

[geoffrich]

In Firefox, when you click inside the tabpanel whitespace, path includes the drawer (see below screenshot). In Chromium, path does not include the drawer, so the expensive tabbable elements calculation runs when it doesn't need to.

I just realized that this is not a Chrome-only issue. It will also occur in any browser when the user attempts to tab out of the drawer in the example. In that case, you need to determine tabbable elements so you know where to return focus to. This will happen regardless of tabindex.

Really the only requirement is a sl-select inside an sl-tabpanel inside a sl-drawer. When you attempt to exit the drawer with Tab, getTabbableElements returns 60k+ elements. Removing one of those from the equation on returns 1k-5k elements. That still seems high, but it doesn't cause the browser to hang.

[claviska]

Thanks for the repro, @geoffrich. After investigating this further, I've narrowed it down to the internal getTabbableElements() function. I haven't pinpointed it yet, but it appears to be related to slotted content. It may be recursively gathering references to the same element.

I'll investigate this more next week!

[claviska]

Turns out the logic here wasn't needed since we're also iterating over child elements. This was causing some unintended recursion where the same elements were returned over and over again.

I've verified the fix works for the use case above, as well as with additional nested elements. This fix will land in beta.45.

[brgrz]

Per my observation the issue hasn't been completely resolved with beta.45. I still notice lags from time to time though the handler violation/timeouts no longer occur.

Also I am unable to use double click to select text inside the sl-tabpanels that are inside sl-drawer.

UPDATE: I updated the repo at https://github.com/brgrz/sl-bugs to show the new bug in action.
Chrome version: latest, 91.0.4472.124

[brgrz]

@claviska Did you have time to look at the new issue?

[claviska]

The first commit fixed a recursion bug. If you're still seeing a lag, it's probably due to the getTabbableElements() function calling window.getComputedStyle() on every tabbable candidate.

I've reworked the logic in 9a89c14 to short circuit after finding the first and last tabbable elements. This is in lieu of getting all tabbable elements, which is expensive and probably not useful. To align the internal function with what it actually does, I've renamed it from getTabbableElements() to getTabbableBoundary().

This should solve any remaining lag unless you have a significant number of non-tabbable elements before/after the first and last tabbable elements in your modal.

[brgrz]

@claviska I've updated the repo at https://github.com/brgrz/sl-bugs with updated code on focusin violation.

If you click anywhere on whitespace in Empty or Advanced tabs you'll see focusin lags in console.

You will see that I wrapped sl-drawer with custom components, just so to abstract away the sl-drawer and bc I want that kind of custom Lego bricks for drawer functionality.

I also tried this same setup (same example code, tabs and form) with vanilla sl-drawer and there are no lags. I hope that doesn't mean I have to go back to using sl-drawer directly.

Doing perf auditing I discovered the lags still happen within the getTabbableBoundary() and walk() functions.

[brgrz]

@claviska Is this tabbable logic actually needed on modals? As far as I can see it only function atm is to provide the ability to tab to the closing X button, which is the first tabbable element within the modal.

[claviska]

That logic controls focus trapping behavior. As you tab through an active, modal-capable component, focus must remain "trapped" in the control.

For most dialogs, the expected behavior is that the dialog's tab order wraps, which means that when the user tabs through the focusable elements in the dialog, the first focusable element will be focused after the last one has been reached. In other words, the tab order should be contained within and by the dialog.

Source

For all intents and purposes, a Shoelace drawer is modal just like a dialog is modal, so the same rules apply.

[brgrz]

Please see the updated repo at https://github.com/brgrz/sl-bugs. It is now using the sl-drawer and not my custom slotted drawer. It is a contrived example but not too complex one. Click on items annotated with "click here" and observe the console.

The example mimics the UI I use in an app - I'd open a list of items in drawer and then allow the user to edit those items which again opens another drawer. But only these two levels are present at any time, no further nesting happens. Also I made sure to reuse the drawer for the item we edit.

[brgrz]

I added another commit to make the lags even more apparent. All I had to do is add a couple of divs to the drawer's content slot.

[brgrz]

After more troubleshooting I can confirm this issue remains it's just less obvious and less frequent.

There is also another issue which is the inability to double click to select text inside the tab panels hosted in drawers, reproduced here https://codepen.io/trailrun80/pen/KKvaMaR which is a direct result of having document.body.tabIndex = -1;

@geoffrich
Is SvelteKit's feature to manipulate tabIndex on the body the only way for it to handle focus? I can imagine this could lead to further problems in future.

I'm unsure who should make changes from here on.

[claviska]

Your pen works in Firefox and Safari. It also works in Chrome and Edge if you remove the tabindex on the body. I think what you're seeing is this, which I've narrowed down and filed as a potential Chromium bug.

https://bugs.chromium.org/p/chromium/issues/detail?id=1263444

I've also tried replicating the lag you've described without success. I'm wondering if the inability to select text was making it seem laggy.

In reference to my tweet from last week — even with your SvelteKit-less repro it took me about an hour to narrow it down and file this bug. 🙃

[brgrz]

I'm actually able to select the first sentence by double clicking but not by dragging and selecting 🤷🏻‍♂️

[claviska]

Same, but dragging doesn't work on any blink-based browser I've tried.

[brgrz]

Holy smokes, while there definitely are lags in Chrome (with my private code) I don't run into them in Firefox. It was a browser issue all along. I feel terrible for not trying this out in FF. Chrome has been such a default for me and I didn't run into such issue for years if ever.

[thdoan]

Hi, I noticed this issue has been closed, but people are still seeing lags in Chrome -- is this the current status?

[claviska]

This commit significantly improved performance, and I was under the impression the remaining issue was the linke bug. If there's still a noticeable lag that's unrelated, please file a new bug with a repro so I can take a look.

[claviska]

FYI, further improvements have been contributed in #800 which can significantly reduce lag.