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

Move the message 'selected' after the text which is selected. #9028

Merged
merged 11 commits into from Jun 28, 2019

Conversation

@lukaszgo1
Copy link
Contributor

commented Dec 7, 2018

Link to issue number:

None

Summary of the issue:

When selecting text for instance in the run box, or when asking NVDA what is selected with NVDA+Shift + up arrow the first info presented was the word 'selected'' itself rather than the text which is selected. Because both selecting and reporting current selection are done to know what is selected it makes sense to present the word 'selected' at the end.

Description of how this pull request fixes the issue:

A new function speakSelectedText was added to speech.py. It is called each time when selection is announced, and word selected is placed at the end of the message.

Testing performed:

Tested in notepad, run box, kindle, Microsoft Word and Notepad++

Known issues with pull request:

None

Change log entry:

Section: Changes

NVDA will now report the word 'selected' after saying what is selected

@Adriani90

This comment has been minimized.

Copy link
Collaborator

commented Dec 7, 2018

However, I think in cases when the shift key is not working properly, reporting selected at the beginning is at least a confirmation that everything worked properly. Otherwise if shift key is blocked or something like that then you get the info after the text. It is a smal Detail but though.

@Adriani90

This comment has been minimized.

Copy link
Collaborator

commented Dec 7, 2018

sorry I mean if the shift key is working properly, you get the info at the end of the selection reporting. So you have to wait until the text is reported to be sure that you have selected the text.

@Adriani90

This comment has been minimized.

Copy link
Collaborator

commented Dec 7, 2018

but yes, testing now in many applications it seems that if you select text with shift and arrow keys, NVDA reports "selected" after the selection. If you press nvda+shift+s (Laptop layout), NVDA reports "selected" first and then the selection. But if I understand this PR correctly you have moved the word "selected" at the end for nvda+shift+up arrow or nvda+shift+s command. Right?

@lukaszgo1

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2018

@Adriani90 wrote

but yes, testing now in many applications it seems that if you select text with shift and arrow keys, NVDA reports "selected" after the selection. If you press nvda+shift+s (Laptop layout), NVDA reports "selected" first and then the selection. But if I understand this PR correctly you have moved the word "selected" at the end for nvda+shift+up arrow or nvda+shift+s command. Right?

Not only that, but also for the situations when you are selecting text. Regarding the shift key what you have written is of course true, but I believe, that we should create optimal experience for the most typical situations, and in the typical scenarios Shift is working.

lukaszgo1 added some commits Dec 7, 2018

@Brian1Gaff

This comment has been minimized.

Copy link

commented Dec 8, 2018

@leonardder

This comment has been minimized.

Copy link
Collaborator

commented Dec 11, 2018

See also #1707 and #3508.

It might be that this one has been forgotten in fixing these issues.

@lukaszgo1

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2018

@leonardder wrote:

See also #1707 and #3508.

It might be that this one has been forgotten in fixing these issues.

It looks like it. With this we ad least reporting text before selection consistently.

@lukaszgo1

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

Any chance for a review of this? This pr is really simple + it is very annoying to hear selected at the beginning.

@leonardder
Copy link
Collaborator

left a comment

There is one case where selection is announced using speech.speakMessage. Could you find out why this is not using speech.speakSelectionmessage? I think there's nothing holding us back from changing that.

@lukaszgo1

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

It looks like that it was done to be consistent with the announcement in case of no selection. I've changed it and testing shows that there is no problems with it.

@leonardder leonardder requested a review from feerrenrut May 21, 2019

@feerrenrut
Copy link
Contributor

left a comment

Generally happy with this change, though given this is used in the same way (almost) entirely though the code base. It would be nice to remove this repetition of the "selected" string.

Perhaps something like:

speech.speakSelected(info.text)

Then we can remove repeated translator comments, and other lines missing translator comments. It also means that any future changes to this UX are more centralised. If you go down this path, don't delete the speakSelectionMessage function, but implement the new function by calling it.

There are also two calls that should continue to use "speakSelectionMesage" one says "unselected" the other "selected instead" I think these can stay as they are.

lukaszgo1 added some commits May 30, 2019

Implement new function speakSelectedText which is responsible for ann…
…ouncing selection. This will make future improvements to announcing selection easier.
@lukaszgo1

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

@feerrenrut Done. I've also updated description accordingly.

@lukaszgo1

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

@feerrenrut This is ready for another review.

@feerrenrut
Copy link
Contributor

left a comment

I'm happy with this, thanks for the contribution!

@feerrenrut feerrenrut merged commit 5491333 into nvaccess:master Jun 28, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Jun 28, 2019

feerrenrut added a commit that referenced this pull request Jun 28, 2019

@lukaszgo1 lukaszgo1 deleted the lukaszgo1:selectedAfterSelection branch Jun 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.