I would propose to change none to something more disturbing like noLabelOrDescription

plus ensuring it is a required prop without default would make the API safer to use

I changed it to inaccessible; how does that sound? (cc @smhigley)

This will not work with composition properly:

<Tooltip>
  <MenuTrigger>
    <button aria-label='foo' />
  </MenuTrigger>
</Tooltip>

In this case child?.props will be actually props of MenuTrigger, not button's.

Menu had the same issue, check #21225. Is there another approach?

This should work with Menu or any composition, since the props applied to the trigger element will always have priority over trigger overrides.

In the case where Tooltip is wrapped around MenuTrigger:

1. Tooltip will fail to find aria-label or aria-describedby
2. Pass and aria-* attribute through prop cloning
3. Menu will let the user's aria-label win

Am I missing anything ?

The check is actually useless, and should be removed, as long as our triggers follow the constraint that user props are always respected (this is currently the case) we can spread whatever trigger props and let the useTriggerElement hook do its job

A bigger example why it's not useful even without composition:

const TooltipButton = React.forwardRef((props, ref) => {
  return <Button {...props} ref={ref} aria-label="foo" />;
});

function App() {
  return (
    <Tooltip content={<div />}>
      {/* 👇 this does not have "aria-label" and "aria-labelledby",
       *     so Tooltip will apply "aria-labelledby" as "content" is not a string
       */}
      <TooltipButton />
    </Tooltip>
  );
}

After rendering that snippet we will get a button that has both aria-label and aria-labelledby. As we discussed with @ling1726 it's more reasonable to pass props always and implement a runtime check to ensure that both attributes are not defined on a trigger HTML element. As we have an access to it via refs it should be doable.

WDYT?

I'm not sure that setting the aria props via a ref is a good idea... it seems a bit hacky for an edge case scenario.

But thinking about it further, it might be wrong to even try to check for existing aria-label, etc... Now that the relationship prop is required, the user has to choose to set the Tooltip to be the label. If your example had relationship="label" on the Tooltip, then it's reasonable to assume that it would override the existing aria-label.

And in the case where they want to preserve the TooltipButton's internal aria-label, they can set relationship="inaccessible". In fact, that is probably a good thing, since it shines a light on the fact that the tooltip's content is inaccessible in that case.

I'm not sure that setting the aria props via a ref is a good idea... it seems a bit hacky for an edge case scenario.

Sorry, it seems that I was not enough clear, I don't suggest to set aria props via ref. My proposal is to keep existing cloning and and validate on DOM that conflicting props are not applied:

if (process.env.NODE_ENV !== 'production') {
  if (triggerRef.current) {
    if (triggerRef.current.hasAttribute('aria-label') && triggerRef.current.hasAttribute('aria-labelledby') {
      console.warn('Please do not combine conflicting attributes')
    }
  }
}

Ah got it, sorry I misunderstood--so it's just adding a warning?

I'm still inclined not to add a warning here:

• There are other hypothetical ways you could unexpectedly end up with both aria-label and aria-labelledby on some of our components. It seems out of scope to check for that in Tooltip.
  • E.g. from your example, a standalone <TooltipButton aria-labelledby="..." /> would have that problem, and it wouldn't benefit from the check in Tooltip.
• An accessibility checker would be a better utility to have a warning like that, and it'd check everything, not just Tooltip.
• It's still a very unlikely edge case.
• People don't pay attention to console warnings.

That said, if you feel strongly about it, I can add the warning 🙂

As for the problem with that you can't check the child props for aria-label, etc. when you have nested triggers, I have an idea for how to fix that as well, by modifying the onlyChild helper function. See my comment here for more details: #21245 (comment)

That change would allow this code to work the same regardless of whether or not there is a MenuTrigger between the Tooltip and button.

That said, if you feel strongly about it, I can add the warning 🙂

Let's add it if someone will report us an issue then :)

@layershifter to clarify, do you mean leave it out, and only add it if/when someone reports an issue? Or do you mean add it now, and see if someone reports an issue because of it?

If it's the former, then there are no more changes needed in this PR right? Could you approve if everything looks ok?

Thanks!