Skip to content

rfc: added useClearButton proposal#4651

Closed
jguddas wants to merge 2 commits into
adobe:mainfrom
jguddas:rfc/use-clear-button
Closed

rfc: added useClearButton proposal#4651
jguddas wants to merge 2 commits into
adobe:mainfrom
jguddas:rfc/use-clear-button

Conversation

@jguddas
Copy link
Copy Markdown
Contributor

@jguddas jguddas commented Jun 10, 2023

This is a proposal for a new hook called useClearButton.

The points also apply when thinking about creating something like a useCloseButton hook.

@snowystinger
Copy link
Copy Markdown
Member

snowystinger commented Jun 12, 2023

Hey! thank you for the interest and appreciate the discussion.

I don't think we need an entirely new hook for clear buttons. The reason that useToggleButton is its own hook, is because it has state that it's dealing with and has unique aria attributes.

Clear button is specific to textfields that can also be cleared with the "Escape" key. It's just a button that is excluded from the tab order.

We could consider adding it to the useTextfield hook in order to attach the clearing behavior onPress, much like has been done in useSearchField https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/searchfield/src/useSearchField.ts#L109 However, it isn't really a pattern specific to textfield's, so I feel like it should be added as needed to those higher level hooks or just added when needed in a React Aria Component implementation, this is also the level where the "Escape" key is currently handled, which is mandatory if you're using a clear button. The reverse isn't true though. For example, a combobox can be cleared with "Escape", but it doesn't have a clear button at the moment. Maybe this is something we should allow for, not sure.

Could you provide examples that you're trying to address? Or can you use a searchfield instead?

@jguddas
Copy link
Copy Markdown
Contributor Author

jguddas commented Jun 13, 2023

You are right, this is already quite a solution rather than a problem statement.

The problem is that all these places need some form of reset functionality, which currently people just somehow hack together, I would rather see some provided, accessible approach that is easy to use.

  • useColorArea
  • useColorField
  • useColorSlider
  • useColorWheel
  • useCalendar
  • useDateField
  • useDatePicker
  • useDateRangePicker
  • useRangeCalendar
  • useTimeField
  • useCheckbox
  • useCheckboxGroup
  • useNumberField
  • useRadioGroup
  • useSearchField (already has clearButtonProps)
  • useSlider
  • useSwitch
  • useTextField
  • useDialog (closeButtonProps)
  • useModalOverlay (closeButtonProps)
  • usePopover
  • useComboBox
  • useSelect

@jguddas
Copy link
Copy Markdown
Contributor Author

jguddas commented Jun 13, 2023

Also, something like this would be nice.

  import {NumberField, Label, Group, Input, Button} from 'react-aria-components';
  
  <NumberField defaultValue={1024} minValue={0}>
    <Label>Width</Label>
    <Group>
      <Button slot="decrement">-</Button>
      <Input />
+     <Button slot="clear">x</Button>
      <Button slot="increment">+</Button>
    </Group>
  </NumberField>

@snowystinger
Copy link
Copy Markdown
Member

I see, could you check out #4640 and see if resetting form fields this way addresses your concerns.

I don't think we want to add various button props to all of these inputs. For instance, which one do we add to a NumberField? clear? or reset?
Sliders can only be reset, not cleared.

Because of this, I would advocate that people combine our hooks and the components they build with our hooks. It would be helpful if there was a showcase of how our library is misused when creating an accessible form field with a reset and/or clear button.

Dialogs and Popovers aren't form elements, so I feel like they are a different class of issue altogether. Let's keep this focused on forms and form elements for now.

@jguddas
Copy link
Copy Markdown
Contributor Author

jguddas commented Jun 17, 2023

I don't think we want to add various button props to all of these inputs. For instance, which one do we add to a NumberField? clear? or reset? Sliders can only be reset, not cleared.

Both are okay IMO, reset isn't that common, clear and close are the most common patterns I see, let's focus on clear for now.

showcase of how our library is misused when creating an accessible form field with a reset and/or clear button.

I wouldn't call it misused, it's just always custom stuff.

https://storiesv2.nextui.org/?path=/story/components-input--clearable

https://github.com/nextui-org/nextui/blob/4e29da619640365a3f921a863777d712f0a2aa8a/packages/components/input/src/use-input.ts#L228-L237

