Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve ColorPicker picker shape keyboard and joypad accessibility #99374

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

FeniXb3
Copy link

@FeniXb3 FeniXb3 commented Nov 17, 2024

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:

ColorPicker.keyaboard.and.joypad.game.mp4
ColorPicker.keyboard.editor.mp4

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 FeniXb3 requested review from a team as code owners November 17, 2024 22:49
@KoBeWi KoBeWi added this to the 4.x milestone Nov 17, 2024
@AThousandShips AThousandShips changed the title Colorpicker picker shape keyboard and joypad accessibility Improve ColorPicker picker shape keyboard and joypad accessibility Nov 18, 2024
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I support this usability improvement in the proposal design, but it needs technical review.

scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
@Calinou
Copy link
Member

Calinou commented Nov 20, 2024

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:

scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
doc/classes/ColorPicker.xml Outdated Show resolved Hide resolved
@Repiteo Repiteo modified the milestones: 4.x, 4.4 Nov 22, 2024
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, it works as expected.

Some usability comments:

  • 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.

  • 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.

Comment on lines 2167 to 2173
// HACK: I cannot draw focus stylebox on the wheel itself, as it's drawing based on shader.
wheel_h_focus_display = memnew(Control);
wheel_margin->add_child(wheel_h_focus_display);
wheel_h_focus_display->set_mouse_filter(MOUSE_FILTER_PASS);
wheel_h_focus_display->set_focus_mode(FOCUS_ALL);
wheel_h_focus_display->connect(SceneStringName(draw), callable_mp(this, &ColorPicker::_hsv_draw).bind(3, wheel_h_focus_display));
wheel_h_focus_display->connect(SceneStringName(gui_input), callable_mp(this, &ColorPicker::_uv_input).bind(wheel_h_focus_display));
Copy link
Member

@Calinou Calinou Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to draw a special circle focus stylebox (using a dedicated StyleBoxFlat in the editor theme/default project theme), so it matches the wheel's shape. This can be left for a future PR as it's not essential.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I modified the theme to have separate styleboxes - one for rectangle-shaped elements and another for circle-shaped elements:
obraz

obraz

@FeniXb3
Copy link
Author

FeniXb3 commented Nov 28, 2024

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:

Nagrywanie.ekranu.2024-11-28.163308.mp4

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:

Nagrywanie.ekranu.2024-11-28.160945.mp4

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 FeniXb3 requested a review from Calinou November 28, 2024 15:43
@Calinou
Copy link
Member

Calinou commented Nov 28, 2024

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.

@akien-mga akien-mga requested a review from KoBeWi December 2, 2024 13:21
@KoBeWi
Copy link
Member

KoBeWi commented Dec 2, 2024

#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
Copy link
Member

KoBeWi commented Dec 3, 2024

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.

@@ -267,6 +274,8 @@ class ColorPicker : public VBoxContainer {
void _sample_draw();
void _hsv_draw(int p_which, Control *c);
void _slider_draw(int p_which);
int get_wheel_h_change(Vector2 color_change_vector);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused.

Comment on lines 2216 to 2217
// HACK: I cannot draw focus stylebox on the wheel itself, as it's drawing based on shader.
wheel_h_focus_display = memnew(Control);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you use wheel_uv for that?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In one moment I did. But it meant that I had to add logic for navigating between two focus modes for this control, capturing next/prev actions, because wheel_uv would be used to handle focus for both parts of HSV Wheel.

It added more complexity to the class that is already overloaded with a lot of ifs and was not working well always (like it was sometimes looping focus between those two states when holding ui_focus_prev action instead of leaving the control and switching to the hex LineEdit. I thought that adding another control for both drawing the focus stylebox and making switching focus more reliable is a better choice.

If it's better to have few more ifs than using this empty control, I can work on it again.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#99515 makes the code cleaner.
If your PR happens to be merged first, I'll simplify it and integrate with my changes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the code to not use a dedicated new Control for handling focus of wheel in HSV Wheel shape. Now the wheel control can grab focus in this shape, while focus stylebox is drawn on wheel_uv. I had whith wheel_uv handling everything and working well with ui_focus_prev and ui_focus_next, but it ui_up was glitching and I choose approach that seemed to work always with some tweaks, without introducing a new control.

scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
scene/gui/color_picker.h Outdated Show resolved Hide resolved
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
@FeniXb3 FeniXb3 force-pushed the colorpicker-picker-shape-accessibility branch 2 times, most recently from 9826b67 to bf82ac0 Compare December 27, 2024 15:50
@FeniXb3 FeniXb3 force-pushed the colorpicker-picker-shape-accessibility branch from bf82ac0 to 84dcfa0 Compare January 4, 2025 09:24
@FeniXb3
Copy link
Author

FeniXb3 commented Jan 4, 2025

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 FeniXb3 force-pushed the colorpicker-picker-shape-accessibility branch from 84dcfa0 to b9495ba Compare January 14, 2025 14:40
@Repiteo Repiteo modified the milestones: 4.4, 4.5 Jan 20, 2025
@FeniXb3 FeniXb3 force-pushed the colorpicker-picker-shape-accessibility branch from b9495ba to c90efdb Compare January 21, 2025 16:22
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
scene/gui/color_picker.h Outdated Show resolved Hide resolved
scene/gui/color_picker.h Outdated Show resolved Hide resolved
scene/gui/color_picker.h Outdated Show resolved Hide resolved
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
@FeniXb3 FeniXb3 force-pushed the colorpicker-picker-shape-accessibility branch 3 times, most recently from 4fe7a5c to 74e506f Compare January 23, 2025 17:08
@FeniXb3 FeniXb3 force-pushed the colorpicker-picker-shape-accessibility branch from 74e506f to aae0af2 Compare January 31, 2025 06:13
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, it works as expected.

Some more feedback from things I noticed. These aren't critical and can be implemented in future PRs, as this PR already provides a decent baseline for keyboard/gamepad usability:

  • It doesn't appear to be possible to select these buttons using the keyboard or gamepad only (highlighted in yellow). This includes using Tab on a keyboard:

image

  • Is it possible to ensure the cursor on the color picker always appears in front of the focus outline? Right now, it'll appear behind the focus outline when at the edges of a circular (or square) picker shape.

image

@KoBeWi
Copy link
Member

KoBeWi commented Jan 31, 2025

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.

@FeniXb3 FeniXb3 force-pushed the colorpicker-picker-shape-accessibility branch 2 times, most recently from 1a6056f to 21e3195 Compare February 1, 2025 21:40
@FeniXb3 FeniXb3 requested a review from a team as a code owner February 1, 2025 21:40
@KoBeWi
Copy link
Member

KoBeWi commented Feb 1, 2025

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.

@FeniXb3 FeniXb3 force-pushed the colorpicker-picker-shape-accessibility branch from 21e3195 to c0e8c14 Compare February 1, 2025 22:03
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
@FeniXb3 FeniXb3 force-pushed the colorpicker-picker-shape-accessibility branch 2 times, most recently from 5ded5aa to 96265b2 Compare February 1, 2025 22:47
@FeniXb3
Copy link
Author

FeniXb3 commented Feb 1, 2025

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?

@KoBeWi
Copy link
Member

KoBeWi commented Feb 1, 2025

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.

@FeniXb3 FeniXb3 force-pushed the colorpicker-picker-shape-accessibility branch from 96265b2 to da7ceb7 Compare February 2, 2025 00:47
@FeniXb3
Copy link
Author

FeniXb3 commented Feb 2, 2025

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

@KoBeWi
Copy link
Member

KoBeWi commented Feb 2, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants