[material-ui][Unstable_TrapFocus] Fix getTabbable function return type

[KalmarLorand]

Fixes #42231

FocusTrap component: Change the return type of the getTabbable function from ReadonlyArray<string> to ReadonlyArray<HTMLElement>.

[mui-bot]

Netlify deploy preview

https://deploy-preview-42237--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 004f1c1

[mnajdova]

Hey @KalmarLorand thanks for taking a stab at this issue. When changing the types you need to run:

pnpm prototypes
pnpm docs:api

to ensure that the propTypes and the docs API are up to date. This will fix the CI failing checks.

[KalmarLorand]

Hey @KalmarLorand thanks for taking a stab at this issue. When changing the types you need to run:

pnpm prototypes
pnpm docs:api

to ensure that the propTypes and the docs API are up to date. This will fix the CI failing checks.

Hey, thanks for the heads-up. I've ran the commands but for some reason nothing no files were changed
No declaration for FocusTrapSlots Built API docs for /base-ui/react-focus-trap/components-api/#focus-trap

[oliviertassinari]

As far as I understand things, this PR should be:

• moved to https://github.com/mui/base-ui
• FocusTrap released from @base_ui/react as unstable
• @base_ui/react/unstable_FocusTrap used in Material UI v6
• FocusTrap's source removed from @mui/base
• Documented in https://master--base-ui.netlify.app/base-ui/getting-started/ as either unstable or legacy

Or maybe alternatively, if clearer:

• moved to https://github.com/mui/base-ui
• FocusTrap released from @mui/base from https://github.com/mui/base-ui
• FocusTrap source removed from https://github.com/mui/material-ui
• Documented in https://master--base-ui.netlify.app/base-ui/getting-started/ as either unstable or legacy

cc @michaldudak to get a sense of what's the strategy with merging @mui/base into @base_ui/react.

[michaldudak]

The safest option, IMO, would be to move the code of legacy Base UI components used in Material UI to the Material UI sources.
We don't want to use any of the new Base UI components in Material UI just yet, as this could limit our options for API design and functionality.
cc @colmtuite

[oliviertassinari]

The safest option, IMO, would be to move the code of legacy Base UI components used in Material UI to the Material UI sources.

@michaldudak What risks do you have in mind?

In the two options described above, it would be handled with two separate npm packages, "Component" for the new ones, "Component (legacy)" for the old ones:

The cons I see with having this code of a Focus Trap split between Material UI and Base UI:

• The person responsible for Focus Trap has to work more on two different repositories to maintain its product scope: https://www.notion.so/mui-org/Focus-Trap-fbe1aed295f748c19576d024e9a48c62 (maybe to change the owner). The biggest challenge might be that the GitHub issues the person needs to check are spread over two different repositories. Or maybe it will be that it's not possible to update both components with the same PR.
• For all the @mui/base users, the migration path from this deprecated package might feel more obscure, or at least it doesn't feel like we play the strategy of carrying the community with us, by reducing the friction. Today, until Base UI gets parity with React Aria, this feels like our biggest differentiator, so it feels like we should play the card. The counterargument could be that maybe playing the Radix Primitives migration card is more effective, but if we look at the downloads: Radix Primitives (@radix-ui/react-slots - @storybook/components) vs. Base UI (@mui/base - @mui/material) it's not so clear which horse is the best to bet on (would migrate the most people).
• We don't educate much developers about the existence of the Base UI brand, people using @mui/base would feel more like a Material UI thing. The counterargument could be that maybe we would carry that feeling we want to avoid to Base UI, but if it's done well, I don't think it has to feel like this.

---

An important constraint I think we should enforce is to have a single owner for the Focus Trap logic, meaning to have it handled similarly to how the maintainer of https://github.com/focus-trap/focus-trap masters this product scope. It shouldn't be an option to have two different owners of two different implementations of the same product scope.

Implementing #42237 (comment) doesn't go against this constraint, but doesn't make it clear, so I highlighted it.

[michaldudak]

It may turn out we don't need a separate FocusTrap component in Base UI, so the current implementation used by Material UI can be moved to its sources. After we implement Material UI with Base UI, it could be removed altogether, as the logic will be baked in components such as the Dialog.

In the two options described above, it would be handled with two separate npm packages, "Component" for the new ones, "Component (legacy)" for the old ones

I'm not sure I'd put the legacy components docs in the same website. A subdomain with docs for @mui/base for existing users (similar to Material UI v4 docs) could work better. Or, (since AFAIK you agreed to put the new docs in a subdomain), we can leave the old docs in place (for @mui/base users) but clearly indicate they relate to the old version and link to the new docs (similarly to how React handles it: https://legacy.reactjs.org/docs/getting-started.html).

The current state of things isn't perfect, but we'll be in a much better place when we implement Material UI with @base_ui/react and have a single source of truth for the components logic.
Only then I'd introduce cross-product component owners.

Today, until Base UI gets parity with React Aria, this feels like our biggest differentiator, so it feels like we should play the card. The counterargument could be that maybe playing the Radix Primitives migration card is more effective

Sorry but I don't understand this bullet point.

We don't educate much developers about the existence of the Base UI brand

I don't want to speak on behalf of @colmtuite, but I don't think we want to just yet. When we have the foundational components ready, we'll advertise Base UI much more aggressively.

[colmtuite]

I'm not sure I'd put the legacy components docs in the same website.

We definitely do not want the legacy components in the same website. We want a fresh, clean site, positioning it as a new product. We don't want anything old weighing it down on day 1.

Ideally, the experience shouldn't change at all for legacy base ui users. Same package, same domain name, same docs, same repo etc. All that changes is they get a note in the docs that new Base UI is live, and they should check it out.

The current state of things isn't perfect, but we'll be in a much better place when we implement Material UI with @base_ui/react and have a single source of truth for the components logic.

Agreed. The pain of duplication is only temporary. When all other libs are built on Base UI, things will be much simpler. But we should live with the pain of duplication for now, otherwise we will compromise Base UI.

I don't want to speak on behalf of @colmtuite, but I don't think we want to just yet. When we have the foundational components ready, we'll advertise Base UI much more aggressively.

We're not focused on or interested in education/marketing right now, because we have nothing to talk about. When we're close to launch, and have our own domain, then we'll begin marketing/educating.

An important constraint I think we should enforce is to have a single owner for the Focus Trap logic

It is critically important that the Base UI team owns all of Base UI. The way to enforce consistency, is to just have other libs be built on top of Base UI.

[ZeeshanTamboli]

I made the changes in Material UI's unstable Focus Trap after its dependency on @mui/base was removed in #42907. I didn't update the @mui/base Focus Trap since it's no longer maintained. @KalmarLorand Thanks for the pull request.