TagGroup #1922

jluyau · 2021-05-17T22:42:30Z

Closes #1921 1921

✅ 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:

Adobe/Quarry

jluyau · 2021-05-17T22:45:52Z

Looking for initial review on implementation.

Things I'm not quite about:

cycleMode name in GridKeyboardDelegate + functionality
Not sure if more implementation should exist in useTag and useTagGroup
should keep Removable class?

Need to:

update tests + docs
add slotting so that it can handle icons

adobe-bot · 2021-05-20T00:29:29Z

Build successful! 🎉

jluyau · 2021-08-23T18:30:03Z

Latest TODO list:

[API] cycleMode prop (as mentioned in earlier comment)
[API] do we still need the Removable interface?
do more items need to move into useTagGroup and useTagList?
add slots for icon/avatar
check that if a tag is removed, focus is properly transferred to the previous/next tag
update tests
update docs

packages/@adobe/react-spectrum/package.json

tiffany-sy-lin · 2021-10-25T16:32:11Z

[API] cycleMode prop (as mentioned in earlier comment)
[API] do we still need the Removable interface?

@snowystinger would you happen to know what these 2 points need to address? It's a bit on the vague side

adobe-bot · 2021-10-27T18:56:54Z

Build successful! 🎉

adobe-bot · 2021-10-28T16:14:48Z

Build successful! 🎉

packages/@adobe/spectrum-css-temp/components/tags/index.css

adobe-bot · 2022-01-21T22:31:38Z

Build successful! 🎉

adobe-bot · 2022-01-21T22:46:22Z

Build successful! 🎉

adobe-bot · 2022-01-21T23:07:26Z

Build successful! 🎉

LFDanLu

Some addition things I noted but I'm fine with handling them in a followup PR

LFDanLu · 2022-01-27T18:58:48Z

packages/@react-aria/tag/src/TagKeyboardDelegate.ts

+        // Ignore disabled tags
+        if (this.disabledKeys.has(newKey)) {
+          return this.getKeyBelow(newKey);
+        }


Open question: should we be skipping the disabled tags?

ButtonGroup, ActionGroup, etc. do so I think so.

I'm not sure, since tags are both data as well as interactive elements. Can any tag be disabled? or only the ones with some sort of interactive capability such as close?
Can probably sort out in a later PR, but I don't know if the answer is as clearcut as I would like

Originally I thought disabled tags should get skipped bc ActionGroup items do too, but if we should be able to delete a disabled tag then they need to be focusable. However it becomes an interactive element which then needs 4.5:1 color contrast which it won't get. Also agree it doesn't need to be a blocker for this PR

Will address/continue conversation in followup PR

LFDanLu · 2022-01-27T19:14:49Z

packages/@react-aria/tag/src/useTag.ts

+  let {rowProps} = useGridRow({
+    node: item
+  }, state, tagRowRef);
+  // Don't want the row to be focusable or accessible via keyboard
+  // eslint-disable-next-line @typescript-eslint/no-unused-vars
+  let {tabIndex, ...otherRowProps} = rowProps;
+
+  let {gridCellProps} = useGridCell({
+    node: [...item.childNodes][0],
+    focusMode: 'cell'
+  }, state, tagRef);


Followup question for next PR: should we be focusing the row or the cell in the TagGroup? Will have to experiment because I had run into issues making focus stay on the row in CardView myself. v2 focused the rows rather that the cells

Will address/continue conversation in followup PR

LFDanLu · 2022-01-27T19:27:41Z

packages/@react-aria/tag/src/useTag.ts

+      onFocus() {
+        state.selectionManager.setFocusedKey(item.childNodes[0].key);
+      },


curious, what prompted the addition of this? Was it to override the onFocus from useGridCell? If so I don't think you need to since the raf won't trigger here since we have focusMode: cell.

Will address in followup PR

packages/@react-spectrum/tag/src/Tag.tsx

LFDanLu · 2022-01-27T19:45:18Z

packages/@react-spectrum/tag/src/Tag.tsx

-        {...styleProps}
-        {...mergeProps(tagProps, hoverProps)}
+        {...mergeProps(tagProps, hoverProps, focusProps)}
+        role="gridcell"


can get rid of this role since it comes from tagProps

Will address in followup PR

LFDanLu · 2022-01-27T19:48:46Z

packages/@react-spectrum/tag/src/TagGroup.tsx

