Toolbar component #5080

snowystinger · 2023-09-15T11:30:06Z

Closes

Update: Scope reduced, this only supports children of types: Button, Checkbox, Link, RSP ActionGroup

Guidance from accessibility:

ActionGroup should support role="group" so that you can have groups within a toolbar without nesting toolbars.

I've tested this out. If the toolbars are nested, then the outermost label is not read. If the inner toolbars are groups, then both labels are read as expected. This can be seen simply in this demo. I recommend popping out into a new window before using VO.
https://codesandbox.io/s/sleepy-northcutt-hfhwnr?file=/src/App.js

Implementing a subset of https://www.w3.org/WAI/ARIA/apg/patterns/toolbar/examples/toolbar/

Open questions, do we need the RSP component?
Answer: Not at the moment, we don't want this held up by a design review.

extra info: I've created a basic version of it so that we can test interactions with ActionGroup, which is the currently the primary use case. What I've implemented is pretty much what anyone else would need to implement to use it in Spectrum.

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

# Conflicts: # packages/react-aria-components/stories/index.stories.tsx

rspbot · 2023-09-15T11:38:50Z

Build successful! 🎉

rspbot · 2023-09-15T11:40:48Z

Build successful! 🎉

# Conflicts: # packages/react-aria-components/src/index.ts # packages/react-aria-components/stories/index.stories.tsx

rspbot · 2023-09-18T00:56:21Z

Build successful! 🎉

rspbot · 2023-09-18T01:03:02Z

Build successful! 🎉

rspbot · 2023-09-20T04:08:09Z

Build successful! 🎉

packages/react-aria-components/src/Toolbar.tsx

packages/react-aria-components/stories/index.stories.tsx

packages/@react-types/actiongroup/src/index.d.ts

rspbot · 2023-09-20T05:04:33Z

Build successful! 🎉

packages/@react-aria/actiongroup/src/useActionGroup.ts

devongovett · 2023-09-20T17:37:39Z

packages/react-aria-components/src/Toolbar.tsx

+// Any reason not to include ref? i feel like we've been sad about not including it down the line
+// same with state...
+// eslint-disable-next-line @typescript-eslint/no-unused-vars
+function useToolbar(props: ToolbarProps & {isInsideAToolbar: boolean}, ref: ForwardedRef<HTMLDivElement>) {


Any reason not to put this in a @react-aria/toolbar package?

previously you and @dannify had been on the fence about it, happy to move it

packages/react-aria-components/src/Toolbar.tsx

devongovett · 2023-09-20T17:39:46Z

packages/react-aria-components/stories/index.stories.tsx

+        <Radio className={styles.radio} value="right">Right</Radio>
+      </RadioGroup>
+      */}
+      {/* disabled buttons do not work in toolbar yet, the buttons cannot be focused


cannot be? isn't that correct?

based off the aria example, some of the disabled buttons are still focusable for screenreaders

and specifically it says this a little further down the page

While they are HTML button elements, as described in the accessibility features section above, in order to remain focusable when disabled, they have aria-disabled instead of the HTML disabled attribute.

hmm that's very subjective. I would expect to skip over them by default, otherwise that is inconsistent with everything else we currently implement.

devongovett · 2023-09-20T17:40:07Z

packages/react-aria-components/stories/index.stories.tsx

+        <Button isDisabled>Cut</Button>
+      </Toolbar>
+      */}
+      {/* select does not work in toolbar yet, focus can't move "Right Arrow" past it


if our listeners are capturing seems like it should?

Determined what happened, I wasn't specifying to the focusmanager to move to the next tabbable element, so focus was going to the hidden select and then right back to the select button, forever

it brings up a bigger issue though, we have some components with non-tabble elements, radio group, which should be navigable by arrow key, and then we have some components with non-tabbable elements, select, which should not be navigable by arrow key (there is a hidden input)

I'm not sure how to distinguish between the two. Any thoughts?

I tried using our accept function in createFocusManager, but ActionGroup isn't implemented as a radiogroup all the time and its button can look very similar to NumberField's increment/decrement buttons...

I don't want to use the context I introduced here because I'm still hoping to find a way to get rid of it

Maybe we could use the FocusScope context to declare if a particular scope intends to control certain keys for navigation within the scope? Problematic that hooks don't have access to our context.

Or we could put a data-attribute on elements like the NumberField increment button that's like, data-reallyNotTabbable... so i'd have something to filter on, hahaha

devongovett · 2023-09-20T17:40:16Z

packages/react-aria-components/stories/index.stories.tsx

+          </ListBox>
+        </Popover>
+      </Select>*/}
+      {/* numberfield does not work in toolbar yet, focus stops on the stepper buttons


LFDanLu

Initial review, will wrap up after lunch

EDIT: nothing else to comment on that hasn't been touched on by other reviews

packages/@react-types/actiongroup/src/index.d.ts

packages/react-aria-components/src/Toolbar.tsx

rspbot · 2023-09-21T05:34:16Z

Build successful! 🎉

rspbot · 2023-09-25T06:10:45Z

Build successful! 🎉

rspbot · 2023-09-26T03:29:57Z

Build successful! 🎉

snowystinger · 2023-09-26T03:33:02Z

packages/@react-aria/focus/src/FocusScope.tsx

@@ -45,7 +45,8 @@ export interface FocusManagerOptions {
  /** Whether focus should wrap around when it reaches the end of the scope. */
  wrap?: boolean,
  /** A callback that determines whether the given element is focused. */
-  accept?: (node: Element) => boolean
+  accept?: (node: Element) => boolean,
+  preview?: boolean


needed so i could use the same setup and determine where focus would move in both the tabbable and focusable case next before actually committing to it

rspbot · 2023-09-26T03:45:56Z

Build successful! 🎉

rspbot · 2023-09-27T02:13:13Z

Build successful! 🎉

rspbot · 2023-09-27T02:14:22Z

Build successful! 🎉

rspbot · 2023-09-27T03:01:23Z

Build successful! 🎉

rspbot · 2023-09-27T03:11:31Z

Build successful! 🎉

devongovett

lgtm. let's get this into the next testing session.

devongovett · 2023-10-10T00:31:08Z

packages/@react-aria/toolbar/src/useToolbar.ts

+  toolbarProps: HTMLAttributes<HTMLElement>
+}
+
+// Any reason not to include state? i feel like we've been sad about not including a state arg in the past


