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

Update the size of the scrollbar for the editor on touchscreen devices #71657

Conversation

m4gr3d
Copy link
Contributor

@m4gr3d m4gr3d commented Jan 19, 2023

Add a theme usability setting which updates the touch area of UI elements (e.g: scrollbar) for the editor on touchscreen devices.
This PR updates the scroll dimension to improve the scrolling experience on touchscreen.

Screenshot_20230122_072412

Screenshot_20230118_161549

3.x version

@EricEzaM
Copy link
Contributor

I don't know what the current state of using /editor in /scene is like, but FYI:
#29730
#53295 (comment)

@akien-mga
Copy link
Member

I wonder if this could be set via the editor theme instead? CC @YuriSizov @Calinou

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Jan 19, 2023

I wonder if this could be set via the editor theme instead? CC @YuriSizov @Calinou

@akien-mga I tried that approach first, and the current theming options for the scrollbar do not allow to control its width/height.

I can look into adding a scroll_size theme option if that's the preferred approach. That'd help with avoiding the dependency on editor.

@YuriSizov
Copy link
Contributor

YuriSizov commented Jan 19, 2023

the current theming options for the scrollbar do not allow to control its width/height.

You should be able to with the margins of the scroll stylebox, but for some reason we use StyleBoxTexture here, which has some weird behavior in that regard. I'm not sure why we still use a texture stylebox in the editor theme, so that's probably something to fix first. Granted, even with a texture the stylebox should be responsible for controlling the minimum size, so maybe that's also worth fixing.

I recall there was some recent talk, a proposal, or maybe even a PR about these stylebox size values. I'll try to look it up.
Edit: Found it godotengine/godot-proposals#5903

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Jan 19, 2023

the current theming options for the scrollbar do not allow to control its width/height.

You should be able to with the margins of the scroll stylebox, but for some reason we use StyleBoxTexture here, which has some weird behavior in that regard. I'm not sure why we still use a texture stylebox in the editor theme, so that's probably something to fix first. Granted, even with a texture the stylebox should be responsible for controlling the minimum size, so maybe that's also worth fixing.

I recall there was some recent talk, a proposal, or maybe even a PR about these stylebox size values. I'll try to look it up.

I played around for a couple hours with the margins on the StyleBoxTexture, but I was still unable to achieve the result of setting a larger custom minimum size for the scroll bar (also used the theme editor to quickly try different margin values).
I also tried switching and experimenting with StyleBoxFlat, but it seems to have fixed dimensions that are not configurable.

@YuriSizov
Copy link
Contributor

YuriSizov commented Jan 19, 2023

It's accounted for here:

Size2 ScrollBar::get_minimum_size() const {
Ref<Texture2D> incr = theme_cache.increment_icon;
Ref<Texture2D> decr = theme_cache.decrement_icon;
Ref<StyleBox> bg = theme_cache.scroll_style;
Size2 minsize;
if (orientation == VERTICAL) {
minsize.width = MAX(incr->get_size().width, (bg->get_minimum_size() + bg->get_center_size()).width);
minsize.height += incr->get_size().height;
minsize.height += decr->get_size().height;
minsize.height += bg->get_minimum_size().height;
minsize.height += get_grabber_min_size();
}
if (orientation == HORIZONTAL) {
minsize.height = MAX(incr->get_size().height, (bg->get_center_size() + bg->get_minimum_size()).height);
minsize.width += incr->get_size().width;
minsize.width += decr->get_size().width;
minsize.width += bg->get_minimum_size().width;
minsize.width += get_grabber_min_size();
}
return minsize;
}

The problem is that this takes a sum of the min size of the stylebox (which is just its margins) and the center size. Normally, the center size doesn't do anything, but in the StyleBoxTexture it is computed as the size of the texture minus the min size. So the resulting size for the scrollbar becomes min_size + (texture_size - min_size) = texture_size. That seems to be very much incorrect as it removes any kind of control. (Edit: I have found the aforementioned discussion about those methods, godotengine/godot-proposals#5903, and indeed, it seems to be a hack for this specific case, but it's not done correctly).

It should work just fine with just margins if it's StyleBoxFlat though. Not sure what could've been a problem.

@YuriSizov
Copy link
Contributor

I made #71686, which should make it possible to properly implement this.

@m4gr3d m4gr3d changed the title Update the size of the scrollbar for the editor on touchscreen devices [WIP] Update the size of the scrollbar for the editor on touchscreen devices Jan 21, 2023
@m4gr3d m4gr3d force-pushed the increase_scroll_bar_size_for_touchscreen_main branch from 4c1d02f to fc6c7bc Compare January 22, 2023 07:15
@m4gr3d m4gr3d changed the title [WIP] Update the size of the scrollbar for the editor on touchscreen devices Update the size of the scrollbar for the editor on touchscreen devices Jan 22, 2023
@Calinou
Copy link
Member

Calinou commented Jan 22, 2023

Could we keep the scrollbars visually as thin as they were before, but increase their draggable area (e.g. using expand/content margin)? It'd probably look better.

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Jan 22, 2023

Could we keep the scrollbars visually as thin as they were before, but increase their draggable area (e.g. using expand/content margin)? It'd probably look better.

@Calinou It's bad UX for touchscreen devices as it creates confusion for the user:

  • The target UI element is obscured by the user's finger on interaction, so it needs to be large enough visually so the user can properly assess whether they're touching it.
  • The scrollbars being visually thin also means that neighboring UI elements may occupy the area that falls within the the scrollbar's draggable area. This creates confusion when the user attempts to interact with those UI elements.

Also, I'm not sure if there's capability for that. The expand margins do the opposite, i.e: they keep the draggable/touch area the same but expand the render area (see https://docs.godotengine.org/en/stable/classes/class_styleboxflat.html#class-styleboxflat-property-expand-margin-bottom).

I took the approach of providing a new TouchScreen theme to provide users with flexibility. Users can keep the Default theme if they want the current look for the scrollbar, or switch to the TouchScreen theme if they're having difficulty interacting with the scrollbars on a touchscreen device.

@YuriSizov
Copy link
Contributor

YuriSizov commented Jan 22, 2023

I took the approach of providing a new TouchScreen theme to provide users with flexibility.

It's probably better as a settings flag that affects any theme, so you can still pick any default theme, or choose your own colors, while keeping this accessibility feature available.

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Jan 22, 2023

I took the approach of providing a new TouchScreen theme to provide users with flexibility.

It's probably better as a settings flag that affects any theme, so you can still pick any default theme, or choose your own colors, while keeping this accessibility feature available.

Sounds like a good option! I'll update the PR.

@m4gr3d m4gr3d force-pushed the increase_scroll_bar_size_for_touchscreen_main branch from fc6c7bc to 3ba8bf0 Compare January 22, 2023 15:21
@m4gr3d m4gr3d requested a review from a team as a code owner January 22, 2023 15:21
…ents (e.g: scrollbar) for the editor on touchscreen devices
@m4gr3d m4gr3d force-pushed the increase_scroll_bar_size_for_touchscreen_main branch from 3ba8bf0 to 63c88df Compare January 28, 2023 08:21
@akien-mga akien-mga requested a review from YuriSizov January 28, 2023 12:41
@@ -453,6 +453,7 @@ void EditorSettings::_load_defaults(Ref<ConfigFile> p_extra_config) {

// Theme
EDITOR_SETTING(Variant::STRING, PROPERTY_HINT_ENUM, "interface/theme/preset", "Default", "Default,Breeze Dark,Godot 2,Gray,Light,Solarized (Dark),Solarized (Light),Black (OLED),Custom")
EDITOR_SETTING(Variant::BOOL, PROPERTY_HINT_NONE, "interface/theme/enable_touchscreen_touch_area", DisplayServer::get_singleton()->is_touchscreen_available(), "")
Copy link
Member

Choose a reason for hiding this comment

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

I'll check but this might make it appear on my Linux laptop with touchscreen, which would be unwanted.
We probably want to use the same criteria as for the 3D viewport touch sticks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The setting uses the same default as the 3D viewport touch sticks.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, it seems DisplayServer::is_touchscreen_available() is only implemented on Android, iOS and Web currently.

I think if/when it gets properly implemented on other platforms we might start getting complaints about those. We should probably rethink that eventually and see if we should differentiate between supporting and preferring touchscreen input.

$ rg ::is_touchscreen_available
servers/display_server.cpp
389:bool DisplayServer::is_touchscreen_available() const {
658:    ClassDB::bind_method(D_METHOD("is_touchscreen_available"), &DisplayServer::is_touchscreen_available, DEFVAL(SCREEN_OF_MAIN_WINDOW));

platform/ios/display_server_ios.mm
581:bool DisplayServerIOS::is_touchscreen_available() const {

platform/android/display_server_android.cpp
239:bool DisplayServerAndroid::is_touchscreen_available() const {

platform/web/display_server_web.cpp
591:bool DisplayServerWeb::is_touchscreen_available() const {

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

Theme changes look fine.

@akien-mga akien-mga merged commit 62b0bfd into godotengine:master Jan 30, 2023
@akien-mga
Copy link
Member

Thanks!

@m4gr3d m4gr3d deleted the increase_scroll_bar_size_for_touchscreen_main branch January 30, 2023 16:05
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.

5 participants