Skip to content

Fix memory leak due to focus listeners not being deleted #143

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.

Sign up for GitHub

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

Merged
cmorten merged 2 commits into from
May 19, 2025
Merged

Fix memory leak due to focus listeners not being deleted #143

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
89998aa
Fix memory leak due to focus listeners not being deleted
ysgit May 19, 2025
46e4311
Remove event listener on container on stop
ysgit May 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 14 additions & 26 deletions src/Virtual.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ export class Virtual {
#spokenPhraseLog: string[] = [];
#treeCache: AccessibilityNode[] | null = null;
#disconnectDOMObserver: (() => void) | null = null;
#boundHandleFocusChange: ((event: Event) => Promise<void>) | null = null;

#checkContainer() {
if (!this.#container) {
Expand Down Expand Up @@ -257,8 +258,6 @@ export class Virtual {

this.#treeCache =
this.#container && tree ? flattenTree(this.#container, tree, null) : [];

this.#attachFocusListeners();
}

return this.#treeCache;
Expand Down Expand Up @@ -290,28 +289,9 @@ export class Virtual {
}

#invalidateTreeCache() {
this.#detachFocusListeners();
this.#treeCache = null;
}

#attachFocusListeners() {
this.#getAccessibilityTree().forEach((treeNode) => {
treeNode.node.addEventListener(
"focus",
this.#handleFocusChange.bind(this)
);
});
}

#detachFocusListeners() {
this.#getAccessibilityTree().forEach((treeNode) => {
treeNode.node.removeEventListener(
"focus",
this.#handleFocusChange.bind(this)
);
});
}
Comment thread
jlp-craigmorten marked this conversation as resolved.

async #handleFocusChange({ target }: Event) {
await tick();

Expand All @@ -322,10 +302,13 @@ export class Virtual {
return;
}

// We've covered the tree having no length so there must be at least one
// index or we default back to the beginning of the tree.
const newActiveNode =
tree.find(({ node }) => node === target) ?? tree.at(0)!;
// We've covered the tree having no length so there should be at least one
// matching node, but if not we will not update the state
const newActiveNode = tree.find(({ node }) => node === target);

if (!newActiveNode) {
return;
}
Comment on lines +307 to +311

@jlp-craigmorten jlp-craigmorten May 19, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure about changing the logic here... be interested to understand your thoughts?

I think my reasoning was that focusing an element that isn't in the tree is considered somewhat lost focus, so the virtual cursor get's placed on the container (the equivalent of the body if working against the entire page).

Perhaps need to fact check what NVDA, VO, actually do in this scenario to compare. Though, there's no requirement for this lib to mirror them exactly so more relevant to follow anything regarding focus management in the specs (which I don't think there is anything).

I guess this change has the potential to trigger focus events for a superset of what we had before as the focusin applies to the entire container opposed to just nodes in the tree itself... might be frustrating for the cursor to jump to the container every time something outside the tree gains focus? Unsure!

@ysgit ysgit May 19, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a tricky one.

Originally I didn't change this logic and it broke some of the ariaLive tests. For instance this test.

What happened is that the spoke phrase log became

      "document",
      "button, Test aria-live polite",
      "document",
      "polite: Existing Content",
      "polite: Existing Content u"

The reason appears to be that the contenteditable divs are not in the accessibility tree - maybe because they don't have a role? They become focused when you do userEvent.click().

I could modify the test to give them a role but then there's a wider question. Shouldn't everything in the focus sequence be in the accessibility tree? If I understand correctly, that's why you can't use aria-hidden on something in the focus sequence.

Another situation where this could occur is on a nested interactive. In that scenario I think the right thing would be to keep the screen reader cursor on the parent interactive.

@jlp-craigmorten jlp-craigmorten May 19, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, interesting!

In the test case you've shared the contenteditible div have an implicit generic role which does allow for aria-live so the live region support is correct. What this lib hasn't done to date is consider whether something being focusable should result in it's inclusion in the tree, currently all generic elements are ignored and effectively replaced by their children... perhaps there are cases where that shouldn't happen 🤔 (e.g. when focusable)

@cmorten cmorten May 19, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s take this away and handle separately - your changes appear to have passed current test suite so feels like a positive change to get in and iterate on


this.#updateState(newActiveNode, true);
}
Expand Down Expand Up @@ -619,6 +602,10 @@ export class Virtual {
return;
}

this.#boundHandleFocusChange = this.#handleFocusChange.bind(this);

this.#container.addEventListener("focusin", this.#boundHandleFocusChange);

this.#updateState(tree[0]);

return;
Expand Down Expand Up @@ -647,6 +634,7 @@ export class Virtual {
*/
async stop() {
this.#disconnectDOMObserver?.();
this.#container?.removeEventListener("focusin", this.#boundHandleFocusChange);
this.#invalidateTreeCache();

if (this.#cursor) {
Expand All @@ -658,7 +646,7 @@ export class Virtual {
this.#container = null;
this.#itemTextLog = [];
this.#spokenPhraseLog = [];

this.#boundHandleFocusChange = null;
return;
}

Expand Down
Loading