Bug 443469 - Button focus state almost invisible
Summary: Button focus state almost invisible
Status: RESOLVED FIXED
Alias: None
Product: Breeze
Classification: Plasma
Component: QStyle (show other bugs)
Version: 5.23.0
Platform: openSUSE Linux
: NOR major
Target Milestone: ---
Assignee: Plasma Bugs List
URL:
Keywords: regression
: 444625 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-10-08 09:08 UTC by Fabian Vogt
Modified: 2021-10-30 00:14 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In: 5.24


Attachments
Dialog box with focused button (27.86 KB, image/png)
2021-10-08 09:08 UTC, Fabian Vogt
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Fabian Vogt 2021-10-08 09:08:44 UTC
Created attachment 142254 [details]
Dialog box with focused button

The active/focused button in dialogs is almost impossible to tell apart from other buttons. The only indication appears to be that the 1px border is blue instead of grey, see the attached screenshot.

This was still easily visible in 5.22.90.
Comment 1 Noah Davis 2021-10-08 14:30:51 UTC
I see this was posted today. Is this really supposed to be marked as Resolved Fixed?
Comment 3 Noah Davis 2021-10-08 14:44:29 UTC
Reopening this since that commit didn't actually fix focus visuals, just autoDefault being disabled.
Comment 4 Nate Graham 2021-10-08 14:54:57 UTC
So the blue background for autodefaultable dialog buttons is now broken? Or this is a complaint that using a single-pixel line for focus in general (not for default buttons) is a regression compared to the previous style of changing the whole button background?

If so I'm not sure that's super valid since we always used a single-pixel outline for focus for everything else in the past like text fields and spinboxes and scrollviews; buttons were the exception and they looked weird for it. Regardless, your focus ring changes targeted at 5.24 are definitely the best way forward IMO.
Comment 5 Noah Davis 2021-10-08 15:04:10 UTC
(In reply to Nate Graham from comment #4)
> So the blue background for autodefaultable dialog buttons is now broken?

The auto default visuals are not general focus visuals. They're only for QPushButtons inside of QDialogs.

> Or this is a complaint that using a single-pixel line for focus in
> general (not for default buttons) is a regression compared to the
> previous style of changing the whole button background?

I'm pretty sure that's what Fabian meant, but feel free to correct me if I'm wrong, Fabian.

> If so I'm not sure that's super valid since we always used a single-pixel
> outline for focus for everything else in the past like text fields and
> spinboxes and scrollviews; buttons were the exception and they looked weird
> for it. Regardless, your focus ring changes targeted at 5.24 are definitely
> the best way forward IMO.

Interactive controls need stronger focus visuals than views. The other controls you mentioned had other ways to show focus besides the outline. Editable controls have the text cursor and checkboxes/radio buttons had the text underline. QSliders and QDials were the one type of interactive control with focus visibility as poor as git master.
Comment 6 Nate Graham 2021-10-08 15:08:51 UTC
(In reply to Noah Davis from comment #5)
> Interactive controls need stronger focus visuals than views. The other
> controls you mentioned had other ways to show focus besides the outline.
> Editable controls have the text cursor and checkboxes/radio buttons had the
> text underline. QSliders and QDials were the one type of interactive control
> with focus visibility as poor as git master.
...And tabs, and checkboxes, and radio buttons (especially with no text or short text).
Comment 7 Noah Davis 2021-10-08 17:10:58 UTC
Git commit 586e7462080900bc8375a7973ca7624574c0b704 by Noah Davis.
Committed on 08/10/2021 at 17:00.
Pushed by ngraham into branch 'master'.

kstyle: Add QFocusFrame to non-view/delegate interactive widget

There are 2 parts that contain the bulk of the code in this patch.

Style::event() is used to apply the QFocusFrame to a widget when it gets
focus with a keyboard input related focus reason. If the focused widget
has a focusProxy, this makes sure the QFocusFrame is applied to the
focusProxy instead.

Style::drawFocusFrame() is mostly what you'd expect. It draws a focus
frame based on the type of widget the QFocusFrame was applied to. I had
to do a workaround for QFocusFrame not fully repainting ouside the
bounds of QSliders and QDials when the handle moves though. What I do is
check if `_focusFrame` is defined and then `_focusFrame->update()`.

M  +252  -3    kstyle/breezestyle.cpp
M  +7    -0    kstyle/breezestyle.h

https://invent.kde.org/plasma/breeze/commit/586e7462080900bc8375a7973ca7624574c0b704
Comment 8 Fabian Vogt 2021-10-08 20:21:54 UTC
To clarify: Originally this bug was about the regression that buttons in dialogs were suddenly not highlighted properly in 5.23.0.

Then I was told that the background highlight is just the indication of the "active default" button and the 1px color change is the usual focus behaviour actually visible outside of dialog buttons as well. Apparently I just never really noticed that before, because most buttons are usually default(able) buttons. For other controls like text input, there's also other indication like selected text, a cursor appearing or a label getting an underline.

So IMO it's important to improve the visbility of button focus not just for this case, but also in general.
Amazingly the MR already landed, so I'm looking forward to 5.24.
Comment 9 Noah Davis 2021-10-29 22:26:35 UTC
*** Bug 444625 has been marked as a duplicate of this bug. ***
Comment 10 Noah Davis 2021-10-30 00:06:00 UTC
Git commit 6515aa667d797453da8476c1291a618349af2fdd by Noah Davis.
Committed on 30/10/2021 at 00:05.
Pushed by ndavis into branch 'cherry-pick-586e7462'.

kstyle: Add QFocusFrame to non-view/delegate interactive widget

There are 2 parts that contain the bulk of the code in this patch.

Style::event() is used to apply the QFocusFrame to a widget when it gets
focus with a keyboard input related focus reason. If the focused widget
has a focusProxy, this makes sure the QFocusFrame is applied to the
focusProxy instead.

Style::drawFocusFrame() is mostly what you'd expect. It draws a focus
frame based on the type of widget the QFocusFrame was applied to. I had
to do a workaround for QFocusFrame not fully repainting ouside the
bounds of QSliders and QDials when the handle moves though. What I do is
check if `_focusFrame` is defined and then `_focusFrame->update()`.


(cherry picked from commit 586e7462080900bc8375a7973ca7624574c0b704)

M  +252  -3    kstyle/breezestyle.cpp
M  +7    -0    kstyle/breezestyle.h

https://invent.kde.org/plasma/breeze/commit/6515aa667d797453da8476c1291a618349af2fdd
Comment 11 Bug Janitor Service 2021-10-30 00:14:38 UTC
A possibly relevant merge request was started @ https://invent.kde.org/plasma/breeze/-/merge_requests/160