Keyboard nav for extensions main screen

[aalves08]

Summary

Fixes #12785

Occurred changes and/or fixed issues

• Add keyboard navigation for Extensions main screen (normal extensions interface + image catalog load -> check extensions menu)
• Refactor focus traps to composables
• Add focus traps to various dialogs/modals on Extensions screen
• Improve LabeledInput accessibility

Technical notes summary

Areas or cases that should be tested

Areas which could experience regressions

Screenshot/Video

Checklist

• The PR is linked to an issue and the linked issue has a Milestone, or no issue is needed
• The PR has a Milestone
• The PR template has been filled out
• The PR has been self reviewed
• The PR has a reviewer assigned
• The PR has automated tests or clear instructions for manual tests and the linked issue has appropriate QA labels, or tests are not needed
• The PR has reviewed with UX and tested in light and dark mode, or there are no UX changes

[rak-phillip]

In pkg/rancher-components/src/components/Card/Card.vue, we do the following

  setup(props) {
    if (props.triggerFocusTrap) {
      basicSetupFocusTrap('#focus-trap-card-container');
    }

Is there any reason why we can't do the same here? It looks like we would be able to drop the modalVisibility prop if we were to achieve this.

[aalves08]

Card component uses basicSetupFocusTrap, which only relies on Vue lifecycle hooks like onMounted and onBeforeUnmount.

This is a watcher-based implementation of the focus trap, because the base component here is already mounted (not triggered by a v-if) meaning that it needs some variable to be watched in order to achieve the correct behaviour.

[rak-phillip]

I honestly don't think that we need the watcher-based approach. Each instance of app-modal that specifies :trigger-focus-trap="true" is conditionally rendered, making the :modal-visibility prop redundant. For example, shell/components/Dialog.vue:

  <app-modal
    v-if="!closed"
    :name="name"
    height="auto"
    :scrollable="true"
    :modal-visibility="!closed"
    :trigger-focus-trap="true"
    @close="closeDialog(false)"
    @before-open="beforeOpen"
  >

Since app-modal is conditionally rendered in each instance represented in this PR. We should be able to rely on the simpler basicSetupFocusTrap implementation. I gave this a quick test and it appears to be working:

diff --git a/shell/components/AppModal.vue b/shell/components/AppModal.vue
index 6877bdb505..04f4281c60 100644
--- a/shell/components/AppModal.vue
+++ b/shell/components/AppModal.vue
@@ -1,6 +1,6 @@
 <script lang="ts">
 import { defineComponent } from 'vue';
-import { watcherBasedSetupFocusTrap } from '@shell/composables/focusTrap';
+import { basicSetupFocusTrap } from '@shell/composables/focusTrap';

 export default defineComponent({
   name: 'AppModal',
@@ -58,13 +58,6 @@ export default defineComponent({
       type:    String,
       default: '',
     },
-    /**
-     * Modal visibility (used for focus-trap)
-     */
-    modalVisibility: {
-      type:    Boolean,
-      default: false,
-    },
     /**
      * trigger focus trap
      */
@@ -100,9 +93,9 @@ export default defineComponent({
       };
     }
   },
-  created() {
-    if (this.triggerFocusTrap) {
-      watcherBasedSetupFocusTrap(() => this.modalVisibility, '.modal-container');
+  setup(props) {
+    if (props.triggerFocusTrap) {
+      basicSetupFocusTrap('.modal-container');
     }
   },
   mounted() {

Is there any modal in this PR that would fail with this approach?

[aalves08]

You are right! I must have switched this stuff so many times that I just got confused. AppModal IS conditionally rendered! therefore this works! we can do the change

[aalves08]

We do have an issue which I wonder if it's due to the way AppModal works (teleport).

By default, the focus-trap lib detects where the focus was before activating the actual focus trap. When the trap get's deactivated, it transfers the focus back to this element. It's a really nice way of getting back to the previous context of where the key navigation was before it got "sucked" into focus trap element.

There's this option for activate called setReturnFocus where we can override this auto-detected starting focus, if you want to send the key navigation back to some other context.

If we don't explicitly set setReturnFocus on the AppModal activate function, what the lib auto-detects is document.body, meaning at activation time it has no idea where the focus initially was 💦

I had to tamper with the source code of the lib to see what target he detected and here's the console.log (check state.nodeFocusedBeforeActivation property):

The problem we are seeing here is that even if we override it with setReturnFocus on these extension modals, we might end in one of two scenarios:

• if the user performs an action (say install) the modal is dismissed but the install button also disappears, meaning that if you want to return to that install button, it can't and returns an error.
• however, when a user goes into a modal, he can dismiss it without performing any action. In the case of install, the install button would be there and would be a valid target 💦 💦 💦 💦 💦

I've spent hours between yesterday and today looking into this and frankly there's no solution without involving what I deem as over-the-top logic for something that should be simpler and kind of straightforward.

My suggestion here is that I create an issue to follow-up on this topic and move this forward.

At this moment I see no easy way out.

Let me know if you want to have a quick sync on this.

[rak-phillip]

The problem we are seeing here is that even if we override it with setReturnFocus on these extension modals, we might end in one of two scenarios:

if the user performs an action (say install) the modal is dismissed but the install button also disappears, meaning that if you want to return to that install button, it can't and returns an error.
however, when a user goes into a modal, he can dismiss it without performing any action. In the case of install, the install button would be there and would be a valid target 💦 💦 💦 💦 💦

We could declare some shared state to specify a default target if the underlying DOM output changes as a result of activating a modal or taking action in a modal. I haven't tested this, but we might be able to utilize the checkCanReturnFocus() method in conjunction with setReturnFocus() to specify a fallback. In your example above, that might be the active tab or the first element in the list. For action buttons, we would need to return focus to the button the triggered the menu.

[aalves08]

Ok, so I've updated the PR with a solution, which is the best that we can do.

After we set a setReturnFocus on trap activation, we can't change it anymore. Given that fact, the condition to evaluate where to move the focus to has to be baked in this method. I thought to redirect the user for the install/uninstall scenario to the reload button if it was visible, but at the time of trap deactivation the button is never visible due to the async nature of helm charts operations.

The library has some limitations and our architecture poses some challenges as well.

Next best thing is to send the user to the first iterable key nav item in the extensions page, if the proceed with the install/uninstall operation. If they don't perform any action, it goes to the install/uninstall button they pressed before.

I've given so many hours to this and I can't come up with anything that would work "ideally". I think we will just need to accept the fact

[rak-phillip]

I still see some inconsistent behavior with dropdown menus that trigger modals. For example:

• Navigate to Extensions
• Click on Extensions Menu and click on Add Rancher Repositories
• Dismiss the menu

Instead of the Extensions Menu trigger receiving focus, it looks like this gets reset to the root document. I think that it makes sense to create an issue so that we can follow-up and explicitly address these scenarios.

[rak-phillip]

I still see some inconsistent behavior with dropdown menus that trigger modals. For example:

• Navigate to Extensions
• Click on Extensions Menu and click on Add Rancher Repositories
• Dismiss the menu

Instead of the Extensions Menu trigger receiving focus, it looks like this gets reset to the root document. I think that it makes sense to create an issue so that we can follow-up and explicitly address these scenarios.

[aalves08]

#13176 (comment)

I've updated the Developer install and add extension repos modals. I think that covers it all if I am not mistaken 🙏

[rak-phillip]

Functions well and does what we need for now. Can we create an issue to follow up the design of how we set return focus? I think we're missing some component interaction that will pull everything together in a clean and predictable way.

[rak-phillip]

This is fine for now, but I still think there's a design problem that we need to solve if we plan on using this more in the future. returnFocusSelector requires that components like DeveloperInstallDialog have knowledge of parent or sibling components. IMO it would be best if we could have some localized data store that will allow us to specify the returnFocusSelector closer the the action that will be triggering the modal.