fix(ui-top-nav-bar,ui-drilldown): fix Drilldown's and TopNavBar's key… by ToMESSKa · Pull Request #1975 · instructure/instructure-ui

ToMESSKa · 2025-05-15T12:47:43Z

…board navigation issues

INSTUI-4494

ISSUES:

in Drilldown and TopnavBar, the first option is not focused when the Drilldown is opened with the keyboard
the screen reader cannot return focus to the Drilldown trigger when user is on the first item of a Drilldown

TEST PLAN:

with keyboard navigation only (no screenreader):

test the 'Admin' menu from the TopNavBar playground
when opening the 'Admin" menu, the first option should be focused and highlighted (previously the first item was not highlighted)
you should be able to close the menu with ESC and the focus should return to the 'Admin' button
test the examples in DrillDown that have a trigger button the same way
they should behave the same way as the 'Admin' dropdown (highlighting first available options, returning focus to the triggers)
test the SmallViewPort version of the TopNavBar playground too, it should behave the same way (highlighting first avaliable option, returning focus to the trigger)

with VoiceOver:

test the 'Admin' menu from the TopNavBar playground
when opening the 'Admin" menu with Control + Options + Tab (or your custom VoiceOver command), the first option should be focused (previously the first item was not focused)
you should be able navigate through the options with Control + Options + Right Arrow/Left Arrow
When the focus is on the first item, keep pressing Control + Options + Left Arrow, you should be able to move back to the admin button (previously the focus jumped to the end of the page because the menu was mounted at the end of the document)
test the examples in DrillDown that have a trigger button the same way
they should behave the same way as the 'Admin' dropdown (focusing the first available option, returning focus to the triggers)
test the SmallViewPort version of the TopNavBar playground too, it should behave the same way (focusing the first available option, returning focus to the trigger)

with JAWS/NVDA:

test the 'Admin' menu from the TopNavBar playground
when opening the 'Admin" menu with Enter, the first option should be focused (previously the first item was not focused)
you should be able navigate through the options with Down Arrow/Up Arrow
When the focus is on the first item, keep pressing Up Arrow, you should be able to move back to the admin button (previously the focus jumped to the end of the page because the menu was mounted at the end of the document)
test the examples in DrillDown that have a trigger button the same way
they should behave the same way as the 'Admin' dropdown (focusing the first available option, returning focus to the triggers)
test the SmallViewPort version of the TopNavBar playground too, it should behave the same way (focusing the first available option, returning focus to the trigger)

with iOS VoiceOver:

test the 'Admin' menu from the TopNavBar playground and the DrillDown examples with a trigger
when opening menu with double tap, the first option should be focused (previously the first item was not focused)
you should be able navigate through the options with swipes
When the focus is on the first item, swipe left, you should be able to move back to the admin button (previously the focus jumped to the end of the page because the menu was mounted at the end of the document)

github-actions · 2025-05-15T12:53:12Z

PR Preview Action v1.6.1
Preview removed because the pull request was closed.
2025-05-29 14:15 UTC

ToMESSKa · 2025-05-15T15:05:28Z

packages/ui-drilldown/src/Drilldown/index.tsx

        }}
        onShowContent={(event) => this.handleToggle(event, true)}
-        mountNode={mountNode}
+        mountNode={mountNode || this.ref}


This solution is based on mounting the DrillDown to the component (not to document.body), allowing us to navigate back from the first item to the trigger button itself.

we should mention this in the release notes, it might break stuff for users

ToMESSKa · 2025-05-15T15:07:09Z

packages/ui-top-nav-bar/src/TopNavBar/README.md

+                        'aria-label': `${item.label} menu`
                      }}


Réka said "open for" is unnecessary and confusing here.

ToMESSKa · 2025-05-15T15:09:27Z

packages/ui-top-nav-bar/src/TopNavBar/TopNavBarLayout/DesktopLayout/styles.ts

      alignItems: 'stretch',
      justifyContent: 'space-between',
      height: componentTheme.desktopHeight,
-      position: 'relative',


these positions and overflows needed to be modified now that Drilldown is no longer mounted to the end of the document, otherwise the styling breaks

Copilot

Pull Request Overview

This PR fixes keyboard navigation and focus issues in both the Drilldown and TopNavBar components. Key changes include:

Updating style rules (changing overflow properties) to prevent focus ring cropping.
Adjusting aria-labels in the TopNavBar to improve accessibility.
Modifying Drilldown behavior to focus the first available item on keyboard invocation and improving mountNode handling.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
packages/ui-top-nav-bar/src/TopNavBar/TopNavBarMenuItems/styles.ts	Adds 'overflow: visible' to prevent focus ring cropping
packages/ui-top-nav-bar/src/TopNavBar/TopNavBarLayout/DesktopLayout/styles.ts	Changes overflow from hidden to visible and removes an unneeded relative positioning
packages/ui-top-nav-bar/src/TopNavBar/README.md	Updates aria-label text for better accessibility semantics
packages/ui-drilldown/src/Drilldown/props.ts	Updates mount node documentation to reflect new default behavior
packages/ui-drilldown/src/Drilldown/index.tsx	Implements keyboard focus handling and adjusts mountNode fallback

