Tooltip to strict on checking children

[eirikbacker]

Description of the bug

Reported by team in Mattilsynet;

Vi har denne Tooltip varianten

        <Tooltip content="Last ned elektronisk signert PDF" portal={false} placement="bottom">
          <Group>
            <CloudDownFillIcon aria-hidden fontSize="1.5rem" fill={mattilsynetTheme.primary} />
            {label}
          </Group>
        </Tooltip>

Dette treffer det du la til på Tooltip

    if (
      !children ||
      children?.type === Fragment ||
      (children as unknown) === Fragment
    ) {
      console.error(
        '<Tooltip> children needs to be a single ReactElement and not: <Fragment/> | <></>',
      );
      return null;
    }

Dersom vi bytter <Group> med <div> går det fint, eller wrapper <Group> med <div>, men det som er rart er at <Group> selv returnerer en <div> 😅

Suggestion:
Less strict children checking in component, but rather warning added in a useEffect where we can inspect the actual amount of rendered DOM children.

[Barsnes]

I suggest we change the check to:

    if (
      !Children.only(children) ||
      children?.type === Fragment ||
      Children.count(children) > 1
    ) {
      console.error(
        '<Tooltip> children needs to be a single ReactElement and not: <Fragment/> | <></>',
      );
      return null;
    }

If this fails we could probably rely on the internal error that is thrown

[eirikbacker]

See your suggestion, but this will break if passing nested Fragments for instance passing:

const Group (props) => <Fragment>{props.children}</Fragment>;

render(<Tooltip><Group>Content</Group></Tooltip>);

I suggest we therefore do the check on render, as this is makes us 100% sure of the resulting DOM:

useEffect(() => {
  if (selfRef.current?.children.length !== 1) console.error('<Tooltip> children needs to be a single ReactElement and not <Fragment/> | <></>');
}, [children]);

[Barsnes]

See your suggestion, but this will break if passing nested Fragments for instance passing:

const Group (props) => <Fragment>{props.children}</Fragment>;

render(<Tooltip><Group>Content</Group></Tooltip>);

I suggest we therefore do the check on render, as this is makes us 100% sure of the resulting DOM:

useEffect(() => {
  if (selfRef.current?.children.length !== 1) console.error('<Tooltip> children needs to be a single ReactElement and not <Fragment/> | <></>');
}, [children]);

What do you mean with nested fragments?
This will not render a tooltip, wether we have a check or not:

    <Tooltip content='content'>
      <>
        <p>hei</p>
      </>
    </Tooltip>

[Barsnes]

I tried doing this with the check we have today, and the tooltip is rendering fine.
Could you send a snippet of your component?

const Group = forwardRef<HTMLDivElement, { children: React.ReactNode }>(
  ({ children }, ref) => {
    return <div ref={ref}>{children}</div>;
  },
);

...

    <Tooltip content='content'>
      <Group>
        <p>hei</p>
      </Group>
    </Tooltip>

EDIT:
This type of group works fine as well

const Group = forwardRef<HTMLDivElement, { children: React.ReactNode }>(
  ({ children }, ref) => {
    return (
      <>
        <div ref={ref}>{children}</div>
      </>
    );
  },
);

[eirikbacker]

Both of these will work fine ☝️ But thats only because Group renders a div. If Group renders only a Fragment, it will not work. Therefore I sugges checking the actually rendered DOM :)

[Barsnes]

Both of these will work fine ☝️ But thats only because Group renders a div. If Group renders only a Fragment, it will not work. Therefore I sugges checking the actually rendered DOM :)

If the group only renders a fragment the tooltip can't be rendered, so I am still not following the issue here. Could you send a snippet of your group?

[Barsnes]

It's also not documented in storybook, since nothing is documented (😓 ), but the component you pass as a child needs to be able to recieve a ref

[eirikbacker]

Precisely; we want to log an error if the consumer is using Tooltip wrong.
If we want to do that, we need to check the actually rendered DOM, and not the vdom React is working with, as the vdom is not 1:1 with rendered elements. In this test, both Group2 and Group3 should log an error, but does not, unless we check the rendered DOM as suggested:

export const Preview: Story = {
  args: {
    content: 'Tooltip text',
    children: defaultChildren,
    placement: 'top',
  },
  decorators,
  render: ({ children, ...args }) => {
    const Group1 = () => (
      <>
        <span>Works</span>
      </>
    );
    const Group2 = () => (
      <>
        <span>Does not work</span>
      </>
    );
    const Group3 = () => (
      <>
        <span>Does not work</span>
        <span>Does not work</span>
      </>
    );

    return (
      <>
        <Tooltip {...args}>
          <Group2 />
        </Tooltip>
      </>
    );
  },
};

[Barsnes]

None of these would work, since none of them can recieve a ref, which the tooltip needs.
Sure we can check the DOM, but how would that solve the problem?
When you use Group1, you get an error message from react saying it needs a ref:

[eirikbacker]

Right, so our Tooltip does not support any element that can not receive a ref, because the wrapping element itself is a clone and not a span or div - sorry did not inspect that part of the code 🤦
I'm unsure if I agree with the solution we have made then as it requires the consumer to provide an element always, where we could do this for them? It also does not match the asChild pattern we have been doing all along?
Suggestion for Tooltip.tex:

const Component = asChild ? Slot : 'span';
…
return (
      <Popover.Context>
       <Popover.Trigger asChild> // TODO: Add logic for mouseover
          <Component>{children}</Component>
       </Popover.Trigger>
        {internalOpen && (
          <Popover className: cl('ds-tooltip', className)>
            {content}
          </Popover>
        )}
      </>
    );

making it consistent with other components, and allowing:

<Tooltip asChild>
  <div>Content</div>
</Tooltip>

...if for some reason needing to wrap a tooltip around non-phrasing content? :)

[Barsnes]

I am pretty sure this will not work either.
The trigger in your example still needs a ref, and if the component can't recieve one, it will not work.
When using asChild, the element you are asChilding also needs to be able to recieve a ref.

Take a look at our article about asChild and composition: https://www.designsystemet.no/grunnleggende/for-utviklere/komposisjon#:~:text=Ynskjer%20du%20%C3%A5%20bruke%20ein%20eigen%20komponent%20m%C3%A5%20du%20passe%20p%C3%A5%20%C3%A5%20spre%20alle%20props%2C%20og%20ha%20st%C3%B8tte%20for%20ref

[eirikbacker]

We do not need a ref on the trigger itself then- since we know the trigger will always render a element with this approach, the ref can be placed on Popover instead, and we can later pick out myPopoverRef.current.previousElementSibling, knowing this always be the tooltip trigger :)

[Barsnes]

This component was made before we started using Slot, so I suggest we implement this in favor of cloneElement and our internal check as a first step.
We can rely on radix' checks on this for now.

Ideally we would do things react knows about, and to my knowledge, using myPopoverRef.current.previousElementSibling, and adding things will make react unaware of this.

[eirikbacker]

Partly agree; should we provide const Component = asChild ? Slot : 'span'; as I suggested, so the consumer does not need to always provide a child element themselves?
It seems like work to allways require <Tooltip content="My tooltip"><span>my text</span></Tooltip>?
Also, using Slot directly will not make the examples of Group1, Group2 and Group3 work, but const Component = asChild ? Slot : 'span'; will fix that?

Though, doing myPopoverRef.current.previousElementSibling is not against React standards, as this is to get a FloatingUI reference to work, which is out of Reacts control anyway. React cares about the vdom, but not the actual DOM, and here we know the vdom will produce actual DOM, so all fine <3