+      // add column of clear buttons if removable
+      if (isRemovable) {
+        childNodes.push({
+          key: `remove-${item.key}`,
+          type: 'cell',
+          index: 1,
+          value: null,
+          level: 0,
+          rendered: null,
+          textValue: item.textValue, // TODO localize?
+          hasChildNodes: false,
+          childNodes: []
+        });
+      }


Think we can remove this since we aren't supporting keyboard focusing the clear button separately? We can keep if there is future plans to reimplement that behavior. Same with the columnCount above

Will address in followup PR

LFDanLu · 2022-01-27T19:54:06Z

packages/@react-spectrum/tag/src/TagGroup.tsx


+  // Don't want the grid to be focusable or accessible via keyboard
+  // eslint-disable-next-line @typescript-eslint/no-unused-vars
+  let {tabIndex, ...otherGridProps} = gridProps;


Do we need this? Feel like we should keep the tabIndex on the TagGroup and just add css to prevent the outline flicker from happening

I was running into focus issues with keyboard nav when the group had a tabIndex. for the most part i was trying emulate ActionGroup's tabIndex-ing

Will look into it more in followup PR

LFDanLu · 2022-01-27T19:55:54Z

packages/@react-spectrum/tag/src/index.ts

 */

 /// <reference types="css-module-types" />



Should probably export Item as well like how ActionGroup and other components that use Item do

export {Item} from '@react-stately/collections';

Will address in followup PR

LFDanLu · 2022-01-27T20:06:13Z

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

+export interface SpectrumTagProps<T> extends TagProps<T> {
+  state: GridState<any, any>
+}


Don't think we expose Tag right? Can probably move this to TagProps or even send it through a context provider instead of through props

Will address in followup PR

majornista · 2022-01-31T18:31:48Z

packages/@react-aria/tag/src/useTag.ts

  const formatMessage = useMessageFormatter(intlMessages);
  const removeString = formatMessage('remove');
-  const tagId = useId();
+  const labelId = useId();


labelId is only returned in the aria-labelledby for clearButtonProps and is not assigned as an id for the Tag's label.

Will address in followup PR

majornista · 2022-01-31T18:33:12Z

packages/@react-aria/tag/src/useTag.ts

    clearButtonProps: mergeProps(pressProps, {
      'aria-label': removeString,
-      'aria-labelledby': `${buttonId} ${tagId}`,
+      'aria-labelledby': `${buttonId} ${labelId}`,


labelId is an invalid id reference because it is not applied as an id for the Tab's label element.

Will address in followup PR

adobe-bot · 2022-02-02T23:20:06Z

Build successful! 🎉

snowystinger · 2022-02-03T00:29:47Z

packages/@react-spectrum/tag/src/Tag.tsx

+            icon: {UNSAFE_className: classNames(styles, 'spectrum-Tag-icon'), size: 'XS'},
+            text: {UNSAFE_className: classNames(styles, 'spectrum-Tag-content', {'tags-removable': isRemovable})}
+          }}>
+          {typeof children === 'string' ? <div ref={labelRef} {...labelProps}><Text>{children}</Text></div> : children}


isn't this fundamentally different from what a user would pass in? I think it should be

Suggested change

{typeof children === 'string' ? <div ref={labelRef} {...labelProps}><Text>{children}</Text></div> : children}

{typeof children === 'string' ? <Text>{children}</Text> : children}

where labelRef and labelProps are passed on the text slot? though what is the labelRef actually used for? i don't see it passed anywhere, looks like it's created and assigned, nothing else. So we might be able to just delete that.

The labelProps has the labelId which in this case is used for voiceover in the remove scenario. I don't mind removing labelRef and any other unused refs.

I think we need to remove that extra div as well though, it's not something that the user would provide, which means that providing a string vs providing React element children would be different right?

<Tag>apples</Tag>

would be labelled differently than

<Tag><Text>apples</Text></Tag>

Or am I reading into this the wrong way?

you're right. i didn't consider that when adding labelProps. i might have to restyle the children scenario

Will address in followup PR

ktabors

Generally looks fine. I think a number fo Daniel's concerns need to be addressed.

ktabors · 2022-02-03T21:29:20Z

packages/@react-aria/tag/src/TagKeyboardDelegate.ts

