APP-7268: Add nav dropdown #615

anjinai · 2025-01-06T19:44:02Z

Overview

This PR introduces a new NavDropdown component that will be used in app to navigate between different versions of fragments. This dropdown allows for version selection while displaying additional data like timestamps and descriptions.

Ticket: https://viam.atlassian.net/browse/APP-7268
Figma: https://www.figma.com/design/g7uvrGb1Wwdasr4tw6kPpu/Fragments?node-id=6231-12788&t=3olT5Jeslga6pn75-4

packages/core/src/lib/nav-dropdown/nav-dropdown.svelte

mcous

Additional notes from in-person

packages/core/src/lib/nav-dropdown/nav-dropdown.svelte

anjinai · 2025-01-15T16:47:15Z

packages/core/src/lib/nav-dropdown/nav-dropdown.svelte

+    } else if (activeIndex >= 0) {
+      const selectedOption = options[activeIndex];
+      if (selectedOption) {
+        window.location.href = selectedOption.href;


I think my confusion around using app/navigation stemmed from trying to navigate to the appropriate href when someone hit the enter key. Is window.location appropriate here?

I think this is on the right track, but the short version is:

We should minimize or eliminate any navigation via JavaScript

If we do need to navigate, we should pass navigate in as a prop so we don't accidentally bail out of an app's client-side navigation

The key here is to make sure that we hook into native browser behaviors as much as possible and only add keydown handlers for stuff that's extra. I've just spent some time experimenting, reading through MDN, and reading through the navigation menubar accessibility spec, and this is what I've seen:

Native browser behaviors:

Enter

Button: trigger click handler

Link: follow link and trigger click handler

Conclusion: we don't need any Enter handlers

Space

Button: trigger click handler

Link: nothing

Conclusion: we might want add a Space handler on <a> elements

After some research, trying to navigate on Space will be tricky so we probably just want to skip it

Escape

No native behaviors related to <button> nor <a>

No a11y needed for <button> - if menu is open, focus is on one of the link elements, not the button

Need to close the menu if focus is on an <a> and Escape is pressed

Conclusion: we need an Escape handler on the link elements

Arrow keys

No native behaviors related to <button> nor <a>

No a11y behaviors needed for <button> - if menu is open, focus is on one of the link elements, not the button

Need to move focus between <a> elements on ArrowDown, ArrowUp

Conclusion: we need ArrowUp/Down handlers on the <a> elements to move focus

mcous

Very happy with this PR in terms of the visuals and semantics! Keyboard navigation isn't in an accessible state yet and is going to take some more work and experimentation. Rather than drag this PR out, let's punt on the keyboard interactions so we can merge the stuff that's working well

Keyboard nav in these sorts of components is not an easy thing to get "right;" all of our existing components have one deficiency or another in this department.

With the structure set and tests in place, it'll be easier to followup with keyboard interactions in isolation. Plus, in the intermediate state, the menu will be keyboard navigable with tab + enter. This does not follow the menubar a11y recommendations, but is better than nothing!

packages/core/package.json

mcous · 2025-01-15T17:40:38Z

packages/core/src/lib/nav-dropdown/nav-dropdown.svelte

+    } else if (activeIndex >= 0) {
+      const selectedOption = options[activeIndex];
+      if (selectedOption) {
+        window.location.href = selectedOption.href;


I think this is on the right track, but the short version is:

We should minimize or eliminate any navigation via JavaScript

If we do need to navigate, we should pass navigate in as a prop so we don't accidentally bail out of an app's client-side navigation

The key here is to make sure that we hook into native browser behaviors as much as possible and only add keydown handlers for stuff that's extra. I've just spent some time experimenting, reading through MDN, and reading through the navigation menubar accessibility spec, and this is what I've seen:

Native browser behaviors:

Enter

Button: trigger click handler

Link: follow link and trigger click handler

Conclusion: we don't need any Enter handlers

Space

Button: trigger click handler

Link: nothing

Conclusion: we might want add a Space handler on <a> elements

After some research, trying to navigate on Space will be tricky so we probably just want to skip it

Escape

No native behaviors related to <button> nor <a>

No a11y needed for <button> - if menu is open, focus is on one of the link elements, not the button

Need to close the menu if focus is on an <a> and Escape is pressed

Conclusion: we need an Escape handler on the link elements

Arrow keys

No native behaviors related to <button> nor <a>

No a11y behaviors needed for <button> - if menu is open, focus is on one of the link elements, not the button

Need to move focus between <a> elements on ArrowDown, ArrowUp

Conclusion: we need ArrowUp/Down handlers on the <a> elements to move focus

mcous · 2025-01-15T17:45:12Z

packages/core/src/lib/nav-dropdown/nav-dropdown.svelte

+            class="relative flex items-center px-2 py-1.5 hover:bg-gray-1"
+            class:bg-gray-1={i === activeIndex}
+            role="menuitem"
+            aria-current={i === activeIndex ? 'page' : 'false'}


issue: this looks like it confuses "active" and "current"

active - which link is currently highlighted as the user keyboard navigates through

current - which link matches the current URL

I think something like this is what we need

Suggested change

aria-current={i === activeIndex ? 'page' : 'false'}

aria-current={href === selectedHref ? 'page' : 'false'}

Whoops, thanks for the correction

mcous · 2025-01-15T17:46:40Z

packages/core/src/lib/nav-dropdown/nav-dropdown.svelte

+          <a
+            {href}
+            class="relative flex items-center px-2 py-1.5 hover:bg-gray-1"
+            class:bg-gray-1={i === activeIndex}


suggestion (keyboard): Should we move this styling to a focus:bg-gray-1 instead? That way, once we set up focus via keyboard events properly, it will all work

mcous · 2025-01-15T17:48:17Z

packages/core/src/lib/nav-dropdown/nav-dropdown.svelte

+export let selectedHref: string;
+
+let isOpen = false;
+let activeIndex = -1;


issue (keyboard): when isOpen becomes true, the WAI example moves focus move automatically to the first item. Should we make sure we set activeIndex to 0 when isOpen becomes true?

Updated, makes sense, ty!

mcous · 2025-01-15T20:13:41Z

packages/core/src/lib/nav-dropdown/__tests__/nav-dropdown.spec.ts

+      expect(screen.getByRole('menu')).toBeInTheDocument();
+    });
+
+    it('navigates options with arrow keys', async () => {


issue (keyboard): we'll to make sure this test tracks focus state and tabindex to ensure the different <a> elements receive focus as the user presses the arrow keys

For context, there are a few strategies one can use to deal with the "active" item when keyboard navigating through a UI like this. The WAI calls out two strategies:

Managing Focus Within Components Using a Roving tabindex

Easier option

Sticks to browser defaults more closely

Managing Focus in Composites Using aria-activedescendant

Much harder to get right

Required if the the visually focused element needs to be different than the native browser focused element

(2) is used in <SearchableSelect>, which has the constraint of requiring browser focus to stay in the text input so the user can search, while visual focus moves through the select items. We have no such constraint in this component, which is a simple menu of links, so we should use (1)

mcous · 2025-01-15T20:16:28Z

packages/core/src/lib/nav-dropdown/nav-dropdown.svelte

+    bind:this={buttonElement}
+    class="relative z-[2] h-7.5 w-full grow appearance-none border border-light bg-white py-1.5 pl-2 pr-1 text-xs leading-tight outline-none group-hover:border-gray-6"
+    on:click={toggleDropdown}
+    on:keydown={handleKeydown}


issue (keyboard): it'll be much harder (though not impossible!) to get keyboard interactions to be accessible if we only attach keyboard handlers to the main control button - thus requiring the browser's focus to remain on the button at all times. Instead, I think this will be easier if we:

Move focus to the <a> menu-items when the menu is open

Attach the keyboard handlers we need to the menu-items rather than the control button

We should be able to get all the control button interactions we need with just default browser behaviors and the on:click handler

packages/core/src/lib/nav-dropdown/nav-dropdown.svelte

mcous · 2025-01-15T20:21:49Z

packages/core/src/lib/nav-dropdown/nav-dropdown.svelte

+      <div
+        class="w-full overflow-auto border border-gray-6 bg-white shadow-sm focus:outline-none"
+        role="menu"
+        use:clickOutside={closeDropdown}


issue (annoying clickOutside shenanigans): IIRC this may prevent you from using the open/close button to toggle the menu closed, because clicking the control button counts as a "click outside," racing the click outside behavior with the button's own toggle behavior.

You may need to add a little guard to this use:clickOutside handler to avoid running if the clicked element is inside buttonElement. For reference, I think <FloatingMenu> in the app has to deal with this

Oh, I didn't even realize that was an issue. Thanks, updated to handle it like floating menu does.

packages/core/src/lib/nav-dropdown/nav-dropdown.svelte

anjinai added 6 commits January 6, 2025 14:43

APP-7268 - create nav dropdown in prime

3978676

add some tests

5e187bd

format

f5d5dbe

lint

a3807c4

lint

9e2850f

import from lib

6de6664

anjinai requested review from ehhong and mcous January 9, 2025 18:02

anjinai commented Jan 9, 2025

View reviewed changes

packages/core/src/lib/nav-dropdown/nav-dropdown.svelte Outdated Show resolved Hide resolved

anjinai commented Jan 9, 2025

View reviewed changes

packages/core/src/lib/nav-dropdown/nav-dropdown.svelte Outdated Show resolved Hide resolved

mcous reviewed Jan 13, 2025

View reviewed changes

packages/core/src/lib/nav-dropdown/nav-dropdown.svelte Outdated Show resolved Hide resolved

packages/core/src/lib/nav-dropdown/nav-dropdown.svelte Outdated Show resolved Hide resolved

packages/core/src/lib/nav-dropdown/nav-dropdown.svelte Outdated Show resolved Hide resolved

anjinai added 7 commits January 14, 2025 14:03

mc feedback and update tests for changes

30e96b9

missed adding selected href to playground version

5c57033

update browserslist - linter said to update

0eda097

format

fac224a

think i got the err

75ea478

fix navigating on enter

75cea77

div update so description is in the right place

df1fdd5

anjinai commented Jan 15, 2025

View reviewed changes

anjinai requested a review from mcous January 15, 2025 16:52

mcous reviewed Jan 15, 2025

View reviewed changes

anjinai added 4 commits January 21, 2025 16:36

some mc changes

8ae0b22

ugh this component isnt working

5722c39

merge main bump v

eea4482

format

a9727c8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

APP-7268: Add nav dropdown #615

APP-7268: Add nav dropdown #615

anjinai commented Jan 6, 2025 •

edited

Loading

mcous left a comment

anjinai Jan 15, 2025

mcous Jan 15, 2025

mcous left a comment

mcous Jan 15, 2025

mcous Jan 15, 2025

anjinai Jan 21, 2025

mcous Jan 15, 2025

anjinai Jan 21, 2025

mcous Jan 15, 2025

anjinai Jan 21, 2025

mcous Jan 15, 2025

mcous Jan 15, 2025

mcous Jan 15, 2025

anjinai Jan 21, 2025

	aria-current={i === activeIndex ? 'page' : 'false'}
	aria-current={href === selectedHref ? 'page' : 'false'}

APP-7268: Add nav dropdown #615

Are you sure you want to change the base?

APP-7268: Add nav dropdown #615

Conversation

anjinai commented Jan 6, 2025 • edited Loading

Overview

mcous left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcous left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anjinai commented Jan 6, 2025 •

edited

Loading