adobe / react-spectrum

A collection of libraries and tools that help you build adaptive, accessible, and robust user experiences.
https://react-spectrum.adobe.com
Apache License 2.0
12.9k stars 1.12k forks source link

Wrong return data type for `useTooltipTrigger` #4908

Open tuananhlai opened 1 year ago

tuananhlai commented 1 year ago

Provide a general summary of the issue here

I was having a bug with useTooltipTrigger, so I checked the code and found out that even though triggerProps have the data type of DOMAttributes & PressProps & HoverProps & FocusEvents, fields from the last 3 types are never returned in the code. I think this might be an error.

  let {hoverProps} = useHover({
    isDisabled,
    onHoverStart,
    onHoverEnd
  });

  let {pressProps} = usePress({onPressStart});

  let {focusableProps} = useFocusable({
    isDisabled,
    onFocus,
    onBlur
  }, ref);

  return {
    triggerProps: {
      'aria-describedby': state.isOpen ? tooltipId : undefined,
      ...mergeProps(focusableProps, hoverProps, pressProps) // these are all variables with type DOMAttributes
    },
    tooltipProps: {
      id: tooltipId
    }
  };

https://github.com/adobe/react-spectrum/blob/be501251a2012b2c1401e79940cbb39151c84d9d/packages/%40react-aria/tooltip/src/useTooltipTrigger.ts#L25C1-L25C1

🤔 Expected Behavior?

triggerProps type should only be DOMAttributes.

😯 Current Behavior

triggerProps type is DOMAttributes & PressProps & HoverProps & FocusEvents.

💁 Possible Solution

  1. Return extra methods for triggerProps to satisfy the data type.
  2. Remove PressProps & HoverProps & FocusEvents from triggerProps definition.

🔦 Context

No response

🖥️ Steps to Reproduce

See the code in this link.

https://github.com/adobe/react-spectrum/blob/be501251a2012b2c1401e79940cbb39151c84d9d/packages/%40react-aria/tooltip/src/useTooltipTrigger.ts#L25C1-L25C1

Version

@react-aria/tooltip v3.6.1

What browsers are you seeing the problem on?

Other

If other, please specify.

No response

What operating system are you using?

MacOS

🧢 Your Company/Team

No response

🕷 Tracking Issue

No response

reidbarber commented 1 year ago

Looks like the correct change since triggerProps is intended to go to an element and not into another hook.

tuananhlai commented 1 year ago

I think returning extra methods to satisfy PressProps & HoverProps & FocusEvents would be better. If not, react-spectrum's Button (or any button created with useButton) could not work as trigger for this tooltip.