Skip to content

Correct lost/incorrect focus after interactions#312

Merged
wparad merged 2 commits into
Authress-Engineering:release/2.4from
ahamelers:accessibility/focus
Jul 15, 2026
Merged

Correct lost/incorrect focus after interactions#312
wparad merged 2 commits into
Authress-Engineering:release/2.4from
ahamelers:accessibility/focus

Conversation

@ahamelers

@ahamelers ahamelers commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Currently, in keyboard navigation and generally, when the copy button is clicked, focus is lost (it goes to an element that is added and then immediately removed from the page). The expectation is for focus to remain on this button.

Also currently, navigation links in the side nav load new content and scroll to that content without a new page load. However, focus remains on the link that triggered the content to appear, instead of moving to the new content. The expectation for keyboard navigation/screen readers is that the focus will move to the top of the new content (the same element that is scrolled into view).

This PR corrects both issues by setting focus on the correct elements in these cases.

Comment thread src/utils/common-utils.js Outdated
setTimeout(() => {
btnEl.innerText = getI18nText('operations.copy');
}, 5000);
btnEl.focus();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we quickly talk about the "why focus is lost", what is causing the bad behavior in the first place? Or said differently, what would have to change so that we wouldn't need to add this line of code?

@ahamelers ahamelers Jul 15, 2026

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.

Focus is lost because of this:

const textArea = document.createElement('textarea');
textArea.value = data;
textArea.style.position = 'fixed'; // avoid scrolling to bottom
document.body.appendChild(textArea);
textArea.focus();
textArea.select();

This copy function creates a stealthy textarea with what is to be copied inside of it, puts that on the page, and then focuses on it in order to select the text. Then, it deletes the textarea after this copying is performed. The focus has then disappeared from the page along with the element.

You would not need to refocus on the button, if the entire function was changed to use a different method of copying, probably something similar to this example from the clipboard API https://developer.mozilla.org/en-US/docs/Web/API/Clipboard/writeText#examples

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It looks like we are only using it in one spot, please verify, and that's in the syntax-highlighter. And we are only passing this.content.toString() or JSON.stringify(this.content), which means we are always sending plaintext to it.

I think we should delete this method, and use the clipboard api directly in the syntax-highlighter.

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.

Okay, do you want me to do this in this PR or in a new one? I also need to adjust some aria stuff so the 'copied' success message is provided to screen readers, if that affects your choice.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Whatever you think is best. Again, I don't know what you are envisioning so that's always a judgment call you will have to make. I can only offer advice after the fact. If decisions like this are challenging to figure out, I'm happy to help walk you through how best to think about them.

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.

I am mainly considering the request to do one PR at a time right now. Is this one acceptable for merging so I can move on to that work?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We shouldn't add code like btnEl.focus(); just to remove it in a followup, that's the only thing that doesn't make sense. Whether you do the correct fix for the clipboard here or in another PR is up to you.

@wparad wparad merged commit 9f97996 into Authress-Engineering:release/2.4 Jul 15, 2026
2 checks passed
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