I can't think of what state a toolbar would have so probably ok. 🤷

devongovett · 2023-10-10T00:33:00Z

packages/react-aria-components/src/Toolbar.tsx

+
+export const ToolbarContext = createContext<ContextValue<ToolbarProps, HTMLDivElement>>({});
+
+function Toolbar(props: ToolbarProps, ref: ForwardedRef<HTMLDivElement>) {


Let's discuss whether we want to temporarily name this UNSTABLE_Toolbar before the next release.

# Conflicts: # packages/react-aria-components/package.json

rspbot · 2023-10-10T03:44:02Z

Build successful! 🎉

rspbot · 2023-10-10T03:44:04Z

## API Changes

unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }

@react-aria/toolbar

AriaToolbarProps

-
+AriaToolbarProps {
+  orientation?: Orientation = 'horizontal'
+}

it changed:

useToolbar

useToolbar

changed by:

AriaToolbarProps

-
+useToolbar {
+  props: AriaToolbarProps
+  ref: RefObject<HTMLDivElement>
+  returnVal: undefined
+}

reidbarber

I noticed when using Up/Down arrow on horizontal, focus stays within the groups (and vice versa for vertical). Is this expected?

snowystinger · 2023-10-10T19:56:37Z

I noticed when using Up/Down arrow on horizontal, focus stays within the groups (and vice versa for vertical). Is this expected?

This is expected https://www.w3.org/WAI/ARIA/apg/patterns/toolbar/ it just happens that ActionGroups use the opposing direction to navigate as well and I just let the child component handle the keys I don't. It's hard to know enough about the children to do anything other than this. For instance, controlling a slider vs navigating a menu vs numberfield spin button. So I think this is an ok interpretation.

reidbarber

LGTM

snowystinger added 2 commits September 15, 2023 21:27

Toolbar component

70c9684

Merge branch 'main' into toolbar-component

ab01193

# Conflicts: # packages/react-aria-components/stories/index.stories.tsx

snowystinger added 2 commits September 18, 2023 10:46

fix accessibility

9ddddfb

Merge branch 'main' into toolbar-component

62fd9e3

# Conflicts: # packages/react-aria-components/src/index.ts # packages/react-aria-components/stories/index.stories.tsx

remove rsp toolbar and fix nested toolbars

db0ee23

fix keyboard navigation

f3c60f3

snowystinger commented Sep 20, 2023

View reviewed changes

packages/react-aria-components/src/Toolbar.tsx Outdated Show resolved Hide resolved

snowystinger commented Sep 20, 2023

View reviewed changes

packages/react-aria-components/stories/index.stories.tsx Outdated Show resolved Hide resolved

snowystinger commented Sep 20, 2023

View reviewed changes

packages/@react-types/actiongroup/src/index.d.ts Outdated Show resolved Hide resolved

