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

Restore status text and screen reader output #6210

Merged
merged 3 commits into from
Jun 20, 2020

Conversation

shoogle
Copy link
Contributor

@shoogle shoogle commented Jun 12, 2020

Resolves:

Fixes bugs and regressions from PR #6173.

  • I signed CLA
  • I made sure the code in the PR follows the coding rules
  • I made sure the code compiles on my machine
  • I made sure there are no unnecessary changes in the code
  • I made sure the title of the PR reflects the core meaning of the issue you are solving
  • I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
  • I made sure the commit message title starts with "fix #424242:" if there is a related issue
  • [N/A] I created the test (mtest, vtest, script test) to verify the changes I made

Comment on lines -355 to +365
if (w->score()->accessibleMessage().isEmpty())
currentInfoChanged();
currentInfoChanged();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The accessibleMessage string is cleared in the function AccessibleScoreView::text(), but that function only gets called if there is a screen reader running. If there is no screen reader then accessibleMessage would never be emptied after it was first set, hence currentInfoChanged() was never called to set the status bar text.

@MarcSabatella
Copy link
Contributor

I can confirm it fixed this issue on Linux; can check on Windows with NVDA and JAWS on Friday to be sure I don’t see any other effects.

@shoogle
Copy link
Contributor Author

shoogle commented Jun 12, 2020

@MarcSabatella, the thing to listen for is whether your accessibleInfo string is over-optimized, because now there will be occasions when currentInfoChanged() is called but the calculated accessibleInfo string will not get used. Next time currentInfoChanged() is called the string will get optimized again, possibly removing bar and beat numbers that should have been spoken.

@MarcSabatella
Copy link
Contributor

Understood. Most likely I won't be able to spend much time on this today. It was all kind of a delicate balance before that mostly just worked by accidental of how NVDA happened to respond, but it would be good to figure out what's going on with JAWS too. I should be able to find time for that next week.

BTW, so far making the timer thing not active on Linux seems an improvement overall, but I need to spend more time with that too.

@shoogle shoogle marked this pull request as draft June 12, 2020 21:38
@shoogle shoogle changed the title fix #306633: Always update status bar text Restore status text and screen reader output Jun 15, 2020
@shoogle shoogle marked this pull request as ready for review June 15, 2020 16:49
return msg;
if (info.isEmpty())
return msg;
return tr("%1, %2").arg(msg).arg(info);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you summarize when you expect this to hold? Is it to read the mode text and note info?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly. Typical values are:

msg  = "Steptime note input mode";
info = "Note C4 (quarter), beat 3, measure 1, staff 2";

msg could also relate to an action, such as "Staccato above added", but info always describes the current selection.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking through the code paths, it does seem these would always be distinct - I had worried at first they might be the same. Now the only thing I'd be concerned about is if there is ever a time when navigating through text (so msg is the character to read) that info would not have already been cleared and would still have the full text. Not that I know of a case offhand, but that's what I'll still want to test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MarcSabatella, you were right to be concerned, hence I have explicitly emptied the info string during text editing. See below.

@anatoly-os
Copy link
Contributor

LGTM. @MarcSabatella do you confirm?

@MarcSabatella
Copy link
Contributor

It does, although I haven't yet tested the lastest change to concatenate message & info. At least I don't think that was in when I did my last test. Will do so today, and do another round of checks on JAWS and Orca.

@MarcSabatella
Copy link
Contributor

Orca definitely does read the full text on each cursor movement, but it did that for the beta too, so this PR isn't a regression in that sense. And things are smoother overall than the beta. But maybe we should clear accessibleInfo when editing text?

Unfortunately, for reasons I don't yet understand, Orca is much slower to read than it was when I initially got it working just before FOSDEM. But that doesn't seem related to these changes or to the previous changes made for beta - the alpha was slow in the same way. Could be a result of other changes on my system, so I wouldn't worry about it. Certainly nothing this PR is causing.

Will check Windows next.

@MarcSabatella
Copy link
Contributor

Story is the same for NVDA and JAWS - they reads the full text of a text on each cursor movement (after reading the character at cursor).

BTW, some really good news here: JAWS is now reading the new messages for note input mode, adding/removing articulations, and navigation within text. So to me, just clearing the info at the right point in text edit would be great.

@shoogle shoogle marked this pull request as draft June 16, 2020 19:47
Fixes regression caused by commit 3960396 in PR musescore#6173.

The accessibleMessage string is cleared in the function
AccessibleScoreView::text(), but that function only gets called if
there is a screen reader running. If there is no screen reader then
accessibleMessage would never be emptied after it was first set, hence
currentInfoChanged() was never called to set the status bar text.
Changes:

- Display mode text for "Normal mode".

- Avoid screen reader output on entering playback mode so that the
  screen reader does not talk over the music.

Also fixes an accessibility bug in commit 3960396 in PR musescore#6173 that
caused the screen reader to say "note input mode" periodically rather
than just once upon entering that mode. This interfered with the usual
screen reader output for that mode, as reported in #306726.
…ifacts

Previously the screen reader would say either 'accessibleMessage' or
'accessibleInfo', with priority given to 'accessibleMessage'. Now it
will say both strings if they are both are non-empty.
@MarcSabatella
Copy link
Contributor

Works great for my with Orca and NVDA.

With JAWS, reading of characters as I cursor through the text is erratic for some reason. It wasn't working at all at first even with your changes, then it seemed it was for a while, now it is reading about half the time, and I can't see a pattern to it. I wouldn't worry about it though, to me this is good to merge.

@shoogle shoogle marked this pull request as ready for review June 17, 2020 21:36
@shoogle
Copy link
Contributor Author

shoogle commented Jun 17, 2020

@MarcSabatella, thanks for the review. @anatoly-os, should be good to go!

@anatoly-os anatoly-os merged commit 92164b1 into musescore:3.x Jun 20, 2020
@shoogle shoogle deleted the status-text branch June 22, 2020 11:29
@@ -4389,7 +4390,7 @@ void MuseScore::changeState(ScoreState val)
showPianoKeyboard(false);
break;
case STATE_NORMAL:
_modeText->hide();
showModeText(tr("Normal mode"));;
Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz Jun 29, 2020

Choose a reason for hiding this comment

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

This text doesn't show on Transifex, due to string freeze I guess

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants