Can't focus fields in the Color Picker when shown in a dialog

[Bouh]

Describe the bug

The color picker of instructions and scene properties didn't work correctly.
The value Hexa, and RGB in numeric cannot be selected and edit.

I guess this bug occur only because of GenericExpressionField component exist.
Because the color picker in the layer list don't use this component and work fine.

This is a new bug, so it's a regression, bug is present in b93 and b92, but not in b89.

To Reproduce

Steps to reproduce the behavior:

1. Use b93 or b92
2. Go in scene properties or an instruction with colors.
3. Try to edit the hexa/rgb values
4. You can't

Other details

• Web-app and desktop.
• b93, b92

[4ian]

I've not checked but I'm pretty sure this only happens when you have a modal opened? This is because the Modal is "trapping" the focus, to avoid it to go out of the modal. Unfortunately, the Popper used to display the ColorPicker is considered as outside the modal, because it's using a "portal". A portal renders the popper in a DOM element "outside" of the modal (allowing free placement on the screen, and to have the ColorPicker not forced to display itself inside the modal borders).
There is a prop to disable the use of a portal (see disablePortal on https://material-ui.com/api/popper/) which fix the focus trapping (the modal now consider the popper as a child, so no focus issues) but then we have the issue of the ColorPicker not able to display anywhere on the screen.

See mui/material-ui#15694 - seems like a hard problem.

[ssangervasi]

The breadcrumb of mui 17497 suggests using disableEnforceFocus on the modal itself to so that portals it contains will work as intended. This would mean the user could focus outside of the modal, but that's better than not being able to edit color values.

[4ian]

This also implies being able to "tab" to other elements in the background, and to be able to do stuff like open other dialogs or change stuff "behind the back" of the current dialog - which sounds risky (in the sense that we'll get people to break GDevelop and deal with bug reports that should not even be possible).
But yeah I agree that not being able to enter values is also a deal breaker. Maybe we could actually allow the text field to display the color in RBG or Hex format - so that advanced users can just write it here.

Or yet another idea to test - see if we can add a "focus trap" inside the Color Picker when it's opened. After all, opening multiple dialogs works, so we should be able to trap the focus to only the picker.
Or yet another idea - show the color picker in a separate dialog - without Popper :) Though it can break the "flow" to have an entire modal opening just for selecting a color.

[4ian]

Turns out that there was another solution hidden in the thread. Don't use Popper, but Popover (which is slightly less "low level" and works almost the same for our use case!).
Actually, this color picker is a popover (in the sense that it must get the focus! Contrary to a autocompletion list that is a Popper as it's not getting the focus).

It works with a Popover:

Fixed in 1fd719f :)

[oliviertassinari]

@4ian I'm not sure that a Popover should be the only solution.
Considering https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/datepicker-dialog.html, I think that a Popper > TrapFocus approach should be a viable alternative to Popover (which is internally Modal > TrapFocus).

We have started to use the Popper > TrapFocus approach for the date range picker component and plan to expand it to the date picker with @dmtrKovalenko. However, has this project did, we have faced mui/material-ui#15694. Material-UI will probably prioritize work on the resolution of portal support. The solution is probably about listening to the synthetic focus event, React forward it through the portals. I have seen a prior-art working in https://github.com/theKashey/react-focus-lock/blob/7a6c0c916a7f5c0a040bece84a8dea4e5f999b6d/src/Trap.js#L143.

[4ian]

Thanks @oliviertassinari for jumping in the discussion :)

I think that a Popper > TrapFocus approach should be a viable alternative to Popover (which is internally Modal > TrapFocus).

I think that it would be a viable alternative indeed.

Material-UI will probably prioritize work on the resolution of portal support. The solution is probably about listening to the synthetic focus event, React forward it through the portals.

That sounds like the "right" fix to this problem.
For our case, switching to Popover was more a workaround... though having a Modal instead of Popper is not a huge issue because we were using a ClickAwayListener around the Popper, so we were mimicking the Popover behavior 🤔

[oliviertassinari]

@4ian Cool, regarding the ClickAwayListener, it only got portal support recently using the synthetic event approach.