devongovett reviewed Sep 20, 2023

View reviewed changes

LFDanLu reviewed Sep 20, 2023

View reviewed changes

remove FocusScope

1226a0d

snowystinger added 7 commits September 22, 2023 21:58

move to queue microtask

06b29c4

fix tests

be9e5a7

Fix React 17 tests

0e06558

fix react 17 tests

2aa5d0a

fix remaining react 17 tests

53318af

fix react 16

e107cee

Merge branch 'main' into toolbar-component

e89e3a0

Fix Numberfield

dd57802

snowystinger commented Sep 26, 2023

View reviewed changes

Merge branch 'main' into toolbar-component

889cd28

snowystinger added 2 commits September 27, 2023 12:02

Reduce scope

8e7940e

type check out test

5f93ff6

Move useToolbar out to a new react-aria package

4b5efd4

Fix reactive toolbar role

c44a930

devongovett previously approved these changes Oct 10, 2023

View reviewed changes

fix type and remove comment

fab3d46

snowystinger dismissed devongovett’s stale review via fab3d46 October 10, 2023 03:26

Merge branch 'main' into toolbar-component

b6be9de

# Conflicts: # packages/react-aria-components/package.json

reidbarber reviewed Oct 10, 2023

View reviewed changes

reidbarber approved these changes Oct 10, 2023

View reviewed changes

devongovett approved these changes Oct 10, 2023

View reviewed changes

snowystinger merged commit 7427349 into main Oct 10, 2023
26 checks passed

snowystinger deleted the toolbar-component branch October 10, 2023 21:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Toolbar component #5080

Toolbar component #5080

snowystinger commented Sep 15, 2023 •

edited

Loading

rspbot commented Sep 15, 2023

rspbot commented Sep 15, 2023

rspbot commented Sep 18, 2023

rspbot commented Sep 18, 2023

rspbot commented Sep 20, 2023

rspbot commented Sep 20, 2023

devongovett Sep 20, 2023

snowystinger Sep 21, 2023

devongovett Sep 20, 2023

LFDanLu Sep 20, 2023

snowystinger Sep 21, 2023

devongovett Sep 21, 2023

devongovett Sep 20, 2023

snowystinger Sep 21, 2023 •

edited

Loading

devongovett Sep 20, 2023

LFDanLu left a comment •

edited

Loading

rspbot commented Sep 21, 2023

rspbot commented Sep 25, 2023

rspbot commented Sep 26, 2023

snowystinger Sep 26, 2023

rspbot commented Sep 26, 2023

rspbot commented Sep 27, 2023

rspbot commented Sep 27, 2023

rspbot commented Sep 27, 2023

rspbot commented Sep 27, 2023

devongovett left a comment

devongovett Oct 10, 2023

devongovett Oct 10, 2023

rspbot commented Oct 10, 2023

rspbot commented Oct 10, 2023

reidbarber left a comment •

edited

Loading

snowystinger commented Oct 10, 2023

reidbarber left a comment


		export const ToolbarContext = createContext<ContextValue<ToolbarProps, HTMLDivElement>>({});

		function Toolbar(props: ToolbarProps, ref: ForwardedRef<HTMLDivElement>) {

Toolbar component #5080

Toolbar component #5080

Conversation

snowystinger commented Sep 15, 2023 • edited Loading

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

rspbot commented Sep 15, 2023

rspbot commented Sep 15, 2023

rspbot commented Sep 18, 2023

rspbot commented Sep 18, 2023

rspbot commented Sep 20, 2023

rspbot commented Sep 20, 2023

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

snowystinger Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LFDanLu left a comment • edited Loading

Choose a reason for hiding this comment

rspbot commented Sep 21, 2023

rspbot commented Sep 25, 2023

rspbot commented Sep 26, 2023

Choose a reason for hiding this comment

rspbot commented Sep 26, 2023

rspbot commented Sep 27, 2023

rspbot commented Sep 27, 2023

rspbot commented Sep 27, 2023

rspbot commented Sep 27, 2023

devongovett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rspbot commented Oct 10, 2023

rspbot commented Oct 10, 2023

@react-aria/toolbar

AriaToolbarProps

useToolbar

reidbarber left a comment • edited Loading

Choose a reason for hiding this comment

snowystinger commented Oct 10, 2023

reidbarber left a comment

Choose a reason for hiding this comment

snowystinger commented Sep 15, 2023 •

edited

Loading

snowystinger Sep 21, 2023 •

edited

Loading

LFDanLu left a comment •

edited

Loading

reidbarber left a comment •

edited

Loading