Switch AccessibleButton and derivatives to using forwardRef

[t3chguy]

For https://github.com/element-hq/customer-retainer/issues/157
Requires #12053

This is to enable AccessibleTooltipButton using Compound's Tooltip which requires this.

---

This change is marked as an internal change (Task), so will not be included in the changelog.

[richvdh]

there's a lot changing here - much of which, at first glance, doesn't appear to match the PR description (ie, it is not about using forwardRef).

Could you either break up the PR, or help me as a reviewer understand what I'm looking at by making a clear description of it?

[t3chguy]

Could you either break up the PR, or help me as a reviewer understand what I'm looking at by making a clear description of it?

This is a broken up PR, this is as small as it can get, given how many places use & extend AccessibleButton

it is not about using forwardRef

Just updated branch which knocked out some bits unrelated to forwardRef from the merged base PR. Everything remaining relates to changing AccessibleButton and derivates from a custom ref prop to forwardRef for compatibility with Compound/Radix - this means updating any call sites rendering those components where a custom ref prop was passed and types to make tsc happier.

[richvdh]

This is a broken up PR, this is as small as it can get, given how many places use & extend AccessibleButton

Well, you seem to have found a way to make it smaller since I first looked at it, so thank you for that.

[t3chguy]

@richvdh yeah its an unfortunate side effect of squash merging combined with PR stacking =(

[richvdh]

Looks generally plausible, but I have some requests mostly around better documentation.

[t3chguy]

@richvdh I have split things out into #12072

[t3chguy]

Another split out #12075

[richvdh]

lgtm

[richvdh]

Suggested change

	* If a ref is passed, it will be forwarded to the rendered element as specified using the `element` prop.
	* If a ref is provided, it will be forwarded to the rendered element (as specified using the `element` prop).
	* If `element` is a DOM element, the ref will be updated to point to the rendered DOM node.

[t3chguy]

If element is a DOM element, the ref will be updated to point to the rendered DOM node.

element can only be a string though, not a DOM element. It can be a name of a DOM element but it also can't be anything else (other than undefined where the value is div) so not sure what this 2nd line is trying to say.

[richvdh]

My understanding is that element can either be an intrinsic DOM element (div, button, or whatever), or a React component (MyFancyButton). (See https://www.typescriptlang.org/docs/handbook/jsx.html#type-checking, etc)

• If it's a react component, we can't really say much about what happens to the ref other than that it is forwarded to that component for it to do with as it chooses. (It might set it to 42, for all we know.)
• However, if it's an intrinsic DOM element, then we can be much more helpful: the act of "forwarding" it means that ref.content will be updated to point to the rendered DOM node corresponding to that element. This is what the user of AccessibleButton needs to know, and is the reason that I wanted to add that second sentence.

Now: if you're saying that element can't in fact be a React component and can only ever be an intrinsic DOM element, then we can clarify this further (since "forwarding" is entirely an implementation detail of AccessibleButton). Maybe something like:

Suggested change

	* If a ref is passed, it will be forwarded to the rendered element as specified using the `element` prop.
	* If a ref is provided, it will be updated to point to the rendered DOM node (which will normally be a `div`,
	* unless overridden via the `element` property).

[t3chguy]

The T type generic can only be one of these keys:

(continued...)

So you cannot use a React component here

[richvdh]

yup I misread the typescript. (#include "I shouldn't have to reverse-engineer typescript types.rant")