Skip to content

RAC Checkbox and Radio unexpectedly call onBlur #8045

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
benwilliams140 opened this issue Apr 7, 2025 · 11 comments · May be fixed by #8108
Open

RAC Checkbox and Radio unexpectedly call onBlur #8045

benwilliams140 opened this issue Apr 7, 2025 · 11 comments · May be fixed by #8108
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@benwilliams140
Copy link

Provide a general summary of the issue here

Sequential presses to a Radio or Checkbox component will call onBlur upon the second press.

🤔 Expected Behavior?

onBlur shouldn't be called on sequential presses of the "input" element (red border)

Image

😯 Current Behavior

If a checkbox or radio is already focused, pressing on the element a second time will call onBlur (since it is technically the label getting pressed, not the actual input).

💁 Possible Solution

Other input elements (eg. TextField) leave the rendering of the Label and Input elements up to the consumer.

🔦 Context

This is technically expected based on default HTML behaviour and the structure of the components, but unexpected for a user (since it looks like they are pressing on the input).

As a side effect, my validation for these components is running unexpectedly when hooked into a form.

🖥️ Steps to Reproduce

https://codesandbox.io/p/sandbox/exciting-nightingale-wjx2jz

Version

[email protected]

What browsers are you seeing the problem on?

Microsoft Edge, Safari, Chrome, Firefox

If other, please specify.

No response

What operating system are you using?

MacOS

🧢 Your Company/Team

No response

🕷 Tracking Issue

No response

@snowystinger
Copy link
Member

We could probably change it to call onBlur/onFocus for focusin/focusout on the label instead. I think that'd be the behaviour you're expecting. Right now it's just using useFocusable for them

let {focusableProps} = useFocusable(mergeProps(props, {
which is being sent to the input only.
This would be an override for RAC, though, since the hook doesn't know that the label will wrap the input.

@snowystinger snowystinger added bug Something isn't working good first issue Good for newcomers labels Apr 8, 2025
@uniqueeest
Copy link

@snowystinger hi. i want to resolve this issue.

@uniqueeest
Copy link

uniqueeest commented Apr 15, 2025

@snowystinger Is it correct that you want me to move the onFocus, onBlur event to the label? The same issue occurs even if you proceed in that way. I'd appreciate a little help with this situation.

    <label
      {...mergeProps(DOMProps, labelProps, hoverProps, renderProps)}
      ref={ref}
      onFocus={props.onFocus}
      onBlur={props.onBlur}
      data-selected={isSelected || undefined}
      data-pressed={isPressed || undefined}
      data-hovered={isHovered || undefined}
      data-focused={isFocused || undefined}
      data-focus-visible={isFocusVisible || undefined}
      data-disabled={isDisabled || undefined}
      data-readonly={state.isReadOnly || undefined}
      data-invalid={state.isInvalid || undefined}
      data-required={state.isRequired || undefined}>
      <VisuallyHidden elementType="span">
        <input {...mergeProps(_inputProps, focusProps)} ref={inputRef} /> // except onBlur, onFocus
      </VisuallyHidden>
      {renderProps.children}
    </label>

@nwidynski
Copy link
Contributor

nwidynski commented Apr 15, 2025

@uniqueeest Something like this should do the trick:

let {onBlur, onFocus, ...inputDOMProps} = inputProps

// ...
<label
  {...mergeProps(DOMProps, labelProps, hoverProps, renderProps)}
  ref={ref}
  onFocus={onFocus}
  onBlur={onBlur}
  data-selected={isSelected || undefined}
  data-pressed={isPressed || undefined}
  data-hovered={isHovered || undefined}
  data-focused={isFocused || undefined}
  data-focus-visible={isFocusVisible || undefined}
  data-disabled={isDisabled || undefined}
  data-readonly={state.isReadOnly || undefined}
  data-invalid={state.isInvalid || undefined}
  data-required={state.isRequired || undefined}>
  <VisuallyHidden elementType="span">
    <input {...mergeProps(inputDOMProps, focusProps)} ref={inputRef} />
  </VisuallyHidden>
  {renderProps.children}
</label>

@uniqueeest
Copy link

@nwidynski I don't think that's a good idea, because we have two different types of focus, blur, so we have a type conflict.

@nwidynski
Copy link
Contributor

@uniqueeest You can safely ignore the type errors or recast the FocusableElement. A type mismatch was expected as this is an override, as @snowystinger mentioned.

Just make sure to also adjust the type signature of the component.

@uniqueeest
Copy link

@nwidynski

  const handleLabelFocus = (e: React.FocusEvent<HTMLLabelElement>) => {
    inputProps.onFocus?.(e as unknown as React.FocusEvent<HTMLInputElement>);
  };
  
  const handleLabelBlur = (e: React.FocusEvent<HTMLLabelElement>) => {
    inputProps.onBlur?.(e as unknown as React.FocusEvent<HTMLInputElement>);
  };

  let DOMProps = filterDOMProps(props);
  delete DOMProps.id;

  return (
    <label
      {...mergeProps(DOMProps, labelProps, hoverProps, renderProps)}
      ref={ref}
      onFocus={handleLabelFocus}
      onBlur={handleLabelBlur}
      data-selected={isSelected || undefined}
      data-pressed={isPressed || undefined}
      data-hovered={isHovered || undefined}
      data-focused={isFocused || undefined}
      data-focus-visible={isFocusVisible || undefined}
      data-disabled={isDisabled || undefined}
      data-readonly={state.isReadOnly || undefined}
      data-invalid={state.isInvalid || undefined}
      data-required={state.isRequired || undefined}>
      <VisuallyHidden elementType="span">
        <input {...mergeProps(inputProps, focusProps)} onFocus={undefined} onBlur={undefined} ref={inputRef} />
      </VisuallyHidden>
      {renderProps.children}
    </label>

If you do this, there won't be an onblur event😂

@benwilliams140
Copy link
Author

What about something like this? facebook/react#6410 (comment)

Note the "focusenter" and "focusleave" logs (these are the checks you'd want)

@nwidynski
Copy link
Contributor

@uniqueeest I'm sorry, I read this issue wrong. What you actually want to do is:

let {labelProps, inputProps, isSelected, isDisabled, isPressed} = useRadio({
  ...removeDataAttributes<RadioProps>(props),
  onFocus: undefined,
  onBlur: undefined,
  // ReactNode type doesn't allow function children.
  children: typeof props.children === 'function' ? true : props.children
}, state, inputRef);
let {focusableProps} = useFocusable(props, ref);

//...
<label
  {...mergeProps(DOMProps, labelProps, hoverProps, focusableProps, renderProps)}
  ref={ref}

This will change the target of focus events to be the label instead of the hidden input. You can further differentiate between the event types by applying the example @benwilliams140 mentioned.

@uniqueeest
Copy link

@nwidynski That way, the onBlur event is called right away from the approach...
As soon as I click on the label, the focus goes straight into the input and the label seems to be onblur.

@benwilliams140
Copy link
Author

@uniqueeest yes that's default browser behaviour (to focus the input after clicking on the label). You would need to differentiate between the event types to ignore blur events where focus is still within the label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
4 participants