Correct lost/incorrect focus after interactions#312
Conversation
| setTimeout(() => { | ||
| btnEl.innerText = getI18nText('operations.copy'); | ||
| }, 5000); | ||
| btnEl.focus(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Focus is lost because of this:
openapi-explorer/src/utils/common-utils.js
Lines 36 to 41 in 241dc32
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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.