@@ -0,0 +1,91 @@
+/*
+ * Copyright 2020 Adobe. All rights reserved.


Shouldn't hold this up, but the copyright should be 2022.

ktabors · 2022-02-03T21:39:51Z

packages/@react-aria/tag/src/useTag.ts

      id: buttonId,
      title: removeString,
-      isDisabled,
-      role


Is this why when I turn on OSX voiceover and ctl-opt-arrow to the ClearButton it only says "dimmed" and not "dimmed and disabled button"?

ktabors · 2022-02-03T21:58:46Z

packages/@react-aria/tag/test/useTag.test.js

@@ -1,50 +0,0 @@
-/*


I see @snowystinger's suggestion to remove this, but his comment made it seem like a useTagGroup.test.js would be useful. This could be followup.

integration meaning only test at the component level, I don't think useTagGroup would be a super useful test either. Testing hooks can be a bit contrived, better to use a component to put them through real world examples I think

packages/@react-spectrum/tag/src/Tag.tsx

ktabors · 2022-02-03T22:02:34Z

packages/@react-spectrum/tag/src/TagGroup.tsx

+    collection: gridCollection,
+    focusMode: 'cell'
+  });
+  let keyboardDelegate = new TagKeyboardDelegate({


When testing I noticed that home, end, page up, and page down didn't work. Should they?

Will address/continue conversation in followup PR

ktabors · 2022-02-03T22:16:30Z

packages/@react-spectrum/tag/test/TagGroup.test.js

   Name           | Component     | TagComponent | props
-   ${'TagGroup'}  | ${TagGroup}   | ${Tag}       | ${{isReadOnly: true, isRemovable: true, onRemove: onRemoveSpy}}
-  `('$Name can be read only', ({Component, TagComponent, props}) => {
+   ${'TagGroup'}  | ${TagGroup}   | ${Item}       | ${{isDisabled: true, isRemovable: true, onRemove: onRemoveSpy}}


Why is this an it.each story if there is only one condition? The same for the rest of them.

A lot of these have the same component definition, should these exist in a beforeEach()?

we have a lot of tests like this, it's probably just copied, not worried about it

i copied the structure from other tests (because they looked cool haha). i can change them to single tests

Will address/continue conversation in followup PR

ktabors · 2022-02-03T22:27:00Z

packages/@react-spectrum/tag/test/TagGroup.test.js

+    expect(tagDisabled).toHaveAttribute('tabIndex', '-1');
+    let tagRowNotDisabled = tagGroup.children[0];
+    let tagNotDisabled = tagRowNotDisabled.children[0];
+    act(() => {tagNotDisabled.focus();});


Should we confirm it is focused by checking the document.activeElement?

Is it better practice to use document.activeElement? i came across .focus() and thought it seemed fine

Will address/continue conversation in followup PR

ktabors · 2022-02-03T22:33:52Z

packages/@react-spectrum/tag/test/TagGroup.test.js

+    ${'(left/right arrows, ltr + horizontal) TagGroup'} | ${{locale: 'de-DE'}}                          | ${[{action: () => {userEvent.tab();}, index: 0}, {action: pressArrowRight, index: 1}, {action: pressArrowLeft, index: 0}, {action: pressArrowLeft, index: 0}]}
+    ${'(left/right arrows, rtl + horizontal) TagGroup'} | ${{locale: 'ar-AE'}}                          | ${[{action: () => {userEvent.tab();}, index: 0}, {action: pressArrowRight, index: 0}, {action: pressArrowLeft, index: 1}, {action: pressArrowLeft, index: 2}]}
+    ${'(up/down arrows, ltr + horizontal) TagGroup'}    | ${{locale: 'de-DE'}}                          | ${[{action: () => {userEvent.tab();}, index: 0}, {action: pressArrowDown, index: 1}, {action: pressArrowUp, index: 0}, {action: pressArrowUp, index: 0}]}
+    ${'(up/down arrows, rtl + horizontal) TagGroup'}    | ${{locale: 'ar-AE'}}                          | ${[{action: () => {userEvent.tab();}, index: 0}, {action: pressArrowDown, index: 1}, {action: pressArrowUp, index: 0}, {action: pressArrowUp, index: 0}]}


This last test does two steps at position 0, maybe start at position 2 and move to position 0? This would make it more similar to the second test.

Will address in followup PR

ktabors · 2022-02-03T22:35:19Z

packages/@react-spectrum/tag/test/TagGroup.test.js

+    expect(tagGroup).toHaveAttribute('aria-labelledby', 'tag group');
+  });
+
+  it('TagGroup allows aria-label on Item', function () {


API question, does this mean we support a complex Item that is just an Icon and an aria-label? I confirmed that I was able to do this in storybook.

I tried to emulate actiongroup (since they have similar structures). one of the action stories has aria-label on the Item so i added it as a test for taggroup. although i don't have any stories with aria-label in Item...so i am happy to remove this if it's not suppose to be supported

Will address/continue conversation in followup PR

ktabors · 2022-02-03T22:49:33Z

packages/@react-spectrum/tag/test/TagGroup.test.js

+
+    let buttonBefore = getByLabelText('ButtonBefore');
+    let buttonAfter = getByLabelText('ButtonAfter');
+    let inputs = getAllByRole('gridcell');


Shouldn't this variable be called tags? I'm looking at the last test where it is and the textfields are called inputs. This was confusing because the meaning of inputs changed between tests.

good point. will change

Will address in followup PR

majornista

Is there a plan to manage focus when a Tag is removed from the TagGroup?
Do we need stories where onRemove does more than just trigger an event?
We need to account for aria-disabled state on the row or gridcell for disabled keys.
We need to account for an empty TagGroup, where the TagGroup element should either not render, or its should have role="group" instead of role="grid".

majornista · 2022-02-04T17:12:39Z

packages/@react-aria/tag/src/useTag.ts

  }
  const pressProps = {
-    onPress: e => onRemove(children, e)
+    onPress: e => onRemove?.(children, e)


If we use item.key, I think we should then be able to use the keyboard delegate to shift focus the next or previous Tag on remove when one exists.

majornista · 2022-02-04T17:49:59Z

packages/@react-aria/tag/src/useTag.ts

+    tagProps: mergeProps(domProps, gridCellProps, {
+      'aria-errormessage': props['aria-errormessage'],
+      'aria-label': props['aria-label'],
+      onKeyDown: !isDisabled && isRemovable ? onKeyDown : null,
+      tabIndex: (isFocused || state.selectionManager.focusedKey == null) && !isDisabled ? 0 : -1,
+      onFocus() {
+        state.selectionManager.setFocusedKey(item.childNodes[0].key);
+      },


isDisabled does not add aria-disabled="true" to the Tag element with role="gridcell", which causes aXe accessibility automation tool to throw the following error regarding the color contrast for the Tag text content in Storybook example: "#react-aria1908471221-9 > .tags_spectrum-Tag-content_f7ae1 Element has insufficient color contrast of 2.08 (foreground color: #5c5c5c, background color: #2c2c2c, font size: 9.0pt (12px), font weight: normal). Expected contrast ratio of 4.5:1." To reproduce, open https://reactspectrum.blob.core.windows.net/reactspectrum/b7ddd9f7dec2dbf18633a71801a4ca50506e4762/storybook-16/index.html?path=/story/taggroup--disabledkeys, and in the Storybook Accessibility addOn, press the Tests Completed button to rerun the tests.

Note that this error does not seem to happen when the entire TagGroup is disabled, because aria-disabled="true" gets added to the TagGroup element with role="grid".

will add aria-disabled

Will address in followup PR

majornista · 2022-02-04T18:10:28Z

packages/@react-spectrum/tag/src/TagGroup.tsx


+  // Don't want the grid to be focusable or accessible via keyboard
+  // eslint-disable-next-line @typescript-eslint/no-unused-vars
+  let {tabIndex, ...otherGridProps} = gridProps;


When there are no Tags within the TagGroup, the container with role="grid" will render with no descendants. This will cause aXe automated accessibility testing tool to throw an error, because elements with role="grid" are expected to have rows, or a rowgroup containing rows, as children.

In the case of an empty TagGroup, the TagGroup could either not render at all or use role="group", which is allowed to be empty, instead of role="grid".

Will address in followup PR

adobe-bot · 2022-02-10T17:11:29Z

Build successful! 🎉

adobe-bot · 2022-02-10T18:26:06Z

Build successful! 🎉

tiffany-sy-lin · 2022-02-10T22:22:24Z

Followup ticket with list of items to address #2837

dannify

I think this is good for an alpha. I agree that the role of grid might be overkill for a TagGroup. We don't have selection in this component so we might be able to get away with a role of "list" instead since the tags are not interactive and there is nothing very special about it?

adobe-bot · 2022-02-11T20:22:33Z

Build successful! 🎉

Will be handled in a followup

initial TagGroup commit

6e3130f

jluyau requested review from dannify, devongovett and snowystinger as code owners May 17, 2021 22:42

jluyau changed the title ~~initial TagGroup commit~~ [WIP] TagGroup May 17, 2021

jluyau added 3 commits May 19, 2021 17:06

lint

dbbe675

another lint

f450b3b

temp skip tests

9280b39

Tiffany Lin and others added 3 commits September 14, 2021 15:05

Merge branch 'main' of github.com:adobe/react-spectrum into taggroup

4e51f88

slotting tags

291e782

tag group styling

4597507

snowystinger reviewed Sep 15, 2021

View reviewed changes

packages/@adobe/react-spectrum/package.json Outdated Show resolved Hide resolved

tiffany-sy-lin and others added 6 commits September 15, 2021 12:46

Merge branch 'main' of github.com:adobe/react-spectrum into taggroup

0d61c4c

update taggroup icon story

2ffa9b6

undo bad pull from main

c220254

linting

377a136

Merge branch 'main' of github.com:adobe/react-spectrum into taggroup

5586040

add roles. update tests

bf97d2c

Tiffany Lin added 3 commits October 27, 2021 10:10

pass custom props to indiv tag

b4918ee

update grid version

d3a79ab

add missing param

b8883fc

Merge branch 'main' into taggroup

69d9ddf

snowystinger reviewed Oct 28, 2021

View reviewed changes

packages/@adobe/spectrum-css-temp/components/tags/index.css Outdated Show resolved Hide resolved

packages/@adobe/spectrum-css-temp/components/tags/index.css Outdated Show resolved Hide resolved

use padding-inline-start for styling

8dce23b

remove unused packages

0cfd8c7

add translations

8c6370a

LFDanLu previously approved these changes Jan 27, 2022

View reviewed changes

majornista requested changes Jan 31, 2022

View reviewed changes

remove extra aria label, unneeded intl files, and add labelId to props

b7ddd9f

tiffany-sy-lin dismissed LFDanLu’s stale review via b7ddd9f February 2, 2022 23:12

tiffany-sy-lin requested a review from majornista February 2, 2022 23:28

snowystinger reviewed Feb 3, 2022

View reviewed changes

ktabors reviewed Feb 4, 2022

View reviewed changes

majornista previously requested changes Feb 4, 2022

View reviewed changes

dannify changed the title ~~[WIP] TagGroup~~ TagGroup Feb 4, 2022

snowystinger previously approved these changes Feb 4, 2022

View reviewed changes

remove console.log

c090376

tiffany-sy-lin dismissed snowystinger’s stale review via c090376 February 10, 2022 17:04

Merge branch 'main' into taggroup

10b2058

tiffany-sy-lin mentioned this pull request Feb 10, 2022

TagGroup Followup Items #2837

Closed

17 tasks

snowystinger approved these changes Feb 10, 2022

View reviewed changes

dannify approved these changes Feb 11, 2022

View reviewed changes

Merge branch 'main' into taggroup

fcddfc0

dannify merged commit cc5deac into main Feb 11, 2022

dannify deleted the taggroup branch February 11, 2022 20:25

	{typeof children === 'string' ? <div ref={labelRef} {...labelProps}><Text>{children}</Text></div> : children}
	{typeof children === 'string' ? <Text>{children}</Text> : children}

		@@ -0,0 +1,91 @@
		/*
		* Copyright 2020 Adobe. All rights reserved.

TagGroup #1922

TagGroup #1922

Uh oh!

Conversation

jluyau commented May 17, 2021

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

Uh oh!

jluyau commented May 17, 2021

Uh oh!

adobe-bot commented May 20, 2021

Uh oh!

jluyau commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

tiffany-sy-lin commented Oct 25, 2021

Uh oh!

adobe-bot commented Oct 27, 2021

Uh oh!

adobe-bot commented Oct 28, 2021

Uh oh!

Uh oh!

Uh oh!

adobe-bot commented Jan 21, 2022

Uh oh!

adobe-bot commented Jan 21, 2022

Uh oh!

adobe-bot commented Jan 21, 2022

Uh oh!

LFDanLu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jluyau commented Aug 23, 2021 •

edited

Loading

snowystinger Feb 3, 2022 •

edited

Loading

tiffany-sy-lin Feb 3, 2022 •

edited

Loading