surge-synthesizer / surge

Synthesizer plug-in (previously released as Vember Audio Surge)
https://surge-synthesizer.github.io/
GNU General Public License v3.0
3.14k stars 399 forks source link

Use JUCE Accessibility branch to bring reasonable accessibility to Surge XT 1.0 #4616

Closed baconpaul closed 2 years ago

baconpaul commented 3 years ago

this is a checklist @baconpaul is keeping he hopes summarizing his reminders of what to do based on this thread. (Check marks mean things he has done on a branch but not merged.)

https://forum.juce.com/t/juce-accessibility-on-develop/45142/83

pitermach commented 2 years ago

Ah right, you probably want to make the keyboard equivalent CTRL as well for consistency. And come to think of it I don't believe up and down worked to adjust sliders which they probably also should.

For the next and previous preset keys, for up and down it isn't a problem, but perhaps it might be worth changing those shortcuts to something else so that it's less likely someone triggers them by accident when adjusting something? Going back to Pianoteq, they're using N and P for next/previous preset and command/CTRL+N and P for next/previous categories. I Don't think using CTRL/Command N is a good idea because that's usually the "new project" command, but perhaps using N and P for patches and Opt/ALt+N and P for categories would work well. Just some food for thought ☺️

mkruselj commented 2 years ago