packages/ui-drilldown/src/Drilldown/index.tsx

ToMESSKa · 2025-05-15T15:13:11Z

packages/ui-drilldown/src/Drilldown/index.tsx

            as="div"
            elementRef={this.handleDrilldownRef}
-            tabIndex={0}
            css={styles?.drilldown}


We no longer need to focus the entire Popover—just the first item—so this part was removed.

ToMESSKa · 2025-05-20T12:44:28Z

packages/ui-top-nav-bar/src/TopNavBar/README.md


                      {...item.submenu && {
                        renderSubmenu: this.generateSubmenu(item),
-                        'aria-label': `Open for ${item.label} menu`


Réka said aria-label is unnecessary hin these places and advised to remove them

ToMESSKa · 2025-05-20T12:46:34Z

packages/ui-top-nav-bar/src/TopNavBar/TopNavBarLayout/SmallViewportLayout/index.tsx

-        {!this.hasBreadcrumbBlock && !trayMountNode && (
-          <div css={styles?.trayContainer} id={this._trayContainerId} />
-        )}
-


I moved this code after line 466 so that when I'm on the first item of the drill-down menu with SR and step back, the focus returns to the Drilldown's trigger, thereby exiting the DrillDown.

ToMESSKa · 2025-05-20T12:53:36Z

packages/ui-top-nav-bar/src/TopNavBar/TopNavBarLayout/SmallViewportLayout/index.tsx

+  focusFirstAvailableItem(
+    event?:


this handles the focus in SmallPortView

…avBar's keyboard navigation issues INSTUI-4494

HerrTopi

Now it seems fine to me. Good work

matyasf

When I use only keyboard in the playground's smallViewport mode and go into the Admin menu, thew whole menu is selected, is this OK?

ToMESSKa self-assigned this May 15, 2025

ToMESSKa force-pushed the INSTUI-4494_cred_25_ia_20_floating_window branch from 4740d11 to db495b3 Compare May 15, 2025 14:44

ToMESSKa commented May 15, 2025

View reviewed changes

ToMESSKa requested review from HerrTopi, balzss and Copilot May 15, 2025 15:10

Copilot AI reviewed May 15, 2025

View reviewed changes

packages/ui-drilldown/src/Drilldown/index.tsx Outdated Show resolved Hide resolved

packages/ui-drilldown/src/Drilldown/index.tsx Show resolved Hide resolved

ToMESSKa commented May 15, 2025

View reviewed changes

ToMESSKa marked this pull request as draft May 16, 2025 14:47

ToMESSKa force-pushed the INSTUI-4494_cred_25_ia_20_floating_window branch from db495b3 to d4c4b4f Compare May 20, 2025 12:38

ToMESSKa commented May 20, 2025

View reviewed changes

ToMESSKa removed the request for review from balzss May 20, 2025 12:54

ToMESSKa marked this pull request as ready for review May 20, 2025 12:55

ToMESSKa requested a review from joyenjoyer May 20, 2025 12:55

joyenjoyer approved these changes May 22, 2025

View reviewed changes

joyenjoyer self-requested a review May 26, 2025 14:40

fix(ui-top-nav-bar,ui-popover,ui-drilldown): fix Drilldown's and TopN…

4fe3cea

…avBar's keyboard navigation issues INSTUI-4494

ToMESSKa force-pushed the INSTUI-4494_cred_25_ia_20_floating_window branch from 37aabb7 to 4fe3cea Compare May 28, 2025 14:24

ToMESSKa requested a review from matyasf May 29, 2025 07:40

HerrTopi approved these changes May 29, 2025

View reviewed changes

matyasf reviewed May 29, 2025

View reviewed changes

joyenjoyer approved these changes May 29, 2025

View reviewed changes

ToMESSKa merged commit 6d7d3fa into master May 29, 2025
8 checks passed

ToMESSKa deleted the INSTUI-4494_cred_25_ia_20_floating_window branch May 29, 2025 14:14

ToMESSKa mentioned this pull request Jun 17, 2025

fix(ui-top-nav-bar): ensure Drilldown overflows correctly in small vi… #2030

Merged

Conversation

ToMESSKa commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ToMESSKa May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matyasf May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ToMESSKa May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ToMESSKa May 15, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

ToMESSKa May 15, 2025

Choose a reason for hiding this comment

Uh oh!

ToMESSKa May 20, 2025

Choose a reason for hiding this comment

Uh oh!

ToMESSKa May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ToMESSKa May 20, 2025

Choose a reason for hiding this comment

Uh oh!

HerrTopi left a comment

Choose a reason for hiding this comment

Uh oh!

matyasf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ToMESSKa commented May 15, 2025 •

edited

Loading

github-actions bot commented May 15, 2025 •

edited

Loading

ToMESSKa May 15, 2025 •

edited

Loading

matyasf May 29, 2025 •

edited

Loading

ToMESSKa May 15, 2025 •

edited

Loading

ToMESSKa May 20, 2025 •

edited

Loading