Inconsistent focus behavior selecting tool icons

[bbastings]

Is your feature request related to a problem? Please describe.

When clicking on the select tool icon in a studio test application, it takes focus (although nothing is shown that tells me that until I type something, then it gets a green outline). This is inconsistent with choosing other tools where focus stays/moves to Home. I assume the difference is that I’m not picking a tool from a sub-menu.

Stuff like this is the reason people start trying to introduce “editable” checks to shortcut processing. Would be great if we can keep things simple, you can use shortcuts when focus is Home or AccuDraw; but we need to do better managing focus, showing the user where focus is, and making it easier to move focus (ex. like how you can now use Esc to focus Home now).

Describe the Solution you'd like

The in-view tools in particular should ensure focus is always Home.

Describe alternatives you've considered

No response

Additional context

No response

[bbastings]

I believe this was the exact same issue (tool button not from sub-menu). In this case it seemed appropriate for the navigate tool to move focus to home since its entire reason to exist was to support WASD to control the camera.
Focus issue with LookAndMoveTool

[mathieu-fournier]

I'm not sure this is desirable since W3C Guidelines states that this is working as designed:

By default, in all modern browsers, this button does not show any focus indication when activated using a mouse/pointer. Browsers show their default focus outline/indication when a user sets focus on the button using Tab.

Performing a shortcut after activating a tool seems reasonable without additional steps.
The question really is, why would a user want to have a button visibly focused after clicking it?
In most cases, after clicking, the user is done with the button and ready for the next action.
I agree that we should try to keep it simple, but not at the expense of user experience.

As for the solution :
The best way I could think of to handle this is to inspect where focus-visible is and act accordingly.
When spacebar is pressed, we check is the focus visible on a button? Yes -> activate button, No -> activate shortcut.
This solution is indeed more complex, but it follows web standards and is great for users.
The good new, it's already build and isolated in a package! Take a look.

[bbastings]

When spacebar is pressed, we check is the focus visible on a button? Yes -> activate button, No -> activate shortcut.

I don't think doing anything with keyboard events on the capture phase is a good idea, which is what I assume you are doing.

The question really is, why would a user want to have a button visibly focused after clicking it?

I don't want to change when/how focus is shown for buttons, I don't want the in-view tool icon buttons to take focus when I click on them. This is not the least bit useful. If you navigated to the button using the keyboard, then sure, you can use "space" to run the tool.

That said, I just want it to work better than what we have currently and not have everyone doing their own thing.

[mathieu-fournier]

I don't want the in-view tool icon buttons to take focus when I click on them.

Sorry, I thought this is what you meant by "we need to do better managing focus, showing the user where focus is". I thought this was a proposition, not an open-ended statement. I agree with you if there is a simpler alternative, let's use it.

I don't think doing anything with keyboard events on the capture phase is a good idea, which is what I assume you are doing.

It's what I'm doing in very precise scenarios, only to prevent specific issues. I dont think it'll have much negative impact, but if it proves to create more problems than it solves, we might need something else yea.

[GerardasB]

I assume the difference is that I’m not picking a tool from a sub-menu.

The observed difference between activating sub-menu and toolbar items is surfaced due to how DropdownMenu component is implemented in iTwinUI: unfortunately iTwinUI will does not return the focus to the trigger, when a menu item is clicked. IMO this is just a shortcoming in iTwinUI implementation (even if not explicitly required), as both legacy StrataKit and MUI menus return the focus to the trigger.

I don't want to change when/how focus is shown for buttons, I don't want the in-view tool icon buttons to take focus when I click on them

Messing around with a focus like that is generally not a good idea. Users expect that clicking a button will move a focus https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/button#clicking_and_focus (this is even patched for MacOS in both iTwinUI and MUI for consistency between browsers).

The root cause for even requiring this workaround is incorrect assumption, that the shortcut system should only handle shortcuts when focus is "home or AccuDraw". This is not the case with https://github.com/iTwin/appui/blob/master/ui/appui-react/src/appui-react/tools/FrameworkToolAdmin.ts

Additionally, there are usually more interactive elements (outside the toolbars) on the page, which seem to not be covered by this issue.

[bbastings]

