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

keep the library focused when any deck controls or the overview are clicked #2307

Merged
merged 3 commits into from
Nov 17, 2019

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Sep 30, 2019

trying to fix https://bugs.launchpad.net/mixxx/+bug/1259040
"Clicking on deck controls affects library keyboard focus"

I added 'setFocusPolicy(Qt::NoFocus);' to the setup slot of most interactive skin widgets, like it worked for the library Preview button in #2264 .
This allows to keep the library focused for keyboard interaction when any deck controls or the overview are clicked.

I didn't apply the fix to the effect selector or spinboxes because I don't overlook the consequences and IMO there's no point to keep the library focused when interacting with those widgets.

This is a quite naive attempt but it works for buttons, sliders, knobs ...
It's so simple that I just can't believe there are no side effects. What can could wrong?
Is there a better place to set the focus policy?
Can it safely be set in wbasewidget ?

New behaviour:

  • click buttons, knobs, sliders, overview: library table/sidebar remains focused
  • effect selector: library is re-focused when any effect wass selected
  • beat size spinboxes accept focus as before

this allows to keep the library focused for keyboard interaction
when any deck controls or the overview are clicked.
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Thank you this works like a charm, there is only a code style issue.

@@ -26,4 +26,5 @@ void WEffectButtonParameter::setup(const QDomNode& node, const SkinContext& cont
SKIN_WARNING(node, context)
<< "EffectButtonParameter node could not attach to effect parameter";
}
this->setFocusPolicy(Qt::NoFocus);
Copy link
Member

Choose a reason for hiding this comment

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

this-> is redundant and should be removed.

Copy link
Member Author

@ronso0 ronso0 Oct 4, 2019

Choose a reason for hiding this comment

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

because this is a wpushbutton actually. label..
will look into this again and fix it

@ronso0
Copy link
Member Author

ronso0 commented Oct 5, 2019

I'm done.

For the sake of completeness it would be nice to disallow click focus in the scrolling waveforms, but I don't find the right spot to fix it there, also I don't really want to mess with the waveforms ;)

@uklotzde uklotzde added the ui label Oct 20, 2019
@uklotzde uklotzde added this to the 2.3.0 milestone Oct 20, 2019
@Be-ing
Copy link
Contributor

Be-ing commented Oct 23, 2019

Doesn't this make everything aside from the library inaccessible to screen readers?

@ronso0
Copy link
Member Author

ronso0 commented Oct 23, 2019

AFAIK there wouldn't be a drawback for screenreaders.
Screenreaders are operated by keyboard using mostly Tab and arrow keys to focus UI elements and convert those elements' 'tags' (title, alt etc. in html) for use with text-to-speech engines or tangible braille devices.
In Mixxx Tab/arrow keys and modifiers can only be used to navigate the main menubar, the library and the sidebar - buttons, knobs, sliders, waveform etc. can't be focused this way, neither can they be toggled with spacebar or Alt-Return or whatever. Our keyboard mappings are supposed to fill that gap.
Beatsize spinboxes and the effect selectors CAN be focused by mouse but AFAICT there's no way to focus any of them specificly with keyboards.

just asked @DJ-Ray about this https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/Optimzation.20for.20blind.20users/near/178858554

@daschuer
Copy link
Member

Oh, I just see the redudnat "this->" is still there. Please remove it and this is geady for merge.

@ronso0
Copy link
Member Author

ronso0 commented Nov 15, 2019

Done.

@daschuer
Copy link
Member

LGTM, Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
2.3 release
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants