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 22 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
ae03bc4
Modify color in ColorPicker in HSV Rectangle mode with keyaboard or j…
FeniXb3 Nov 7, 2024
68526dd
Modify color in ColorPicker in VHS Circle mode with keyboard or j…
FeniXb3 Nov 10, 2024
f921894
Modify color in ColorPicker in OKHSL Circle mode with keyboard or…
FeniXb3 Nov 10, 2024
e83b406
Modify color in ColorPicker in HSV Wheel mode with keyboard or jo…
FeniXb3 Nov 12, 2024
3b9dd30
Modify color in ColorPicker in HSV Wheel mode with keyboard or jo…
FeniXb3 Nov 12, 2024
540cad0
Check separate input actions instead of getting vector in ColorPicker
FeniXb3 Nov 12, 2024
32c2877
Use themable stylebox for ColorPicker shape focus
FeniXb3 Nov 15, 2024
58a0e62
Use another hack to draw focus stle around HSV wheel
FeniXb3 Nov 15, 2024
6ddefed
Handle ColorPicker focus change when picker was focused while chaingi…
FeniXb3 Nov 16, 2024
7c3caa0
Fix ColorPicker HSV wheel regression of not chainging hue by keyboard
FeniXb3 Nov 16, 2024
8fe8590
Focus on picker shape if HTML textedit is not visible when activating…
FeniXb3 Nov 16, 2024
e4b8f5a
Use default button focus style for ColorPicker shape focus
FeniXb3 Nov 16, 2024
b583267
Merge branch 'master' into colorpicker-picker-shape-accessibility
FeniXb3 Nov 16, 2024
deaf7a3
Use dedicated control to draw focus stylebox over HSV wheel in ColorP…
FeniXb3 Nov 17, 2024
1fafafb
Cleanup
FeniXb3 Nov 17, 2024
4bd57ef
Add documentation for ColorPicker's picker_focus theme item
FeniXb3 Nov 17, 2024
8143904
Cleanup
FeniXb3 Nov 17, 2024
95a22d0
Fix documentation
FeniXb3 Nov 18, 2024
c7fddb2
Modify comments and simplify code according to review suggestions.
FeniXb3 Nov 20, 2024
39526a6
Simplify checking if color change vector is zero
FeniXb3 Nov 20, 2024
b2fef7a
Apply suggestions from code review
FeniXb3 Nov 21, 2024
3abc8f8
Merge branch 'master' into colorpicker-picker-shape-accessibility
FeniXb3 Nov 25, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions doc/classes/ColorPicker.xml
Original file line number Diff line number Diff line change
Expand Up @@ -201,5 +201,8 @@
<theme_item name="shape_rect_wheel" data_type="icon" type="Texture2D">
The icon for rectangular wheel picker shapes.
</theme_item>
<theme_item name="picker_focus" data_type="style" type="StyleBox">
[StyleBox] used when the picker is focused. The [theme_item picker_focus] [StyleBox] is displayed [i]over[/i] the picker shape, so a partially transparent [StyleBox] should be used to ensure the picker shape remains visible. A [StyleBox] that represents an outline or an underline works well for this purpose. To disable the focus visual effect, assign a [StyleBoxEmpty] resource. Note that disabling the focus visual effect will harm keyboard/controller navigation usability, so this is not recommended for accessibility reasons.
FeniXb3 marked this conversation as resolved.
Show resolved Hide resolved
</theme_item>
</theme_items>
</class>
2 changes: 2 additions & 0 deletions editor/themes/editor_theme_manager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1751,6 +1751,8 @@ void EditorThemeManager::_populate_standard_styles(const Ref<EditorTheme> &p_the
p_theme->set_constant("label_width", "ColorPicker", 10 * EDSCALE);
p_theme->set_constant("center_slider_grabbers", "ColorPicker", 1);

p_theme->set_stylebox("picker_focus", "ColorPicker", p_config.button_style_focus);

p_theme->set_icon("screen_picker", "ColorPicker", p_theme->get_icon(SNAME("ColorPick"), EditorStringName(EditorIcons)));
p_theme->set_icon("shape_circle", "ColorPicker", p_theme->get_icon(SNAME("PickerShapeCircle"), EditorStringName(EditorIcons)));
p_theme->set_icon("shape_rect", "ColorPicker", p_theme->get_icon(SNAME("PickerShapeRectangle"), EditorStringName(EditorIcons)));
Expand Down
191 changes: 188 additions & 3 deletions scene/gui/color_picker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,23 @@ void ColorPicker::set_focus_on_line_edit() {
callable_mp((Control *)c_text, &Control::grab_focus).call_deferred();
}

void ColorPicker::set_focus_on_picker_shape() {
switch (_get_actual_shape()) {
case SHAPE_HSV_RECTANGLE:
callable_mp(w_edit, &Control::grab_focus).call_deferred();
break;
case SHAPE_HSV_WHEEL:
callable_mp(wheel_h_focus_display, &Control::grab_focus).call_deferred();
break;
case SHAPE_VHS_CIRCLE:
case SHAPE_OKHSL_CIRCLE:
callable_mp(wheel_uv, &Control::grab_focus).call_deferred();
break;
default: {
}
}
}

void ColorPicker::_update_controls() {
int mode_sliders_count = modes[current_mode]->get_slider_count();

Expand Down Expand Up @@ -282,13 +299,29 @@ void ColorPicker::_update_controls() {

switch (_get_actual_shape()) {
case SHAPE_HSV_RECTANGLE:
wheel_edit->hide();
w_edit->show();
uv_edit->show();
btn_shape->show();

if (wheel_uv->has_focus()) {
uv_edit->grab_focus();
} else if (wheel_h_focus_display->has_focus()) {
w_edit->grab_focus();
}

wheel_edit->hide();
wheel_h_focus_display->hide();
break;
case SHAPE_HSV_WHEEL:
wheel_edit->show();
wheel_h_focus_display->show();

if (w_edit->has_focus()) {
wheel_h_focus_display->grab_focus();
} else if (uv_edit->has_focus()) {
wheel_uv->grab_focus();
}

w_edit->hide();
uv_edit->hide();
btn_shape->show();
Expand All @@ -297,20 +330,37 @@ void ColorPicker::_update_controls() {
case SHAPE_VHS_CIRCLE:
wheel_edit->show();
w_edit->show();

if (uv_edit->has_focus()) {
wheel_uv->grab_focus();
} else if (wheel_h_focus_display->has_focus()) {
w_edit->grab_focus();
}

uv_edit->hide();
btn_shape->show();
wheel->set_material(circle_mat);
circle_mat->set_shader(circle_shader);
wheel_h_focus_display->hide();
break;
case SHAPE_OKHSL_CIRCLE:
wheel_edit->show();
w_edit->show();

if (uv_edit->has_focus()) {
wheel_uv->grab_focus();
} else if (wheel_h_focus_display->has_focus()) {
w_edit->grab_focus();
}

uv_edit->hide();
btn_shape->show();
wheel->set_material(circle_mat);
circle_mat->set_shader(circle_ok_color_shader);
wheel_h_focus_display->hide();
break;
case SHAPE_NONE:
wheel_h_focus_display->hide();
wheel_edit->hide();
w_edit->hide();
uv_edit->hide();
Expand Down Expand Up @@ -391,6 +441,8 @@ void ColorPicker::_slider_value_changed() {
h = sliders[0]->get_value() / 360.0;
s = sliders[1]->get_value() / 100.0;
v = sliders[2]->get_value() / 100.0;

hsv_keyboard_picker_cursor_position = Vector2i(0, 0);
last_color = color;
}

Expand Down Expand Up @@ -741,6 +793,7 @@ void ColorPicker::set_picker_shape(PickerShapeType p_shape) {
btn_shape->set_button_icon(shape_popup->get_item_icon(p_shape));
}

hsv_keyboard_picker_cursor_position = Vector2i(0, 0);
current_shape = p_shape;

#ifdef TOOLS_ENABLED
Expand Down Expand Up @@ -808,6 +861,7 @@ void ColorPicker::_set_mode_popup_value(ColorModeType p_mode) {
} else {
set_color_mode(p_mode);
}
hsv_keyboard_picker_cursor_position = Vector2i(0, 0);
}

Variant ColorPicker::_get_drag_data_fw(const Point2 &p_point, Control *p_from_control) {
Expand Down Expand Up @@ -1129,6 +1183,7 @@ void ColorPicker::_hsv_draw(int p_which, Control *c) {
if (!c) {
return;
}
Rect2 focus_rect = Rect2(Point2(), c->get_size());

PickerShapeType actual_shape = _get_actual_shape();
if (p_which == 0) {
Expand Down Expand Up @@ -1210,6 +1265,9 @@ void ColorPicker::_hsv_draw(int p_which, Control *c) {
Size2 real_size(c->get_size().x - corner_x * 2, c->get_size().y - corner_y * 2);
x = CLAMP(real_size.x * s, 0, real_size.x) + corner_x - (theme_cache.picker_cursor->get_width() / 2);
y = CLAMP(real_size.y - real_size.y * v, 0, real_size.y) + corner_y - (theme_cache.picker_cursor->get_height() / 2);
if (c == wheel_uv) {
focus_rect = Rect2(Point2(corner_x, corner_y), real_size);
}
}
c->draw_texture(theme_cache.picker_cursor, Point2(x, y));

Expand Down Expand Up @@ -1287,6 +1345,11 @@ void ColorPicker::_hsv_draw(int p_which, Control *c) {
circle_mat->set_shader_parameter("v", v);
}
}

if (c->has_focus()) {
RID ci = c->get_canvas_item();
theme_cache.picker_focus->draw(ci, focus_rect);
}
}

void ColorPicker::_slider_draw(int p_which) {
Expand All @@ -1308,6 +1371,7 @@ void ColorPicker::_uv_input(const Ref<InputEvent> &p_event, Control *c) {
real_t rad = center.angle_to_point(bev->get_position());
h = ((rad >= 0) ? rad : (Math_TAU + rad)) / Math_TAU;
s = CLAMP(dist / center.x, 0, 1);
hsv_keyboard_picker_cursor_position = Vector2i(0, 0);
} else {
return;
}
Expand All @@ -1320,7 +1384,7 @@ void ColorPicker::_uv_input(const Ref<InputEvent> &p_event, Control *c) {
bev->get_position().y < corner_y || bev->get_position().y > c->get_size().y - corner_y) {
{
real_t dist = center.distance_to(bev->get_position());

wheel_h_focus_display->grab_focus();
if (dist >= center.x * 0.84 && dist <= center.x) {
real_t rad = center.angle_to_point(bev->get_position());
h = ((rad >= 0) ? rad : (Math_TAU + rad)) / Math_TAU;
Expand Down Expand Up @@ -1375,6 +1439,7 @@ void ColorPicker::_uv_input(const Ref<InputEvent> &p_event, Control *c) {
real_t rad = center.angle_to_point(mev->get_position());
h = ((rad >= 0) ? rad : (Math_TAU + rad)) / Math_TAU;
s = CLAMP(dist / center.x, 0, 1);
hsv_keyboard_picker_cursor_position = Vector2i(0, 0);
} else {
if (spinning) {
real_t rad = center.angle_to_point(mev->get_position());
Expand All @@ -1400,6 +1465,85 @@ void ColorPicker::_uv_input(const Ref<InputEvent> &p_event, Control *c) {
emit_signal(SNAME("color_changed"), color);
}
}

// TODO: Is it better to cast and check it or to not check it and get vector?
Ref<InputEventKey> kev = p_event;
Ref<InputEventJoypadButton> jbev = p_event;
Ref<InputEventJoypadMotion> jmev = p_event;

if (kev.is_valid() || jbev.is_valid() || jmev.is_valid()) {
// TODO: Consider adding new ui actions specific to ColorPicker, like the ones used for LineEdit
FeniXb3 marked this conversation as resolved.
Show resolved Hide resolved
Vector2 color_change_vector = Vector2();
FeniXb3 marked this conversation as resolved.
Show resolved Hide resolved
if (p_event->is_action_pressed("ui_left", true)) {
color_change_vector.x -= 1;
} else if (p_event->is_action_pressed("ui_right", true)) {
color_change_vector.x += 1;
} else if (p_event->is_action_pressed("ui_up", true)) {
color_change_vector.y -= 1;
} else if (p_event->is_action_pressed("ui_down", true)) {
color_change_vector.y += 1;
}

if (!Math::is_zero_approx(color_change_vector.length())) {
FeniXb3 marked this conversation as resolved.
Show resolved Hide resolved
if (actual_shape == SHAPE_HSV_RECTANGLE) {
s = CLAMP(s + color_change_vector.x / 100.0, 0, 1);
v = CLAMP(v - color_change_vector.y / 100.0, 0, 1);
} else if (actual_shape == SHAPE_VHS_CIRCLE || actual_shape == SHAPE_OKHSL_CIRCLE) {
Vector2 center = c->get_size() / 2.0;

// TODO: It's a hack, as it messes up if I calculate it this way always
FeniXb3 marked this conversation as resolved.
Show resolved Hide resolved
if (hsv_keyboard_picker_cursor_position.x == 0 && hsv_keyboard_picker_cursor_position.y == 0) {
FeniXb3 marked this conversation as resolved.
Show resolved Hide resolved
hsv_keyboard_picker_cursor_position.x = center.x + (center.x * Math::cos(h * Math_TAU) * s);
hsv_keyboard_picker_cursor_position.y = center.y + (center.y * Math::sin(h * Math_TAU) * s);
}

real_t potential_new_cursor_distance = center.distance_to(hsv_keyboard_picker_cursor_position + color_change_vector);
if (potential_new_cursor_distance <= center.x) {
hsv_keyboard_picker_cursor_position += color_change_vector;
}

real_t dist = center.distance_to(hsv_keyboard_picker_cursor_position);
real_t rad = center.angle_to_point(hsv_keyboard_picker_cursor_position);
h = ((rad >= 0) ? rad : (Math_TAU + rad)) / Math_TAU;
s = CLAMP(dist / center.x, 0, 1);
} else if (actual_shape == SHAPE_HSV_WHEEL) {
if (c == wheel_uv) {
s = CLAMP(s + color_change_vector.x / 100.0, 0, 1);
v = CLAMP(v - color_change_vector.y / 100.0, 0, 1);
} else if (c == wheel_h_focus_display) {
int h_change = 0;

if (Math::is_equal_approx(h, 0) || Math::is_equal_approx(h, 0.5f) || Math::is_equal_approx(h, 1)) {
FeniXb3 marked this conversation as resolved.
Show resolved Hide resolved
color_change_vector.x = 0;
} else if (Math::is_equal_approx(h, 0.25f) || Math::is_equal_approx(h, 0.75f)) {
color_change_vector.y = 0;
}

if (h > 0 && h < 0.5) {
h_change -= color_change_vector.x;
} else if (h > 0.5 && h < 1) {
h_change += color_change_vector.x;
}

if (h > 0.25 && h < 0.75) {
h_change -= color_change_vector.y;
} else if (h < 0.25 || h > 0.75) {
h_change += color_change_vector.y;
}

h_change = CLAMP(h_change, -1, 1);
h = Math::wrapf(h + h_change / 360.0, 0, 1);
}
}

accept_event();
_copy_hsv_to_color();
last_color = color;
set_pick_color(color);

emit_signal(SNAME("color_changed"), color);
}
}
}

void ColorPicker::_w_input(const Ref<InputEvent> &p_event) {
Expand Down Expand Up @@ -1452,6 +1596,31 @@ void ColorPicker::_w_input(const Ref<InputEvent> &p_event) {
emit_signal(SNAME("color_changed"), color);
}
}

// TODO: Is it better to cast and check it or to not check it and get axis?
Ref<InputEventKey> kev = p_event;
Ref<InputEventJoypadButton> jbev = p_event;
Ref<InputEventJoypadMotion> jmev = p_event;

if (kev.is_valid() || jbev.is_valid() || jmev.is_valid()) {
// TODO: It should be done in process instead of input to handle joypads better
// TODO: Consider adding new ui actions specific to ColorPicker, like the ones used for LineEdit
float color_change = Input::get_singleton()->get_axis("ui_up", "ui_down");
if (!Math::is_zero_approx(color_change)) {
if (actual_shape == SHAPE_HSV_RECTANGLE) {
h = CLAMP(h + color_change / 360.0, 0, 1);
} else if (actual_shape == SHAPE_VHS_CIRCLE || actual_shape == SHAPE_OKHSL_CIRCLE) {
v = CLAMP(v - color_change / 100.0, 0, 1);
}

accept_event();
_copy_hsv_to_color();
last_color = color;
set_pick_color(color);

emit_signal(SNAME("color_changed"), color);
}
}
}

void ColorPicker::_slider_or_spin_input(const Ref<InputEvent> &p_event) {
Expand Down Expand Up @@ -1647,6 +1816,7 @@ void ColorPicker::_html_focus_exit() {
} else {
_update_text_value();
}
hsv_keyboard_picker_cursor_position = Vector2i(0, 0);
}

void ColorPicker::set_can_add_swatches(bool p_enabled) {
Expand Down Expand Up @@ -1795,6 +1965,8 @@ void ColorPicker::_bind_methods() {

BIND_THEME_ITEM(Theme::DATA_TYPE_CONSTANT, ColorPicker, center_slider_grabbers);

BIND_THEME_ITEM(Theme::DATA_TYPE_STYLEBOX, ColorPicker, picker_focus);

BIND_THEME_ITEM(Theme::DATA_TYPE_ICON, ColorPicker, screen_picker);
BIND_THEME_ITEM(Theme::DATA_TYPE_ICON, ColorPicker, expanded_arrow);
BIND_THEME_ITEM(Theme::DATA_TYPE_ICON, ColorPicker, folded_arrow);
Expand Down Expand Up @@ -1832,6 +2004,7 @@ ColorPicker::ColorPicker() {
hb_edit->add_child(uv_edit);
uv_edit->connect(SceneStringName(gui_input), callable_mp(this, &ColorPicker::_uv_input).bind(uv_edit));
uv_edit->set_mouse_filter(MOUSE_FILTER_PASS);
uv_edit->set_focus_mode(FOCUS_ALL);
uv_edit->set_h_size_flags(SIZE_EXPAND_FILL);
uv_edit->set_v_size_flags(SIZE_EXPAND_FILL);
uv_edit->connect(SceneStringName(draw), callable_mp(this, &ColorPicker::_hsv_draw).bind(0, uv_edit));
Expand Down Expand Up @@ -1970,13 +2143,23 @@ ColorPicker::ColorPicker() {
wheel->set_mouse_filter(MOUSE_FILTER_PASS);
wheel->connect(SceneStringName(draw), callable_mp(this, &ColorPicker::_hsv_draw).bind(2, wheel));

// TODO: Hack - I cannot draw focus stylebox on 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));

wheel_uv = memnew(Control);
wheel_margin->add_child(wheel_uv);
wheel_uv->set_focus_mode(FOCUS_ALL);
wheel_uv->connect(SceneStringName(gui_input), callable_mp(this, &ColorPicker::_uv_input).bind(wheel_uv));
wheel_uv->connect(SceneStringName(draw), callable_mp(this, &ColorPicker::_hsv_draw).bind(0, wheel_uv));

w_edit = memnew(Control);
hb_edit->add_child(w_edit);
w_edit->set_focus_mode(FOCUS_ALL);
w_edit->set_h_size_flags(SIZE_FILL);
w_edit->set_v_size_flags(SIZE_EXPAND_FILL);
w_edit->connect(SceneStringName(gui_input), callable_mp(this, &ColorPicker::_w_input));
Expand Down Expand Up @@ -2087,7 +2270,9 @@ void ColorPickerButton::pressed() {
float v_offset = show_above ? -minsize.y : get_size().y;
popup->set_position(get_screen_position() + Vector2(h_offset, v_offset));
popup->popup();
if (DisplayServer::get_singleton()->has_hardware_keyboard()) {
if (!picker->is_hex_visible() && picker->get_picker_shape() != ColorPicker::SHAPE_NONE) {
picker->set_focus_on_picker_shape();
} else if (DisplayServer::get_singleton()->has_hardware_keyboard()) {
picker->set_focus_on_line_edit();
}
}
Expand Down
Loading