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

frontend: Highlight control dock buttons using style class, Remove NonCheckableButton #11742

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gxalpha
Copy link
Member

@gxalpha gxalpha commented Jan 18, 2025

Description

Checkable buttons are troublesome because QAccessible interprets them as checkboxes, and doesn't pass the "clicked" signal to them (see QTBUG-110737). Instead, we only get the "toggled" signal, which also gets triggered by things other than user input (i.e., the button getting changed by the program).

Making them uncheckable means that they behave like normal buttons again. To style them, we can just add a class and address them that way.
Note that uncheckable here means actually not checkable - unlike the cursed NonCheckableButton class previously used that makes a button that is checkable ignore the user input (and only allow check state changes via code, effectively making it solely a styling tool). The disadvantage is that such a button still gets recognized by QAccessible as a checkable button (and by extension as a checkbox), which has the accessibility issues mentioned above.
We can remove the NonCheckableButton class and simply use the class property for styling.

Motivation and Context

Those buttons being inaccessible via screen readers has bugged me forever, and I previously had a few other attempts at fixing them (all of which had some issues and were never PR'd). This should be the proper fix.
Partially addresses #6200 (#6200 (comment)).

How Has This Been Tested?

Tested that the buttons still work when pressed by the user. Tested that the buttons now work when pressed via a screen reader (macOS VoiceOver in my case), and also that they get recognized as buttons (and not as checkboxes) by the screen reader.
Tested that when using websockets and starting a recording / virtual camera / entering studio mode, the buttons still get the appropriate state.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Code cleanup (non-breaking change which makes code smaller or more readable)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

Checkable buttons are troublesome because QAccessible interprets them as
checkboxes, and doesn't pass the "clicked" signal to them (see
QTBUG-110737). Instead, we only get the "toggled" signal, which also
gets triggered by things other than user input (i.e., the button getting
changed by the program).

Making them uncheckable means that they behave like normal buttons
again. To style them, we can just add a class and address them that way.
Note that uncheckable here means *actually* not checkable - unlike the
cursed NonCheckableButton class previously used that makes a button
that *is* checkable ignore the user input (and only allow check state
changes via code, effectively making it solely a styling tool).
The NonCheckableButton class was previously used for situations where a
*checkable* button was meant to not be checkable by the *user*, but only
through code. This was useful mostly as a styling tool (and to confuse
developers like me as to what it actually did).

The disadvantage is that such a button - a button that is actually
checkable (has the checkable attribute) but ignores the user - still
gets recognized by QAccessible as a checkable button, which has
accessibility issues when used like a normal button (see QTBUG-110737).

We can still get the styling effect on *actually* uncheckable buttons by
giving them a class, so this widget should not be necessary.
@gxalpha gxalpha added Bug Fix Non-breaking change which fixes an issue Code Cleanup Non-breaking change which makes code smaller or more readable Accessibility Improves accessibility of the program UI/UX Anything to do with changes or additions to UI/UX elements. labels Jan 18, 2025
@gxalpha gxalpha requested a review from Warchamp7 January 18, 2025 15:18
Copy link
Collaborator

@tytan652 tytan652 left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accessibility Improves accessibility of the program Bug Fix Non-breaking change which fixes an issue Code Cleanup Non-breaking change which makes code smaller or more readable UI/UX Anything to do with changes or additions to UI/UX elements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants