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

Speak typed words based on TextInfo if possible #8110

Closed
wants to merge 51 commits into from
Closed

Speak typed words based on TextInfo if possible #8110

wants to merge 51 commits into from

Conversation

LeonarddeR
Copy link
Collaborator

@LeonarddeR LeonarddeR commented Mar 22, 2018

Link to issue number:

Closes #8065
Fixes #7812
Fixes #6215

Summary of the issue:

When speaking typed words, NVDA always relies on a buffer to speak the typed word. This causes the following problems.

  • When selecting text and then typing a word, NVDA doesn't speak the first letter of the typed word. This is an issue reported by a @BabbageCom customer.
  • When editing a word, NVDA does not speak the word as how it has been edited, but only announces the characters that have been added to the word.

Description of how this pull request fixes the issue:

NVDA now tries to
use the TextInfo information to announce the last entered word. This works as follows:

  1. speech.speakTypedCharacters no longer speaks typed words, but devotes this to a new function, speech.speakPreviousWord that takes the word separator as an argument.

  2. speech.speakPreviousWord creates a TextInfo for the caret and calls textInfo.findWordBeforeCaret. This new function manipulates the textInfo and:

    • Returns True if the TextInfo can be used to announce the previous word
    • Returns False if the caret is currently part of a word, unless the last word separator was a space. This is the logic that provides the fix for NVDA Breaks Words At Apostrophes with Speak Typed Words #6215. No word echo will be performed.
    • Raises a LookupError if no suitable word can be found to speak, in which case speech.speakPreviousWord falls back to the buffer based word echo.

    speech.speakPreviousWord also deals with reporting of spelling errors as you type.

  3. NVDAObject._reportErrorInPreviousWord has been removed, logic from this function has been spread out over the two functions explained above.

  4. This disables uniscribe based word offset calculations by default and enables uniscribe specifically for edit text controls. This is because the idea of uniscribe about what characters separate words is totally different from what most other applications do (i.e. dot, comma are not considered word separators). Note that this will result in slightly different moving by word behaviour in virtual buffers.

Open to discussion

  1. In this pr, I disabled word echo for non editable cases (i.e. where there is no caret). If desired, that can be reverted, but I'd say speaking typed words doesn't make sense if there's nowhere to be typed. An exception could be typing in lists in order to quickly navigate to list items (e.g. in Explorer), but I'd also say that word echo in these cases works delaying.
  2. I added a caret snapshot variable for personal debugging purposes, but also thought I'd be handy to keep it.

Testing performed:

Tested several applications, including:

  • Notepad
  • Wordpad
  • Microsoft Word, both with and without UIA
  • Mozilla Firefox
  • Mozilla Thunderbird
  • Google Chrome
  • Libre office

Known issues with pull request:

  1. It turns out that firefox word offset calculation (i.e. based on IAccessibleTextObject.TextAtOffset) is broken. This is a known bug that also affected spelling error as you type announcements, though in that case this problem was much less prevalent. It looks like Thunderbird and chrome do not have this issue or in a less prevalent way, but I didn't test these that extensively. As of the current implementation of this pr, we shouldn't be affected by this, though it needs testing.
  2. From an UX perspective, word announcements behavior will differ from what people are accustomed to in Notepad and Firefox, as some word separators are considered part of a word. This is most prevalent when pressing dot or comma, which won't give a typed word announcement.
  3. In LibreOffice, IAccessibleTextObject.TextAtOffset seems to use uniscribe for word boundaries whereas LibreOffice itself does not. Therefore, this pr overrides _getWordOffsets for SymphonyTextInfo to use word offsets calculation based on OffsetsTextInfo. This is much more reliable, except for words containing comma's (i.e. 1,3, 4,5, etc.)

Change log entry:

@LeonarddeR LeonarddeR changed the title I8065 Speak typed words based on TextInfo if possible Mar 22, 2018
@ehollig
Copy link
Collaborator

ehollig commented Mar 22, 2018

How would this work with Chinese and Japanese or with languages that do not have spaces?

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Mar 23, 2018 via email

@jiangtiandao
Copy link

jiangtiandao commented Mar 25, 2018

I cloned the code and tried.
It seems that reading on Chinese input method works as usual.

@dnz3d4c
Copy link

dnz3d4c commented Mar 26, 2018

Could you provide a test build for Korean users?

@LeonarddeR
Copy link
Collaborator Author

@dnz3d4c commented on 26 mrt. 2018 02:50 CEST:

Could you provide a test build for Korean users?

Sure. Here is a try build.

@dnz3d4c
Copy link

dnz3d4c commented Mar 26, 2018

Thanks @LeonarddeR

@michaelDCurran
Copy link
Member

It is likely that this may regress #456. Although it was not totally clear from the reporter, moving to uniscribe for virtualBuffers fixed Thai word segmentation issues.
What would be the impact to this pr if we kept using uniscribe for virtualBuffers? Was this change necessary to implement speak typed words properly, or was it just a nice improvement that fitted well with it?

@michaelDCurran
Copy link
Member

In what real-world cases will this code fall back to the old character buffer implementation?

@LeonarddeR
Copy link
Collaborator Author

@michaelDCurran commented on 15 Apr 2018, 23:57 CEST:

What would be the impact to this pr if we kept using uniscribe for virtualBuffers?

That would have no impact at all.

Was this change necessary to implement speak typed words properly, or was it just a nice improvement that fitted well with it?

