Meter toolbar navigation & accessibility #3696

vsverchinsky · 2022-09-27T00:29:16Z

Resolves: #3230
Resolves: #3810
Resolves: #3484

I signed CLA
The title of the pull request describes an issue it addresses
If changes are extensive, then there is a sequence of easily reviewable commits
Each commit's message describes its purpose and effects
There are no behavior changes unnecessary for the stated purpose of the PR

Recommended:

Each commit compiles and runs on my machine without known undesirable changes of behavior

DavidBailes · 2022-09-27T10:11:43Z

I'm not sure what the current design spec is for this, but what is read by screen readers is different from my initial suggestion: #2779 (comment)
What I was expecting was that when you tabbed to the button containing the playback/recording icon and the slider, screen readers would read something like
"record meter peak -30db button", and
"recording volume slider -4db",
respectively.

However, what gets read by nvda when tabbing to the button and slider is:
"Record button", and
"Record Meter Peak -60 dB slider 46"
respectively.
I think the current implementation is less clear and more confusing compared to what I was expecting.

There are also some issues to do which how the slider is read when its value is changed, but these are secondary to the above issue.

DavidBailes · 2022-09-28T09:14:05Z

@LWinterberg . Given that the scale for the recording volume slider presented visually is wrong (#3690),
I think it would be better if the values of the volume slider read by screen readers were in the range 0 to 100, rather than incorrect ones in the range -60db to 0db. I think that would be far less confusing. What do you think?

Tantacrul · 2022-09-30T13:31:58Z

EDIT: ignore my previous comment. Actually, I think we should still display the meter the way it is for recording. However, we should display a % value while dragging the slider - and I think this should be passed to the screen reader too.

vsverchinsky · 2022-10-04T09:55:50Z

@DavidBailes @Tantacrul I've updated a PR, can you please tell if it's alright now?

Tantacrul · 2022-10-04T16:40:15Z

My days! I am really struggling to figure out how to tab to the record volume slider (using MacOS) in order to change it and hear a screenreader 😦.

Is it mainly a Windows thing? Need tutorial!

Apart from that two quick points:

When changing the slider I can see a colon before the %. So it says :77%. I suppose it should say Recording volume: 77%. The same with the play volume too.

When I hover over the 'Add effect' button, Voice Over (MacOS) calls it a 'Taste'. (!) Are we aware of this strangeness? I guess This should be raised as a separate issue. I was just curious if anyone had any thoughts.

Paul-Licameli · 2022-10-05T12:02:18Z

src/toolbars/MeterToolBar.cpp

@@ -187,7 +188,7 @@ void MeterToolBar::Populate()
   if( mWhichMeters & kWithPlayMeter ){
      mPlaySetupButton = safenew AButton(this);
      mPlaySetupButton->SetLabel({});
-      mPlaySetupButton->SetName(_("Playback"));
+      mPlaySetupButton->SetName(wxString::Format(_("Playback meter peak %d db"), -DecibelScaleCutoff.GetDefault()));


Did you confirm that the buttons reformat after the value is changed and the Preferences dialog is dismissed?

If that doesn't work, then I maybe commit 3598fe4 added enough calls to Invalidate().

DavidBailes · 2022-10-05T12:54:34Z

@DavidBailes @Tantacrul I've updated a PR, can you please tell if it's alright now?

Just to let you know that I've started to have a look at this. It clearly isn't complete, and I'll add detailed comments soon.

DavidBailes · 2022-10-07T14:10:37Z

Some comments on the volume sliders:

The names of the sliders are currently "record meter" and "play meter". They are not meters! In 3.1.3 the names of the sliders in the mixer bar were "recording volume" and "playback volume". The current names should be changed to something similar to those in 3.1.3.
The step sizes for the arrow keys and page up/down keys are too big. They should be the same as in 3.1.3: step size of 1 percent for arrow keys, 20% for page up/down.
When as slider is the focus, the value of the slider is read by the screen reader periodically, for no good reason. This is caused by the value change event created in MeterPanel::UpdateSliderControl(). The creation of this event should be removed from this function.
When the sliders are changed using the keyboard, the new value is not read by screen readers. I presume that adding the creation of a value change event at some suitable point in MeterPanel::SetMixer() would fix that.

Comments on the class MeterAx:

This looks like it's only been partly changed to be appropriate to the sliders - for example, MeterAx::GetName() is still the code from when this was a meter, not a slider. (Code along these lines will be needed for the accessibility of the meter buttons ).
ASliderAx provides a good template for what MeterAx should be like. Note that ASliderAx::GetName() isn't actually needed, as WindowAccessible::GetName() does the same thing.
Currently the behaviour of some of the member functions of MeterAx depends on the value of MeterPanel::mAccSilent, however this no longer needs to be the case, now that MeterAx represents a slider. MeterPanel::mAccSilent was introduced at commit 3d420e0, and may well be needed for the accessibility of the Meter buttons.

I'll add some comments on the meter buttons shortly.

Paul-Licameli · 2022-10-07T20:59:49Z

src/toolbars/MeterToolBar.cpp

      /* i18n-hint: (noun) The meter that shows the loudness of the audio being recorded.*/
      mRecordMeter->SetName( XO("Record Meter"));
      /* i18n-hint: (noun) The meter that shows the loudness of the audio being recorded.
       This is the name used in screen reader software, where having 'Meter' first
       apparently is helpful to partially sighted people.  */
      mRecordMeter->SetLabel( XO("Meter-Record") );
-      mSizer->Add( mRecordMeter, wxGBPosition( 0, 0 ), wxDefaultSpan, wxEXPAND );
   }

   if( mWhichMeters & kWithPlayMeter ){


Does mWhichMeters still need to be a bitwise or of non-exclusive choices?

There was old, old code for an alternative combined play & record meter toolbar in one, instead of the separate ones. I forget when that was removed, but it appears the program is still constructing this third, combined meter toolbar, and yet never showing it and giving you no user interface to turn it on!

Paul-Licameli · 2022-10-07T21:16:26Z

src/widgets/ASlider.cpp

@@ -530,6 +530,7 @@ LWSlider::LWSlider(wxWindow *parent,
      speed = 0.5;
      break;
   case FRAC_SLIDER:
+   case PERCENT_SLIDER:


The commit that adds PERCENT_SLIDER for the play and record meters is perhaps incomplete.

I can double-click on the slider thumb and get a dialog to enter a value. But it must be a fraction from 0 to 1, not a percentage value. Is that acceptable? A design question @Tantacrul

I also see that not all calls to SendUpdate() are followed by ShowTip() -- the dialog has some of those calls. Should ShowTip() be added?

At this commit, at least in macOS, I do not yet get the hover texts over these two slider thumbs. But I do get them for track pan and gain and the mixer board gain slider. Maybe a later commit will fix this. I haven't built them all yet.

Paul-Licameli · 2022-10-07T21:35:50Z

src/widgets/MeterPanel.cpp

@@ -346,11 +346,11 @@ MeterPanel::MeterPanel(AudacityProject *project,
      mSlider = std::make_unique<LWSlider>(this, XO(""),
         pos,
         size,
-         FRAC_SLIDER,
+         PERCENT_SLIDER,


OK, after another commit that changes this default, I do see tooltips, on macOS -- but only if I click the thumb, not when I just hover the mouse over it. This is different from the pan and gain sliders.

The text I see is like ": 47%" suggesting something was supposed to be formatted before the ":" but is left empty.

I think that is mName being empty at these lines of ASlider.cpp:

/* i18n-hint: An item name followed by a value, with appropriate separating punctuation */ label = XO("%s: %s").Format( mName, val );

With pan and gain sliders, I see a hover text, but when I press the mouse, I see the same text, but it moves and is drawn in a bordered, rounded rectangle. That's a little strange.

Paul-Licameli · 2022-10-07T21:42:34Z

src/toolbars/MeterToolBar.cpp

@@ -183,6 +183,7 @@ void MeterToolBar::Populate()
       This is the name used in screen reader software, where having 'Meter' first
       apparently is helpful to partially sighted people.  */
      mRecordMeter->SetLabel( XO("Meter-Record") );
+      mRecordMeter->SetSliderName(XO("Recording Level"));


Okay, at another commit, now I see the problems of previous commits corrected. Names before the :, and a nice hover text with a rounded border.

In fact, now the old behavior of the pan and gain sliders looks worse! There should be a minor issue I think to fix them to be consistent with the volume sliders. Which is not part of the scope of this PR. @LWinterberg @Tantacrul

It is still the case, however, that when double clicking the volume slider thumb to get a dialog, I'm still not entering a percentage value, but instead a fraction in the range 0.0 to 1.0.

Thanks, dialog now shows values in range 0..100

Paul-Licameli · 2022-10-07T22:00:14Z

src/toolbars/MeterToolBar.cpp

@@ -150,7 +151,7 @@ void MeterToolBar::Populate()
      //(maybe we should do it differently for Arabic language :-)  )
      mRecordSetupButton = safenew AButton(this);
      mRecordSetupButton->SetLabel({});
-      mRecordSetupButton->SetName(_("Record"));
+      mRecordSetupButton->SetName(wxString::Format(_("Record meter peak %d db"), -DecibelScaleCutoff.GetDefault()));


Why GetDefault() and not just Read()?

Now if I change interface preferences so the bottom of scale is not -60 dB, the drawing updates for it correctly, but the name spoken for the button still says -60, inconsistently with the display.

DecibelScaleCutoff.GetDefault() also occurs in the constructor of MeterPanel. I put it there at commit 01a53e7. As a behavior-preserving code transformation, it was correct as I did it, eliminating the use of the constant ENV_DB_RANGE. But was old behavior correct? Perhaps not, and that other line should also be corrected to use Read().

Thanks, fixed

Paul-Licameli · 2022-10-07T22:04:03Z

I'm done with this round of review.

DavidBailes · 2022-10-11T12:58:09Z

Some comments on the meter buttons. The names of these buttons read by screen readers does not get updated to reflect the updated peak, whether their is monitoring, and whether there has been clipping.
One way of fixing this would be as follows:

derive a class from Abutton, say called MeterButton, and use it instead of AButton in this toolbar.
In the MeterButton class override the function wxString GetName() const, and in this function return a string with the same contents as the string produced by MeterAx::GetName(). (MeterAx now represents a slider, so MeterAx::GetName() should be removed).

With respect to fully transferring the fix in commit 3d420e0, once the accessibility names of the buttons are correct, I can check whether anything further needs doing about this. There may need to be a minor modification to AButtonAx to give it a silent mode.

DavidBailes · 2022-10-11T14:39:10Z

My days! I am really struggling to figure out how to tab to the record volume slider (using MacOS) in order to change it and hear a screenreader 😦.

Testing with commit 700256 on MacOS

I can tab to the sliders
After pressing VO+Shift+Down arrow to interact with one, I can use the arrow keys to change the value of the slider
VoiceOver does not automatically read the new value of the slider when it is changed - don't think it's ever done this for sliders in Audacity.

When I hover over the 'Add effect' button, Voice Over (MacOS) calls it a 'Taste'. (!) Are we aware of this strangeness? I guess This should be raised as a separate issue. I was just curious if anyone had any thoughts.

Fortunately, I can't reproduce this.

DavidBailes · 2022-10-12T08:08:30Z

Some comments on the meter buttons. The names of these buttons read by screen readers does not get updated to reflect the updated peak, whether their is monitoring, and whether there has been clipping.
One way of fixing this would be as follows:

Another way of fixing it would be:

Derive a class from AButtonAx, say MeterButtonAx, and set this as the accessible for the AButton in this toolbar.
In the MeterButtonAx class override the function wxAccStatus AButtonAx::GetName, and in this function create a string with the same contents as the string created by MeterAx::GetName(). (MeterAx now represents a slider, so MeterAx::GetName() should be removed).

Again, there may need to be some minor fixes to fully transfer the fix in commit 3d420e0.

petersampsonaudacity · 2022-10-20T11:43:34Z

@dozzzzer @LWinterberg

Testing on W19 with @vsverchinsky Vitaly's branch: audacity-win-3.2.2-alpha-20221018+915aba6-x64

In this branch build:

a) the sliders on the Meter Toolbars now have the value of the current position

b) the tool-tip nicely updates when the slider is moved

@DavidBailes David, are these readable by screen readers? My tests with NVDA seems to indicate that this is OK, but I am no accessibility expert.

DavidBailes · 2022-10-21T10:31:28Z

Comments on the meter buttons in the latest commit:

if the meter is clipped, this is not read by a screen reader. In bool MeterPanel::IsClipping() const, it should be mBar[c].clipping, not mBar[c].isclipping, as in the old MeterAx::GetName().
when appropriate, the screen reader should read "monitoring" or "active", but not both, as in the old MeterAx::GetName().
the accessibility name does not include the current peak level, as in the old MeterAx::GetName(). In MeterToolBar::Populate(), the accessibility names of the buttons should just be set to "record meter" and "playback meter", as the peak level should be included in MeterButtonAx::GetName().
When the button is pressed by pressing Enter, there is an unwanted "beep" indicating an invalid action. See also Unwanted beep when either the add or replace effect button is pressed using the keyboard. #3484

petersampsonaudacity · 2022-10-21T14:03:30Z

@vsverchinsky and @crsib

since Vitaly's branch build audacity-win-3.2.2-alpha-20221018+915aba6-x64
https://github.com/audacity/audacity/actions/runs/3272357779

at least fixes: #3810 Hovertext tooltips for recording and playback sliders no longer show the values
and this is a very useful improvement for users,

is there any chance that we could pull this fix right now (for 3.2.2) while the further important accessibility issues that @DavidBailes raises can be dealt with separately - leaving this PR thread and #3230 still open - while closing off #3810.

Bearing in mind that #3810 is a recent regression issue introduced in 3.2.0 with the new combined Meter/Mixer toolbars. ?

DavidBailes · 2022-10-24T08:43:17Z

When the button is pressed by pressing Enter, there is an unwanted "beep" indicating an invalid action.

You may well be aware of this already, but there's a comment in void MeterPanel::OnKeyDown(wxKeyEvent &evt) about avoiding a beep.

vsverchinsky · 2022-10-26T11:59:46Z

the accessibility name does not include the current peak level, as in the old MeterAx::GetName(). In MeterToolBar::Populate(), the accessibility names of the buttons should just be set to "record meter" and "playback meter", as the peak level should be included in MeterButtonAx::GetName().

@DavidBailes Do I understand correctly that button name should include peak hold value and the slider should include peak value (DecibelScaleCutoff)?

DavidBailes · 2022-10-26T12:48:50Z

@DavidBailes Do I understand correctly that button name should include peak hold value and the slider should include peak value (DecibelScaleCutoff)?

The slider should NOT include the peak value.

The button name should include the peak hold value, as it did in the old MeterAx::GetName(). (And the order of phases that make up the name should remain the same.) Snippet of old code:

float peak = 0.;
bool clipped = false;
for (unsigned int i = 0; i < m->mNumBars; i++)
{
   peak = wxMax(peak, m->mBar[i].peakPeakHold);
   if (m->mBar[i].clipping)
      clipped = true;
}

if (m->mDB)
   *name += wxT(" ") + wxString::Format(_(" Peak %2.f dB"), (peak * m->mDBRange) - m->mDBRange);
else
   *name += wxT(" ") + wxString::Format(_(" Peak %.2f "), peak);

if (clipped)
   *name += wxT(" ") + _(" Clipped ");

}

DavidBailes

The reading by screen readers of the meter toolbar is now fine.
Glad to see that you got rid of the unwanted "beep", and that your fix also fixes the issue: #3484

DavidBailes · 2022-11-07T10:16:54Z

@Paul-Licameli . I know you're busy, but could you review this. I may want to submit a fix for another bug, and want to avoid conflicts when it's merged.

Paul-Licameli · 2022-11-07T11:28:49Z

@Paul-Licameli . I know you're busy, but could you review this. I may want to submit a fix for another bug, and want to avoid conflicts when it's merged.

Very nearly done

Paul-Licameli · 2022-11-07T12:30:23Z

src/toolbars/MeterToolBar.cpp

+            // been added to ensure at least one space, and stop
+            // words from being merged
+            if(panel->IsMonitoring())
+               *name += wxT(" ") + _(" Monitoring ");


If you add the space yourself, for the special reasons explained in the comment, then do not include the spaces in the strings to be translated.

This applies to five translatable strings in this function.

I see the minor fault was in copy-pasted code and not introduced here. So it's just a recommendation.

petersampsonaudacity · 2022-11-08T10:33:29Z

@crsib @vsverchinsky

So now this has been approved by @Paul-Licameli and @DavidBailes - how does this get pulled so it can be tested in a 2.3.3 alpha or beta?

vsverchinsky · 2022-11-08T12:08:52Z

@dozzzzer did yet complete testing it, right?

dozzzzer · 2022-11-08T13:22:55Z

@dozzzzer did yet complete testing it, right?

Not yet.

…lications

…event handler

petersampsonaudacity · 2022-11-09T11:29:27Z

Note that latest 3.3.0 alpha: audacity-win-3.3.0-alpha-20221109+fe50013-x64-msvc2022

does not yet have thus merged in.

Looks good on latest 3.2.2.beta: audacity-win-3.2.2-beta-20221109+f0bb347-x64-msvc2022

vsverchinsky requested review from Paul-Licameli and DavidBailes September 27, 2022 00:29

crsib linked an issue Sep 30, 2022 that may be closed by this pull request

Allow keyboard focus on the combined meter/mixer sliders #3230

Closed

Paul-Licameli reviewed Oct 5, 2022

View reviewed changes

vsverchinsky force-pushed the i3230-meter-toolbar-navigation branch 2 times, most recently from ed6550f to 521d500 Compare October 7, 2022 08:54

Paul-Licameli reviewed Oct 7, 2022

View reviewed changes

vsverchinsky mentioned this pull request Oct 14, 2022

Hovertext tooltips for recording and playback sliders no longer show the values #3810

Closed

vsverchinsky force-pushed the i3230-meter-toolbar-navigation branch 2 times, most recently from 8130718 to 37e7f12 Compare October 18, 2022 09:51

vsverchinsky changed the base branch from release-3.2.1 to release-3.2.2 October 18, 2022 09:52

vsverchinsky force-pushed the i3230-meter-toolbar-navigation branch from 37e7f12 to 93bf92d Compare October 27, 2022 12:28

vsverchinsky requested a review from Paul-Licameli October 27, 2022 13:34

DavidBailes approved these changes Oct 28, 2022

View reviewed changes

Paul-Licameli reviewed Nov 7, 2022

View reviewed changes

Paul-Licameli approved these changes Nov 7, 2022

View reviewed changes

vsverchinsky added 12 commits November 9, 2022 11:14

AButton drawing/sizeing changes

88532b7

Remove icon from MeterPanel

891b77e

MeterPanel new layout

e5e32a8

MeterPanel navigation and accessibility fixes

c4c1edc

ToolBar avoid self-focusing

b850c3b

ASlider '%' tips, show tip on keyboard value change

7b74b9e

Meters tip text change

a9c987a

Use LWSlider tip instead native tip in meters

1f1f10e

Reuse recolored buttons generation across toolbars/eleminate code dup…

a2f983c

…lications

Make AButtonAx public and extendable

912826a

Meter panel accessibility improvements

e48e503

Attempt to fix beep sound when wxMenu is invoked inside wxEVT_BUTTON …

8c07740

…event handler

vsverchinsky force-pushed the i3230-meter-toolbar-navigation branch from 93bf92d to 8c07740 Compare November 9, 2022 08:32

vsverchinsky merged commit f0bb347 into audacity:release-3.2.2 Nov 9, 2022

LWinterberg linked an issue Nov 9, 2022 that may be closed by this pull request

Hovertext tooltips for recording and playback sliders no longer show the values #3810

Closed

vsverchinsky deleted the i3230-meter-toolbar-navigation branch November 9, 2022 18:19

JohnColket mentioned this pull request Dec 14, 2022

Windows DirectSound Microphone Gain Control seems inoperative #4068

Closed

Meter toolbar navigation & accessibility #3696

Meter toolbar navigation & accessibility #3696

Conversation

vsverchinsky commented Sep 27, 2022 • edited Loading

DavidBailes commented Sep 27, 2022

DavidBailes commented Sep 28, 2022

Tantacrul commented Sep 30, 2022 • edited Loading

vsverchinsky commented Oct 4, 2022

Tantacrul commented Oct 4, 2022 • edited Loading

Choose a reason for hiding this comment

DavidBailes commented Oct 5, 2022

DavidBailes commented Oct 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Paul-Licameli Oct 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Paul-Licameli commented Oct 7, 2022

DavidBailes commented Oct 11, 2022

DavidBailes commented Oct 11, 2022

DavidBailes commented Oct 12, 2022

petersampsonaudacity commented Oct 20, 2022 • edited Loading

DavidBailes commented Oct 21, 2022 • edited Loading

petersampsonaudacity commented Oct 21, 2022 • edited Loading

DavidBailes commented Oct 24, 2022

vsverchinsky commented Oct 26, 2022

DavidBailes commented Oct 26, 2022

DavidBailes left a comment

Choose a reason for hiding this comment

DavidBailes commented Nov 7, 2022

Paul-Licameli commented Nov 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petersampsonaudacity commented Nov 8, 2022

vsverchinsky commented Nov 8, 2022

dozzzzer commented Nov 8, 2022

petersampsonaudacity commented Nov 9, 2022

vsverchinsky commented Sep 27, 2022 •

edited

Loading

Tantacrul commented Sep 30, 2022 •

edited

Loading

Tantacrul commented Oct 4, 2022 •

edited

Loading

Paul-Licameli Oct 7, 2022 •

edited

Loading

petersampsonaudacity commented Oct 20, 2022 •

edited

Loading

DavidBailes commented Oct 21, 2022 •

edited

Loading

petersampsonaudacity commented Oct 21, 2022 •

edited

Loading