[AlertDialog, Dialog, Popover] Configure initial focus

[michaldudak]

Added the initialFocus prop to control what element is focused after the Dialog, AlertDialog, or Popover is open.
By default it focuses the first element, unless the component was opened by touch interaction - in that case the popup itself is focused.

Part of #714

[mui-bot]

Netlify deploy preview

https://deploy-preview-732--base-ui.netlify.app/

Generated by 🚫 dangerJS against 3405016

[vladmoroz]

• An existing bug, but just noticed that in /experiments/dialog/, in the second dialog (CSS animation example) the initial focus isn't placed into the popup at all
• Is "" meant to be among pointer types?
• Since pointer type includes "keyboard", should we name this argument and type differently?

[michaldudak]

@vladmoroz I'll address the bug in a separate PR.

An empty string is a valid value of PointerEvent's pointerType field, so I included it here as well.

As for naming, yeah, I suppose we can think of something better. triggerType?

[vladmoroz]

@michaldudak what does the empty string mean?

What about "eventType"? "triggerType" sounds like it has to do with the trigger part

[michaldudak]

An empty string is an unknown pointer type (see https://developer.mozilla.org/en-US/docs/Web/API/PointerEvent/pointerType#value).

It's not really the type of the event (as this is PointerEvent or MouseEvent), but the user's input device that caused the event to fire (= triggered it).

[vladmoroz]

An empty string is an unknown pointer type (see https://developer.mozilla.org/en-US/docs/Web/API/PointerEvent/pointerType#value).

Eww 😬

If the browser supports pointer device types other than those listed above, the value should be vendor-prefixed to avoid conflicting names for different types of devices.

Do we return an empty string in this case or do we pass through the original type?

[michaldudak]

Do we return an empty string in this case or do we pass through the original type?

We pass through whatever there is originally.

[vladmoroz]

Looks good functionality-wise, I'll leave the code review to @atomiks though

[atomiks]

Doesn't seem like we need the implementation to be this detailed given we're only checking for touch presently. Instead of adding a click handler, we could simply add a pointerdown handler to read the pointerType, and ignore click.

[michaldudak]

In this case activating the trigger with keyboard would not be recognized at all, would it? I agree we don't need this internally, but we expose it to users so they can provide a function to the initialFocus prop with logic dependent on the method of activation.

[atomiks]

We can reduce it to a boolean isTouch, though it could be a breaking change in the future if we need to add more strings

[michaldudak]

IMO the API feels less weird with pointerType than isTouch. Also it doesn't have significant perf implications as the extra logic runs only on trigger click, so I'd leave it as it is.

[vladmoroz]

I agree about exposing all types, this makes for a much more flexible API.

My only nit is about PointerType name, where "keyboard" feels out of place. Perhaps both the type and the argument should be something like InteractionType? No big deal though

[michaldudak]

👍 yeah, "interaction" sounds better. How about InteractionMethod, though?

[vladmoroz]

InteractionType feels more like an analogy to event.pointerType, no? I would be cool with just interaction too.

I don't love "method" personally, it makes me think of something more systematic, like a derivative from "methodology":

[atomiks]

Would be great for this to be a ref instead to avoid an extra re-render, but I suppose there's no way to avoid reading it during render instead of in an effect/event handler?

[michaldudak]

When testing it, I never saw this state causing a rerender by itself. It's always batched with other state setters.