The latter. I think we should just keep using uniscribe for now.

@LeonarddeR
Copy link
Collaborator Author

@michaelDCurran commented on 16 Apr 2018, 00:22 CEST:

In what real-world cases will this code fall back to the old character buffer implementation?

I could think of several of these cases, I tried to document them in the code.

  1. When trying to look up a word before the caret, but there is no word before the caret. I believe this applies to cases where you press enter in the python console for example, in which case the last word you entered isn't part of the TextInfo as soon as NVDA tries to get the last typed word. This also applies to Miranda NG
  2. Sometimes, the IA2Text implementation for Firefox seems to lag behind, in which case text info based word echo would be unreliable. I need to recheck the symptoms that happen when we would use TextInfo based word echo in these cases.
  3. When using an editor that uses auto indentation, that editor adds several spaces or tabs at the start of a new line when pressing enter. That makes TextInfo based echo unreliable.

@michaelDCurran
Copy link
Member

michaelDCurran commented Apr 16, 2018 via email

@LeonarddeR
Copy link
Collaborator Author

@michaelDCurran commented on 16 apr. 2018 23:39 CEST:

Re Firefox updating fast enough, I wonder if we can delay our code a
bit. Either re-queue it one more time, or perhaps even do something
similar to hasCaretMoved, but perhaps for only 50ms or so.

I'm not 100% sure whether it is because Firefox lags behind. It really feels like it, though. However, using a short delay might make this somewhat more clear.

For auto indenting, we could specifically handle this in some kind of
script for kb:enter perhaps? Then we'd no to check the line above if the
current line started with whitespace.

Interesting idea, I'm going to try this.

@LeonarddeR
Copy link
Collaborator Author

@michaelDCurran commented on 16 apr. 2018 23:39 CEST:

For auto indenting, we could specifically handle this in some kind of
script for kb:enter perhaps?

This is now covered. I reverted the firefox hack while at it, since it didn't play nice with reporting the typed character before sending enter to the system. I will have to look into the firefox issue again.

@LeonarddeR
Copy link
Collaborator Author

@michaelDCurran commented on 16 apr. 2018 23:39 CEST:

Re Firefox updating fast enough, I wonder if we can delay our code a
bit. Either re-queue it one more time, or perhaps even do something
similar to hasCaretMoved, but perhaps for only 50ms or so.

Queuing one additional time doesn't seem to be enough, so there are now three attempts with 5 ms inbetween.

michaelDCurran
michaelDCurran previously approved these changes May 6, 2018
@michaelDCurran
Copy link
Member

Currently a unit test fails: FAIL: test_onlySpaces (tests.unit.test_textInfos.TestFindWordBeforeCaret_exceptions)

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented May 7, 2018 via email

@LeonarddeR
Copy link
Collaborator Author

The more and more I test, the more I'm tempted to disable this for IA2Web objects altogether. I'm experiencing several issues, including the following:

  1. Create a new issue on Github for NVDA.
  2. Start typing above one of the headings (i.e. create a blank line above a ## line).

IN quite a few occurrences, when pressing space after typing a word, NVDA announces the number sign that starts the new line as part of the typed word.

@LeonarddeR LeonarddeR added the component/text-info TextInfo objects and text review label May 18, 2018
@LeonarddeR
Copy link
Collaborator Author

I realised that a disadvantage of the new approach is that it is more difficult to test, as it now relies on actual caret movement to cache the position before the caret moves. Having said that, the current approach is much less error prone.

I will have to look into this.

@LeonarddeR
Copy link
Collaborator Author

The current implementation is a bit sluggish in UIA consoles, probably because of the _caretMovementTimeoutMultiplier.

@AppVeyorBot
Copy link

PR introduces Flake8 errors 😲

See test results for Failed build of commit 365e8a1202

@dpy013
Copy link
Contributor

dpy013 commented Nov 26, 2019

hi @LeonarddeR
Will this pr be merged into 2019.3?
thanks

@LeonarddeR
Copy link
Collaborator Author

I wrote this pull request on behalf of @BabbageCom. As I'm leaving @BabbageCom after the 29th of November, I can no longer afford maintaining this pr other than applying very basic review actions. If this pull request requires major changes, they will have to be applied by someone else, e.g. @sjfbol or whoever else is willing to take it.

hi @LeonarddeR
Will this pr be merged into 2019.3?
thanks

Nope.

@michaelDCurran
Copy link
Member

Although there are definite advantages of this approach, there are several limitations and bugs noted in the pr description. Also @LeonarddeR has suggested he will have no time to work on it in future. Therefore we are closing this. However, if anyone wants to pick up this work they are welcome. At very least learning from how some of this code was done.

@Adriani90 Adriani90 added the Abandoned requested reports or updates are missing since more than 1 year, author or users are not available. label Sep 6, 2023
@Adriani90
Copy link
Collaborator

Labeled this as abandoned so someone can find it easier by filtering the label.

@Adriani90
Copy link
Collaborator

cc: @mltony, @cary-rowen this might be interesting in light of word navigation and word pronouncing, maybe #16219 have opened up some new possibilities.
Note this seems abandoned code as per comment above, so someone else could take it up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Abandoned requested reports or updates are missing since more than 1 year, author or users are not available. BabbageWork Pull requests filed on behalf of Babbage B.V. component/text-info TextInfo objects and text review
Projects
None yet
9 participants