Single letter keybinds are not a good idea because of virtual keyboard mode which we added to XT 1.0 - they'd clash most likely. Hence I would always prefer using a key modifier. In that case I really like Ctrl+arrows because it's nearby (but also because I, sneakily enough, changed Pianoteq's default keybinds to be like that, too!)...

pitermach commented 2 years ago

Ah. Sounds like having the ability to remap Surge's default key binds might be a good feature addition in a future release, but that's a discussion for another ticket. :D As long as one of the arrow keys would work to do the quantized steps I think we'll be good. For the change patch keys, if you wanted to remap to something with modifiers I suppose Alt/OPT+N and P for patches and alt+Shift+N/P for categories would solve that problem but shortcut keys are definitely a subjective thing, and I'm happy you have them in the first place because most plugins don't.

mkruselj commented 2 years ago

Alt+P requires a bit of a finger stretch, especially on laptops which usually don't have the right side Alt key. Whereas Ctrl+left/right arrows are all very close together (and I don't recall seeing a laptop that doesn't have Ctrl on either side)...

outsidepro-arts commented 2 years ago

Oh folks, I didn't know about CTRL+Arrows to quantize fine tunning... Excuse me... I've just checked it right now and this really works in Pitch slider... Again excuse me! then, this question no more actual. I thought the arrow keys are equivalent to mouse wheel behavior and didn't checked this modificator with arrow keys.

Then I have the next propotion. It might be superfluous, so community here can tell about a few words. This propotion concerns navigation comfort and may be omited for this first Surge version.

Surge has 10 blocks of synth controls. This is patch and categories controls, Surge Synth Controls, Global Controls, Oscillator Controls, Mixer Controls, Filter Controls, Envelope Controls, LFO Controls, FX Controls and Modulators. When we are navigating using Tab key, it wold take a huge time until we get needed slider or checkbox an opposite side of controls stream. Maybe makes it sense to bind the focus moving to each controls group to CTRL+Digits? We have 10 groups right now, so there are enough digits to bind all groups... If we are working with screen reader navigators, we don't need to press tab key untill work deadline reached🤣, but if an user works with system navigation... @baconpaul @pitermach @ScottChesworth @mkruselj

baconpaul commented 2 years ago

Oh that’s a good idea. I’m not sure how easy it is but I can look

baconpaul commented 2 years ago

By they way this is one of the risk with this project. Even the most “accessible” surge still had hundreds of controls. Even before introducing keyboard navigation and screen readers it is an enormous ui to navigate. I hope we can find our way to something useful!

ScottChesworth commented 2 years ago

If you can get it going, then Control+Tab to jump forward through groups and Control+Shift+Tab to reverse would be standardized and intuitive across operating systems.

baconpaul commented 2 years ago

ok i consolidated all of this in the above. i'll try to get another build together before christmas with this materially done so we can go into the first two weeks of new year frozen for final beta.

baconpaul commented 2 years ago

As well as consolidating feedback, I got a few small things off my checklist done this morning, including cleaning up the code quite a lot so the key bindings are in one spot and making shift-F10 work for show menu on the currently editable elements. (I need to add it to the others).

I'll continue to chip away. But from this point out the nightlies should be getting 'slowly better'.

And again, appreciate all the useful feedback here.

mkruselj commented 2 years ago

Here's a buglet: arrow keys don't work on macro sliders!

And maybe another: Shift+F10 only works after first clicking on a widget to focus it. TBH it should always work under mouse pointer (as well as when tab-focusing widgets!) no matter where it is (that's how it works in Windows too).

Another thing: I think we should only use up/down arrow keys. Left/right are used for patch/category switching, so now when a widget is focused, those shortcuts (Ctrl+left/right) don't work anymore, which is not quite how I envisioned the feature...

If these things are resolved, I personally feel we don't need a separate "use accessible keyboard shortcuts" menu option. It can all go under "use keyboard shortcuts" IMO (and enabled by default).

mkruselj commented 2 years ago

So actually after some further thought and discussion with @baconpaul... We've come at an impasse and we're asking you people for your thoughts!

@pitermach @outsidepro-arts @ScottChesworth

I am of the opinion (as mentioned above), that we don't need to have two separate options here - one for general keyboard shortcuts, one for accessibility keyboard shortcuts. They're all just shortcuts, and I feel there can be one map that covers both use cases. Now, the point of contention here are arrow keys, and the aforementioned clash with existing Ctrl+left/right and Ctrl+Shift+left/right bindings to change patches/categories.

I would like to know - is it a really big problem for accessibility if we designate arrows so that ONLY left-right are doing the slider value edits (so then also Shift+left/right does the finetune and Ctrl+left/right do the quantize), and we make up/down not do anything at all on sliders? Then we can change our patch/category change shortcuts to Ctrl+up/down and Ctrl+Shift+up/down.

Would this be a huge hurdle for visually impaired users? Or is it a matter of just documenting it properly and managing expectations that way?

@baconpaul's point is that arrows are all so close, and since we don't have undo, one wrong keypress could screw up a patch. I can definitely see that as a very valid argument, too. But I really really like Ctrl+some pair of arrows to walk the patches, as a regular user. It's super convenient, it's nearby (even on a laptop that has a butchered keyboard!), it doesn't require finger gymnastics... Reasons are many. We've discussed a few other possibilities, PgUp/PgDn (butchered on laptops, usually behind a Fn key...), < and > (not handy on a non-US keyboard like QWERTZ which I use for example)...

Please let us know!

ScottChesworth commented 2 years ago

I am of the opinion (as mentioned above), that we don't need to have two separate options here - one for general keyboard shortcuts, one for accessibility keyboard shortcuts. They're all just shortcuts, and I feel there can be one map that covers both use cases.

Agreed.

I would like to know - is it a really big problem for accessibility if we designate arrows so that ONLY left-right are doing the slider value edits (so then also Shift+left/right does the finetune and Ctrl+left/right do the quantize), and we make up/down not do anything at all on sliders? Then we can change our patch/category change shortcuts to Ctrl+up/down and Ctrl+Shift+up/down.

Here's another approach to consider that would be more standardized and avoids changing the shortcuts you already know/use:

  1. Prev/next preset/category stay where they are now. Left/Right arrows do nothing when focus is on sliders.
  2. Slider value adjustment moves to Up/Down arrows, Shifted for finetune, add Control for quantized adjustment.
  3. Add Home/End to move to 0% and 100% respectively, PageUp/PageDown to jump in maybe 10% or 20% increments.

That way, you've provided a great level of control over sliders, with zero upheaval for folks who are already using keystrokes to surf sounds.

Would this be a huge hurdle for visually impaired users? Or is it a matter of just documenting it properly and managing expectations that way?

I can pretty much guarantee that the proposal above will be what screen reader users will try intuitively. Definitely still document it though, it shows you're being intentional about accessibility.

@baconpaul's point is that arrows are all so close, and since we don't have undo, one wrong keypress could screw up a patch.

If that's a concern that applies to all users and you have a method to protect against mistakes intended for all users then great, we can roadtest that protection and report back. You don't need to go out of your way to protect against screen reader users pressing wrong keys based on proximity, though. If that's happening to you in situ, my guess is that it's some combination of 1) you're probably not used to interacting this way and 2) maybe you're trying to test without looking at your fingers to simulate our UX? The short answer to those points is that it's not a fair comparison like it might seem on the surface, because 1) this is our primary method of interacting, and 2) hundreds or thousands of hours of doing that builds up muscle memory. I'm not saying that no screen reader user ever hits the wrong button, I'm just saying that you don't need to protect against it, because we develop tactile muscle memory for the keyboards/keystrokes we use most often, just like how the mouse you've used for the longest time inevitably feels the most like home when it's in your hand.

mkruselj commented 2 years ago

This is a spectacularly useful comment. Thank you very much!

baconpaul commented 2 years ago

Agreed. Fantastic. Will adjust code today

pitermach commented 2 years ago

Yep, that was a great suggestion ☺️

Speaking of hotkeys, I don't know if anyone mentioned this yet but you should also make sure that in addition to Shift+F10 the "Application" key also opens menus. THat's the key which is to the left of right CTRL on full-size keyboards. It's what that key does in most Windows apps so it's very likely people will try to use it in Surge as well. If this already works then I apologize - nome of the keyboards I have on my desk right now have this key so I can't easily test this.

mkruselj commented 2 years ago

That key is slowly becoming extinct and is usually replaced with a Fn button. I only have it on my old W7 laptop...

But valid point, we should support that one if JUCE has a keycode for it.

pitermach commented 2 years ago

True, as I said my current keyboard doesn't have one either. However, until recently the Reaper keymap most blind people used required extensive use of this key - to the point that many laptop users ran scripts to simulate it. As a result, there's going to be a lot of people using full keyboards who are used to bringing up menus this way so I think it's worth doing for consistency.

mkruselj commented 2 years ago

No problem! I just discovered that JUCE doesn't have a constant for this KeyPress, but I have found which keycode it really is (it's 93) so we can totally implement that!

pitermach commented 2 years ago

That'd be great, thanks again!

outsidepro-arts commented 2 years ago

Hello folks!

I ask @pitermach @ScottChesworth and other active testers to check one trouble in Windows: the accessibility model doesn't consider the real elements position. I.e., if you'll try to route mouse to the navigator (for example, if we are using NVDA, the keyboard shortcut is NVDA+NumPad Slash) the mouse will be routed outside the Surge window (if the zoom value does not covers the all space). If the zoom value is set to largest, the mouse will be routed to left corner of the Surge window. If you'll click on some element which calls something (for example, patches menu) using physical mouse, the model synchronises with real UI till the next action with navigator. I may guess that is problem of Juce, but I've tested UVI Falcon at yesterday (it uses Juce, but no accessibility enhancements, only Juce core accessibility model works) and there's no the same problem. Maybe a some method should be called to sync the accessibility model when interface initializes... Please note that keyboard navigation works well and if you'll navigate to some UI element, then try to route navigator to system focus and finally route mouse to navigator, it will be performed if this element has real UI control representation.

pitermach commented 2 years ago

@outsidepro-arts I had a quick look at this, at least as much as I can without being able to see what's happening. With Zoom set to the largest option, no issues on Mac, I can root the mouse to all controls and it goes there no problem.

On Windows, with Both NVDA and JAWS most controls are still reachable for me with the exception of a few of the lowest most sliders like the last few LFO controls. There NVDA clicks outside of Surge into the Reaper window, but below the plugin rather than the top left corner like it does for you.