And there is always something that is not 100%.

  • Localized aria-label 🟥
  • Excluded from tab order (not really all that bad accessibility wise IMO) 🟥
  • Clicking the clear button should move focus to input ✔️
  • Reachable by screen reader ✔️
  • Keyboard support (Escape key 🟥 and space/enter ✔️ )

@snowystinger
Copy link
Copy Markdown
Member

Thanks for the reply and the example. You are right that being excluded from the tab order is not a big concern for this example. And because it's reachable by keyboard, it's not a big issue that the Escape key isn't hooked up. It is just a visual choice that it looks like it's in the same place as what I would call a "Clear button". I would say the only ding here is the missing label, which we cover here https://react-spectrum.adobe.com/react-aria/useButton.html#anatomy I would log a bug against nextui for this.

I think we could consider this, as the button should probably be labeled as "clear ${label.id}" -> "clear email". This would probably be good for our useSearchField at least, as "clear search" may not always be the best, for instance, if there are multiple search fields near each other.

The last thing I'm a bit hung up on is this. How do we know if we should turn on the "Escape" clears? I do not think we should always have that turned on in every textfield. I think that we should allow people to build a pattern that looks like either our SearchField OR nextui's email field.

@jguddas
Copy link
Copy Markdown
Contributor Author

jguddas commented Jun 19, 2023

I think we could consider this, as the button should probably be labeled as "clear ${label.id}" -> "clear email".

That's a great point @snowystinger, I would love if we could have something like that.

const { labelProps } = useLabel();
const { clearButtonProps } = useClearButton({
  labeledBy: labelProps.id,
  disableEscape: true, // esc clears by default
  disableTab: true // is reachable by default
});

I see 2 problems

  1. There needs to be an element in the DOM with the ID clear for <button aria-labledby="clear label">x</button> to work.
  2. Other languages have a different order, for example Suche löschen in German.

@snowystinger
Copy link
Copy Markdown
Member

snowystinger commented Jun 19, 2023

  1. isn't a problem, an element can be labeled by itself, <button id="foo" aria-label="clear" aria-labelledby="foo label-id">x</button>
  2. also not a problem when combining labelledby is my understanding. this is common in group components, read the component label first, then the group they belong to. we do this in NumberField for instance with the increment/decrement buttons which would face a similar concern if it was one singular localized string

I don't quite see how the useClearButton hook helps us determine if a "Clear button" has been used in the dom though, and I'd prefer not to add props to all of our hooks involving textfields for this.

Including a codesandbox of the use case, we're talking about https://codesandbox.io/s/musing-dhawan-2k4mvm?file=/src/App.js

Will have some other team members have a look as well.

@LFDanLu
Copy link
Copy Markdown
Member

LFDanLu commented Jun 21, 2023

If I remember correctly, useSearchField has clearButtonProps because we try to mirror native behavior with our hooks as much as possible. For hooks like useComboBox, useDatePicker, useNumberField, etc their native counter parts don't have clear buttons nor keyboard shortcuts for clearing/reseting the value applied so I'm personally hesitant to introduce that behavior into the hooks.

I do agree though that it would helpful if there was a way to spin up an accessible clear/reset button quickly, but we would need to decide if we want our hooks to handle that case directly (and thus become more opinionated), if it should live in a separate hook and place the onus on the user for using it correctly (aka useClearButton as you've mentioned), or if users should just leverage useButton to implement their own clear buttons (no guarantee that people would implement it correctly though which feels like the original issue you are trying to address with this).

@snowystinger
Copy link
Copy Markdown
Member

Hey again, I wanted to let you know we discussed this more as a team today.
We've decided we don't want to move forward with a useClearButton but we gained a lot out of the discussion, so thank you for bringing it up.

We don't want to move forward with this for a couple of reasons:

  1. We have a useButton, as well as the RAC Button component, which can be used as specified above
  2. We don't want to build it into every field, both because we don't want to expand the props on every component as well as it being a bit unclear on certain components what its function would be. (ie ColorArea/ColorWheel/CheckboxGroup)
  3. We feel that the work being done on native form reset will cover many of these use cases
  4. Clear buttons aren't in the aria patterns for most of the components
  5. You can maintain a useClearButton hook for your use cases

Again, thank you for the discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants