godot icon indicating copy to clipboard operation
godot copied to clipboard

Improve ColorPicker picker shape keyboard and joypad accessibility

Open FeniXb3 opened this issue 1 year ago • 8 comments

While experimenting with accessibility, I realized that ColorPicker does not allow you to move the picker cursor around the color rectangle/wheel with keyboard or joypad (and I needed it, as it's the only part of ColorPicker that I want to show).

This PR implements:

  • ability of both parts of picker shape to grab focus,
  • setting picker focus StyleBox in theme,
  • modifying picker cursor position with keyboard or joypad,
  • keeping focus on proper control while changing picker shape,
  • automatic focusing on the picker shape if it is visible and the HTML LineEdit is not visible (as it's what is currently being focused by default).

How it works in game and in the editor:

https://github.com/user-attachments/assets/0361be2d-364b-44d5-95d3-aa5ed930138a

https://github.com/user-attachments/assets/d5660772-ffb1-48e3-9817-60b75c14941e

I don't really like how it works with the controller, as I'm using gui_input to check for actions and the controller does not send echo events, but it's possible to use the controller at least.

FeniXb3 avatar Nov 17 '24 22:11 FeniXb3

I don't really like how it works with the controller, as I'm using gui_input to check for actions and the controller does not send echo events, but it's possible to use the controller at least.

We should make the controller send fake echo events in this case, so that various GUI nodes can handle these directly. However, this should also be done in a way that doesn't affect existing game logic, which is going to be difficult to handle. Perhaps this should only be done for the built-in actions (those starting with ui_)?

Edit: Proposal opened:

  • https://github.com/godotengine/godot-proposals/issues/11199

Calinou avatar Nov 20 '24 22:11 Calinou

The hue slider should go faster when you adjust it with the keyboard arrows or gamepad buttons. Compare its speed to the color value on the left:

color_value_vs_hue_slider.mp4

Ideally, the speed at which the hue is adjusted should match the color value on the left, which is probably 3× to 4× faster. This can be done by increasing the increment for each button press on the hue slider.

The hue slider goes exactly 3.6× slower than value slider in another shape, so your estimations were pretty correct. ^^ It is by design - value and saturation have values 0-100, but hue has values from range 0-360. Making each button press to increase hue in bigger increments always would miss some values. But I added a multiplier if the event is echo, so after few repetions it speeds up. But again, as for now it does not affect controllers.

What I'm wondering now is if we should have it configurable in the editor, both for games and for ColorPickers in the editor somehow (in editor settings?). What do you think @Calinou ?

When using a color picker with a wheel shape, moving upwards should "slide" along the circle when you've reached the top (same for any other edge):

color_value_wheel_slide.mp4

Right now, it stops so you need to manually move to the left then move back up again.

Fixed, please check it now:

https://github.com/user-attachments/assets/85d85fb5-f762-4709-b27c-e413fa811b07

I thought about making the cursor to spin around on the edge if you keep holding the same button, but while it went well for the HSV Wheel shape, it was buggy for both Circle shapes, so I stashed it for now. This is how it looked for HSV Wheel:

https://github.com/user-attachments/assets/7780bdbe-7e4e-4f8a-8a51-23dc5c7450ed

I am not sure if it is worth to try implementing it well for all Circle shapes or skip it, as I don't know if anyone will take advantage of this feature.

FeniXb3 avatar Nov 28 '24 15:11 FeniXb3

What I'm wondering now is if we should have it configurable in the editor, both for games and for ColorPickers in the editor somehow (in editor settings?). What do you think @Calinou ?

This would be a pretty niche option, so I'd focus on picking a good default instead.

Calinou avatar Nov 28 '24 18:11 Calinou

#82979 could be used for better joypad navigation if it was merged. For now the only way is implement the behavior using NOTIFICATION_PROCESS manually, like it was done in Slider for example.

Though the joypad navigation is a bit janky right now. When you enter the main color box, the only way out is either accepting or canceling; you can no longer select another element. There could be 2 focus modes here: one for navigating the elements and one for changing colors using the elements. You could switch them with accept/cancel. We have that in LineEdit. Though it's something that can be added later.

KoBeWi avatar Dec 02 '24 23:12 KoBeWi

The wheel is difficult to use. image You need to press the direction depending on the current rotation. It would be easier if you could press a button to start moving and then it would rotate as long as it's still pressed, without needing to press other buttons.

KoBeWi avatar Dec 03 '24 12:12 KoBeWi

#82979 could be used for better joypad navigation if it was merged. For now the only way is implement the behavior using NOTIFICATION_PROCESS manually, like it was done in Slider for example.

I did it like in Slider, thanks for the suggestion. I tried with get_vector in NOTIFICATION_INTERNAL_PROCESS before, but it was not working. Now I see that I might have missed calling set_process_internal(true) in the right moment.

Though the joypad navigation is a bit janky right now. When you enter the main color box, the only way out is either accepting or canceling; you can no longer select another element. There could be 2 focus modes here: one for navigating the elements and one for changing colors using the elements. You could switch them with accept/cancel. We have that in LineEdit. Though it's something that can be added later.

I added this second focus mode like in LineEdit. I thought about it before, but I didn't see benefits in my usecase so I skipped it before. It works well, for both LineEdit and ColorPicker picker shapes now if you use a ColorPicker directly. But if you show it via ColorPickerButton, it failes in both cases. The reason is that both accept and cancel actions close the popup that ColorPicker is in. You can see the behaviour on this video:

https://github.com/user-attachments/assets/7eeff182-4fee-495d-96ca-35dde341a4c3

I didn't bother to implement this as I use focus next/prev actions to navigate through ColorPicker's controls. I configured inputs for those actions to shoulder buttons and I was using them to test this feature. I forgot that by default there are no joypad buttons connected to those actions. Is there any reason this is not done by default? I wonder if I can simply configure them within this PR to make it work better out of the box.

The wheel is difficult to use. image You need to press the direction depending on the current rotation. It would be easier if you could press a button to start moving and then it would rotate as long as it's still pressed, without needing to press other buttons.

Done. I had it stashed, as I mentioned in one of the previous comments here, as I wasn't sure if I should try and implement similar behaviour for both circle shapes. In the end I separated their behaviours.

FeniXb3 avatar Dec 07 '24 21:12 FeniXb3

The reason is that both accept and cancel actions close the popup that ColorPicker is in.

You could try using accept_event() in the shape's gui input, to prevent the event from propagating.

KoBeWi avatar Dec 07 '24 21:12 KoBeWi

The reason is that both accept and cancel actions close the popup that ColorPicker is in.

You could try using accept_event() in the shape's gui input, to prevent the event from propagating.

I do. What I see now while debugging is that the ColorPickerPopupPanel::_input_from_window gets the input event before my gui_input handler so the accept_event() don't do much in this case. As tihs popup has no "OK" button, I can understand that accept action is a reasonable way to confirm the color choice,

FeniXb3 avatar Dec 07 '24 22:12 FeniXb3

I believe the feature is done with suggestions applied, if I didn't miss something. I squashed all the commits already as well.

I attach a sample project to make it easier to test it. It has configured ui next and prev actions on controller shoulder buttons and accept/cancel on A/B buttons of Xbox controller. godot-colorpicker-accessibility-test.zip

I believe those buttons should be configured in the InputMap by default, but I think it should be separate PR.

Is there anything else I should do?

FeniXb3 avatar Jan 04 '25 09:01 FeniXb3

The interactions are much better since I last checked. I agree that the cursor should appear in front of the focus outline. Other than that the behavior is fine.

KoBeWi avatar Jan 31 '25 22:01 KoBeWi

Shape menu can't be opened with gamepad. It appears for 1 frame and disappears. Using keyboard works fine. Might be unrelated though.

EDIT: I think the menu option is accepted immediately for some reason. Same happens with color mode.

KoBeWi avatar Feb 01 '25 22:02 KoBeWi

Shape menu can't be opened with gamepad. It appears for 1 frame and disappears. Using keyboard works fine. Might be unrelated though.

EDIT: I think the menu option is accepted immediately for some reason. Same happens with color mode.

That's interesting. Could you send me the project that you use to test it? You can current version of my test projects attached. colorpicker_test.zip I test it on both ColorPickerButton and ColorPicker straight in the tree, without the popup. The only usability issue I have is that ui_accept and ui_cancel both close the popup opened by ColorPickerButton. It makes most controls in any color picker created by ColorPickerButton unusable. You cannot change the shape, add preset or load palette, because it would close the popup right away. I wonder if we can introduce another action for closing the popup that would be bound to Ctrl+Enter or something like that. What do you think?

FeniXb3 avatar Feb 01 '25 22:02 FeniXb3

I use a random project with default ColorPicker on the scene. It's not project-specific.

EDIT: Ok you need to disable display/window/subwindows/embed_subwindows project setting for the bug to happen.

I wonder if we can introduce another action for closing the popup that would be bound to Ctrl+Enter or something like that. What do you think?

I think the main problem is that ui_accept will close the popup even if accepting something else, e.g. when you edit SpinBox value and press Enter, it will both accept the value and close the popup. The picker popup should be aware of focus and close only when not accepting input for another control. That's for another PR though.

KoBeWi avatar Feb 01 '25 23:02 KoBeWi

All controls mentioned by @Calinou should be accessible with keyboard and joypad now. You can now also add and remove presets. I added a new action for deleting preset, ui_colorpicker_delete_preset with default events Key::KEY_DELETE and JoyButton::X.

Tooltip for preset button describes how to use or remove the color with mouse: Zrzut ekranu 2025-02-01 084026

I have not pushed changes that dynamically add information about other events connected with deleting the color preset: Zrzut ekranu 2025-02-01 205542

I am not sure if this is a good idea. And if yes, should I manually modify translations in all *.po files to include extra %s or is there another workflow for that?

While experimenting with that I realized that Delete key is translated, at least in polish and I believe it should not be: Zrzut ekranu 2025-02-01 225842

Additionally, I finally decided to add default joypad events for:

  • ui_cancel - JoyButton::B
  • ui_accept - JoyButton::A
  • ui_focus_next - JoyButton::RIGHT_SHOULDER
  • ui_focus_prev - JoyButton::LEFT_SHOULDER

FeniXb3 avatar Feb 02 '25 00:02 FeniXb3

And if yes, should I manually modify translations in all *.po files to include extra %s or is there another workflow for that?

Translations are edited on Weblate. They are synchronized by maintainers, you should not edit .po files manually.

KoBeWi avatar Feb 02 '25 00:02 KoBeWi

Shape menu can't be opened with gamepad. It appears for 1 frame and disappears. Using keyboard works fine. Might be unrelated though.

EDIT: I think the menu option is accepted immediately for some reason. Same happens with color mode.

I use a random project with default ColorPicker on the scene. It's not project-specific.

EDIT: Ok you need to disable display/window/subwindows/embed_subwindows project setting for the bug to happen.

I realized that this is due to the nature of gamepad input being sent to every window whether it's focused or not. With this option disabled each popup or popup menu are treated as separate windows and all handle gamepad input, so ui_acccept is being processed by both the main window and the popup kinda simultaneously. You can see this behaviour on the video below:

https://github.com/user-attachments/assets/3d487215-17e0-4936-9ef6-315e07ec3fe1

It means that any UI with popups is inaccessible with gamepad if "embed subwindows" option is disabled. I will see if I can do some workarounds for ColorPicker, but it looks like a more general input design change for not embeded popups for a separate discussion and PR.

FeniXb3 avatar Feb 03 '25 19:02 FeniXb3

I rebased to the newest master and it seems that issues with PopupMenu's popup capturing ui_accept event from gamepad that opened it were fixed by someone else in the meantime. I pushed my changes that fix another issue when subwindows are not embedded that I showed before. Input on the main window is now blocked when any PopupMenu is opened in ColorPicker. I believe it would be good to handle it more generally in seperate PR.

I started to work on allowing picking a color from the screen with keyboard or gamepad, but it is not included here yet, as I have some cases still to handle.

https://github.com/user-attachments/assets/315b843c-29d1-4c5d-82b2-4a943d3782fd

Will it be better to have a separate PR for this or should I keep on working on this here?

FeniXb3 avatar Feb 20 '25 17:02 FeniXb3

This PR is already approved, so it's better to leave further changes for later if they are not necessary.

KoBeWi avatar Feb 20 '25 17:02 KoBeWi

Needs a rebase before this can be merged

Repiteo avatar Mar 07 '25 21:03 Repiteo

@Repiteo thanks for the info, I missed it. It's rebased now.

FeniXb3 avatar Mar 07 '25 21:03 FeniXb3

Thanks! Congratulations on your first merged contribution! 🎉

Repiteo avatar Mar 12 '25 01:03 Repiteo