This makes me think what might be happening is the window would need to be scrolled to see those last few sliders, which would explain why this works on Mac because VO always tries to get controls scrolled into view, while no Windows screen reader I'm aware of does that. You could also be running into NVDA's high DPI resolution glitches if you have a big enough screen (mine is only 1600X900 so it's not high DPI)

Either way though like you said this works fine on normal zoom and since every control can be tabbed to and interacted with from the keyboard so IDK how much of a problem this is.

outsidepro-arts commented 2 years ago

@pitermach It is little problem while we haven't group by group navigation. Also, some elements require click on via mouse - they don't performed by the default action executing... Therefore I'm executing every GUI control using mouse cursor. It is not problem for me - I often use mouse cursor ("Golden Cursor", build-in mouse control via keyboard, routing, OCR and etc). When you're working with sound or music professionaly, all means are neccessary.

By the way, just interesting thing: if Falcon or Odin synthesizer has been runnen in just started system, this problem goes away - Surge starts to sync accessibility model correctly. The synthesizers listed above written in Juce framework too and Juce provides the basic accessibility model there.

Maybe it is problem of my system specifically... That's why I asked all folks to tell about this bug watching...

pitermach commented 2 years ago

The system thing may very well be a factor - I found NVDA's mouse emulation all over the place on various computers I've used, from working great to clicking where it shouldn't.

I just had another look and realized that the buttons that open menus, like for the oscillator type indeed don't respond to NVDA+Enter which is a bit odd, but they respond fine to both JAWS and Narrator's equivalent so I'm gonna put this one on NVDA's UIA implementation.

Once these controls start responding to enter or the arrow keys to open them, this shouldn't be as much of a problem.

On 20 Dec 2021, at 20:02, Outsidepro @.***> wrote:

@pitermach https://github.com/pitermach It is little problem while we haven't group by group navigation. Also, some elements require click on via mouse - they don't performed by the default action executing... Therefore I'm executing every GUI control using mouse cursor. It is not problem for me - I often use mouse cursor ("Golden Cursor", build-in mouse control via keyboard, routing, OCR and etc). When you're working with sound or music professionaly, all means are well.

By the way, just interesting thing: if Falcon or Odin synthesizer has been runnen in just started system, this problem goes away - Surge starts to sync accessibility model correctly.

Maybe it is problem of my system specifically... That's why I asked all folks to tell about this bug watching...

— Reply to this email directly, view it on GitHub https://github.com/surge-synthesizer/surge/issues/4616#issuecomment-998191040, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAABTNF6VKTCTSWXO7SX4LUR54TJANCNFSM45YHRRLA. Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub. You are receiving this because you were mentioned.

outsidepro-arts commented 2 years ago

Acessibility bug found: the LFO type group and FX slots does not returns the child elements. @baconpaul

baconpaul commented 2 years ago

Thanks! Appreciate all this feedback and promise I will turn to it shortly!

pitermach commented 2 years ago

Got 2 more things for you:

Regarding your menu checklist, it looks like the modulation items are skipped over with the arrow keys - referring to options like edit and clear modulation when you have one added. There are also some menu items that show up as a blank item to screen readers that can be arrowed to that don't seem to do anything when clicked.

Also, the save patch dialog has a few things that can make it confusing to get to:

Some more ideas on how this could be improved:

outsidepro-arts commented 2 years ago

Hello, @pitermach!

There are also some menu items that show up as a blank item to screen readers that can be arrowed to that don't seem to do anything when clicked.

These blank items are widgets which have a control elements near. When you're focusing on such items, just continue navigating via screen reader's navigator. As i see, it cannot be accessible in current implementation, but it really productive method for interracting with.

baconpaul commented 2 years ago

OK! Lots to do here!

I will have some chunks of time next week to knock of much of this before the new year. Appreciate the feedback.

pitermach commented 2 years ago

No worries, take your time. These last 2 can be worked around if you know where to look for the controls.

I can confirm what @outsidepro-arts said though in that we have a regression with the LFO type now appearing as an empty group. This is happening in NIGHTLY-2021-12-22-4f93b15, though I think the issue appeared in an earlier build.

While trying to figure out what version I was running before when I noticed the LFO issue, I discovered that the about dialog is also currently not visible to accessibility when it's open.

On 22 Dec 2021, at 15:08, Paul @. @.>> wrote:

OK! Lots to do here!

I will have some chunks of time next week to knock of much of this before the new year. Appreciate the feedback.

— Reply to this email directly, view it on GitHub https://github.com/surge-synthesizer/surge/issues/4616#issuecomment-999604091, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAABTOQL4XRUSFPDLERKJDUSHLXNANCNFSM45YHRRLA. Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub. You are receiving this because you were mentioned.

baconpaul commented 2 years ago

Ha OK so I at least have made a push so the about dialog is screen readable, the URLs are clickable, and the copy option is accessible. I haven't fixed (or even really consolidated) the rest of the bugs here yet, but this is my dev priority next week with a hope to get it stable by the new year.

baconpaul commented 2 years ago

OK folks with the next nightly (in about an hour, so around 10pm US Eastern time) we will be much much further along. In the last week I've fixed a bunch of tab order, keyboard behavior, focus, visibility, and other things.

The big thing I still haven't done is the tab order on radio groups. Right now it still tabs to parent then each individual item. I know how to make it tab to just selected and keep focus on just selected, but the code is a bit tricky and there's been lots of other bits and bobs.

There's a couple of things which I couldn't fix (couldn't make ctrl-tab swap groups for the life of me) and one thing which is actually a juce problem I'm discussing with the team (the extra blanks in the menus in a screen reader), so the 1.0 release, which we still plan for around Jan 15 or so, will have some accessibility gotchas still. We're confident we will have to do a 1.1 once the code gets in the wild though.

but just dropping a note here since

  1. Things are a lot better
  2. Probably tomorrow (I hope) I'll get the tab order thing done. If not then Friday
  3. So some testing over NYE weekend or earlier would be lovely

Thanks again for all the help and support here. This has been a tricky feature. I definitely underestimated it.

pitermach commented 2 years ago

You're very welcome. Like we said before, I can't say enough how much all this is appreciated. Accessibility is very much complex like you said, and the fact you're probably the first synth with so many features to implement this and because you're working with a toolkit that also implemented all this very recently isn't making it any easier.

Thanks for the wavetable name fixes, I can also confirm I can edit the LFO sequencer with the arrow keys. I also had a look at the modulation overview window, which doesn't seem to have a tab order at the moment but using it through screen reader navigation is very clear.

Regarding CTRL+Tab, would it be easier to implement hotkeys to jump to the first control in the major groups instead? You could do something like this:

When the new version comes out, I could help writing a page for the Surge wiki on some tips how to use it with a screen reader if you like, this could also be a good place to put any known issues and how to work around them for the first version, though this should probably wait until we know for sure what won't make the 1.0 release.

baconpaul commented 2 years ago

oh right the modlist has 'natural' tab order - i can fix that.

Interesting idea on those extended modifiers! @mkruselj thoughts?

I was also thinking of alt-shift-G and alt-ctrl-G to go to next or prior group or some such.

mkruselj commented 2 years ago

Alt+Shift+O is quite a Rachmaninoff stretch (can't use AltGr because that's considered as Ctrl+Alt). Other than that I agree with that proposal.

So maybe instead of Alt+Shift we use Ctrl+Shift for these shortcuts?

I think for jumping between sections Ctrl+Tab and Ctrl+Shift+Tab should be used, because it's familiar from web browsers. Unless screen readers already take those shortcuts for something else?

ScottChesworth commented 2 years ago

I think for jumping between sections Ctrl+Tab and Ctrl+Shift+Tab should be used, because it's familiar from web browsers. Unless screen readers already take those shortcuts for something else?

Those are familiar keystrokes yep. NO screen reader that I know of commandeers them.

baconpaul commented 2 years ago

Yeah I couldn’t get the event delivered in juce so o need to figure out what is eating it. The group jump may not make 1.0

outsidepro-arts commented 2 years ago

Hello @baconpaul and all here!

Thanks again for all the help and support here. This has been a tricky feature. I definitely underestimated it.

And we're sincerely tell you many thanks! You've did big work and we really appreciated that. You're first person who interested this experience quite deep. I'll not tire to tell you cheers in every my comment everywhere!

@pitermach:

Regarding CTRL+Tab, would it be easier to implement hotkeys to jump to the first control in the major groups instead? You could do something like this:

Also, we can implement the digits keys with something modificators. I don't think that CTRL+5 might be used in Surge somewhere. ☺

mkruselj commented 2 years ago

Numbers are a bit less intuitive to remember rather than O = oscillators, M = modulators, F = filters...

pitermach commented 2 years ago

Numbers went through my head as well when writing that but the key suggestions I ended up going with follow what Windows apps usually do of using the first letter of a function unless it's taken at which point you use the next available one (which is how I ended up with I for mixer or X for FX). But over all I think doing it this way makes more sense yeah.

A couple other observations:

baconpaul commented 2 years ago

Oh I didn’t do home and end on sliders. Let me

baconpaul commented 2 years ago

And adding acc to the fx bank is an obvious and easy thing. It is a much simpler plugin!

baconpaul commented 2 years ago

Oh and thanks for all the kind words of course!

baconpaul commented 2 years ago

And yeah I added check mark visibility just this morning when I realized we didn’t show it. I may have to ask the juce team about that - will see

baconpaul commented 2 years ago

Unfortunately the 'isTicked' property on a menu doesn't seem to come through to juce accesibility now. I've raised that with the JUCE team (https://forum.juce.com/t/accessible-menu-items-and-isticked/49455) but it is unlikely that our 1.0 release will have these menu ticks be screen reader aware, I'm sorry.

Working on the rest of the items this weekend though.

baconpaul commented 2 years ago

OK I just did a push that gets as much as I can do done for 1.0. Only thing left is the step sequencer triggers and any bugs you find.

Some things turned out to be hard to fix in juce and i've opened a series of issues with the juce devs; some things i couldn't make work without changes too big this late in the cycle. So I have opened a new issue #5714 for post-release accesibility

But when the new nightly drops (around 3pm eastern time today) most of the items you've all raised should be in place, including tab ordering into radio groups, more of the popups having accessibility, fewer 'invisible' elements and generally better reader layout, etc...

Welcome feedback as always. We are still planning to release 1.0 in the next couple of weeks but we can make changes if you find them (including the change for step triggers I didn't get today).

Thanks! and Happy new Year!

baconpaul commented 2 years ago

One other change coming in about 90 minutes. The ticked menu status doesn't come through but we decided to patch our copy of juce to add the word " (Ticked)" to menu items with a tick mark in the screen reader for now. This doesn't display in the GUI (where you get the checkmark) just in the accessible interface.

My guess is that nightly is up by about 6pm eastern.

ScottChesworth commented 2 years ago

we decided to patch our copy of juce to add the word " (Ticked)" to menu items with a tick mark in the screen reader for now.

Excellent! Two tiny tweaks that would make a big difference to UX:

  1. Use the word "checked" instead of "ticked". It's pretty much universal screen reader terminology.
  2. You probably don't want the parentheses. That'll cause most screen readers to pause between reading the end of the menu item and speaking "checked". It's natural to assume that separation would be a clearer UX, but in practice it eats into productivity, because it forces the user to scroll through each item in the menu, listen to each option to completion and then pause before they can move along to the next item in order to ascertain what's selected. At slower speech rates with the more human-sounding voices in particular, those pauses can be huge!
mkruselj commented 2 years ago

So it would be better to do comma, checked instead of parentheses, ticked? Super easy to change.

ScottChesworth commented 2 years ago

So it would be better to do comma, checked instead of parentheses, ticked?

Comma will also force a pause, and more often than not, the same length pause as parentheses. I reckon you want "  checked". The two spaces should ensure that the final word of the menu option doesn't ever get run together with the word checked.

FYI, here's a line dumped direct from NVDA screen reader speech buffer when I land on a ticked menu option in Reaper's menu system: "Record mode: normal  checked  r"

As you can hopefully see now after my 87th attempt at editing, NVDA separates the menu option, the selection status and the shortcut key using double space.