@GerardasB I'm happy to change ToolAdmin.processKeyboardEvent (processShortcutKey gate keeper) to check what you think is appropriate. Before the strict Home/AccuDraw checks were added shortcuts were getting processed while trying to type into text fields, like the key-in field.

  protected processKeyboardEvent(keyEvent: KeyboardEvent, _wentDown: boolean): boolean {
    if (this._isFocusHome || undefined !== IModelApp.accuDraw.getFocusItem())
      return false; // Focus is Home or AccuDraw, allow shortcuts...

    // NOTE: Provide a convenient way for the user to move focus to Home to use shortcuts.
    // Escape is the only practical choice with its default behavior that can cancel/close/blur.
    // Intentionally not checking wentDown as some ui elements stop propagation of down but not up (moving to capture is not a good option).
    // Apps that want to use Escape to start the default tool still can with Home focus in processShortcutKey.
    if (keyEvent.key === "Escape" && ToolSettings.escapeMovesFocusToHome && !keyEvent.defaultPrevented && !keyEvent.isComposing)
      this._setFocusHome();

    return true;
  }

    if (!wentDown) return false; // This check still belongs in processShortcut...
    if (UiFramework.isContextMenuOpen) return false; // Can't check this in core?
    if (e.key === Key.Escape.valueOf()) return false; // Nice being able to focus home with ESC when in editable target that didn't eat the event...if you think this isn't a good idea, how does the user move focus?
    if (isElement(e.target) && isEditable(e.target)) return false; // Ok to add to core...

both legacy StrataKit and MUI menus return the focus to the trigger.

Will this be fixed then to be consistent?

Additionally, there are usually more interactive elements (outside the toolbars) on the page, which seem to not be covered by this issue.

Yes, I was less concerned about that as you will always have the situation where it's an editable element too requiring users to manually move focus. If explicitly moving focus is required less often great as long as it's clear to the user when shortcuts can be used.

@mathieu-fournier Are you aware of situations that would not be handled by the checks in FrameworkToolAdmin?

[GerardasB]

to check what you think is appropriate

I'd expect the isEditable(e.target) check to be the most important (so that shortcuts are not invoked when typing into an input). Additionally, allow consumers to override the defaults & further customize (which is probably already possible by overriding the methods).

if (e.key === Key.Escape.valueOf()) return false; can probably be removed in AppUI since it's handled in core?
if (UiFramework.isContextMenuOpen) return false; currently this is not reliable, as it checks for specific internal class name.

appui/ui/appui-react/src/appui-react/UiFramework.ts

Line 701 in f40471c

const contextMenu = document.querySelector("div.core-context-menu-opened");

This should be more general, e.g. checking for element with https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/dialog_role

Will this be fixed then to be consistent?

@mayank99 is submenu item focus behavior worth fixing over in iTwinUI or should I address this for toolbars only in AppUI?

[mathieu-fournier]

@mathieu-fournier Are you aware of situations that would not be handled by the checks in FrameworkToolAdmin?

Which version, these checks are extensively overriden in our stack.

This is the trace we have for the Input field focus case :

1. ToolAdmin recently added a special case, only for accudraw. This, in combination with focus on home, would correctly handle the input field focus case.

2. AppUI's overrides this and block shortcuts when accudraw has focus, because of the isEditable check.

3. Studio's overrides it again, removing the check to allow shortcuts using accudraw (see the comment). This would allow shortcuts to trigger when typing in input fields tho, a bug.

4. Opensite+ re-extends it, doing a similar check than ToolAdmin, to allow certain shortcuts when accudraw has focus.

5. Bonus :
   Opensite+, now trying the keyboard package, have shorctuts disabled in input fields by default. Accudraw Shortcuts add a edge case, enabling themselves only when accudraw input field are focused.

Other cases are when a modal is visible, button focus, highlighted text (conflicting with Ctrl-C), dragging operations, IME composition, when the wrong frontstage is loaded, DOM elements undo/redo, ...
Every case I know is listed here, every middlewares have a short text explaining which individual case they handle.

[mayank99]

@mayank99 is submenu item focus behavior worth fixing over in iTwinUI or should I address this for toolbars only in AppUI?

I'd say it's better handled on consumer end (AppUI in this case). Consumers always have more information about the correct destination for focus after activating a menu-item. Plus, focusing the menu's trigger in the base iTwinUI component could conflict with consumers' focus handling if they were relying on the previous behavior.

[karolis-zukauskas]

@bbastings @GerardasB @mathieu-fournier
I had the impression that "Escape" key exited the active Tool (acting like a "cancel" from users perspective) and that was part of core logic - seems like I was wrong and this behaviour was implemented with overrides to ToolAdmin and a UiFramework shortcut on app side.

What do you think about bringing this to core - "Escape" exits active tool.

[bbastings]

The UX folks should be consulting us (in case of technical issues) and making these product decisions...

In MicroStation, Esc starting the default/selection tool is a preference (defaults to off). As long as Esc starting the default tool only applies when focus is Home, it's "fine". Personally, I don't find it that useful as tools are most commonly used in a verb/noun fashion and you don't usually pre-select the elements before starting a tool; but I know some like it (ACAD users?).

It's very important that Esc can move focus off an "editable" component (ex. key-in field in dta) to support being able to use any shortcut. The user would benefit from a visible indication that focus is Home so they can know how their keystroke will be interpreted...

[karolis-zukauskas]

@andypham-bentley do we have insight from past user workshops about expected "Escape" key behaviour?