Refactor NVDAHelper32remote Live region handler#9079
Refactor NVDAHelper32remote Live region handler#9079feerrenrut merged 27 commits intonvaccess:masterfrom
Conversation
|
Hi, I’d recommend asking a review from @michaelDCurran, as he is the expert on controller client. Thanks.
|
|
done. |
jcsteh
left a comment
There was a problem hiding this comment.
In terms of code, this looks fine to me. However, Joseph is correct: @michaelDCurran is the best person to review this.
While I understand the desire to have this as a groundwork PR, I'm cautious of changes which introduce no user visible benefit while still introducing regression risk. I guess this is less important now that we no longer have the incubating stage, since we don't have to wait weeks before we can land dependent changes. If this were a concern, a tiny feature you could introduce here would be disabling of live region reporting if report dynamic content changes (NVDA+5) is disabled. However, I'll let @michaelDCurran decide whether he wants that done here or not.
LeonarddeR
left a comment
There was a problem hiding this comment.
I agree with @jcsteh, it might help to have live regions reporting disabled with dynamic content changes.
|
While (strictly speaking) RPC interfaces should get a version bump when
changed, I don't think we ever have for internal interfaces, since there
are no consumers other than the same copy of NVDA.
|
michaelDCurran
left a comment
There was a problem hiding this comment.
So far looks good. But:
- nvdaHelper/vbufBackends/mshtml/node.cpp, line 393 still calls nvdaController_speakText.
Ooh, didn't know about this! I'll handle that one. I didn't even check this in IE :-) |
f7c4f0f to
3e990f3
Compare
|
This change would be very beneficial. Thank you @LeonarddeR for pointing this out. Could you please consider a mean to identify the node triggering the speech so that we could programatically disable Live announces for a specific node? |
|
@derekriemer Are you intending to give this a follow up? |
|
I have no idea how to fix the crash in my IE code. I do intend to, but it
may be years at this rate.
…On Tue, Oct 1, 2019 at 4:32 AM Leonard de Ruijter ***@***.***> wrote:
@derekriemer <https://github.com/derekriemer> Are you intending to give
this a follow up?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9079?email_source=notifications&email_token=ABI2FPPU665JMT7TXGMZVMDQMMRLFA5CNFSM4GKUGB72YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAAZRSI#issuecomment-536975561>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABI2FPJJYBFSRB2B34KD4BTQMMRLFANCNFSM4GKUGB7Q>
.
--
Derek Riemer: Improving the world one byte at a time!
- University of Colorado Boulder Department of computer science, 4th
year undergraduate student.
- Accessibility enthusiast.
- Proud user of the NVDA screen reader.
- Open source enthusiast.
- Skier.
Personal website <http://derekriemer.com>
|
@derekriemer, maybe could I try to be of some help if you like. |
|
Sure, let me push to this branch, and you can try running my new code in
NVDA in IE. You'll need to attach windbg to the processId of IE
(nav.processID in the python console after loading IE. After the tab
crashes (load any page) you'll need to figure out how to get windbg to
print out what the problem is.
…On Thu, Oct 3, 2019 at 8:49 AM Julien Cochuyt ***@***.***> wrote:
I have no idea how to fix the crash in my IE code. I do intend to, but it
may be years at this rate.
@derekriemer <https://github.com/derekriemer>, maybe could I try to be of
some help if you like.
This PR might bring a long awaited feature for us.
Could you please state how to reproduce the incident you're facing?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9079?email_source=notifications&email_token=ABI2FPMG47RQ5LERWY3WJRTQMYA7JA5CNFSM4GKUGB72YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAIOS3I#issuecomment-537979245>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABI2FPIACRL4CT6B5XRAMCDQMYA7JANCNFSM4GKUGB7Q>
.
--
Derek Riemer: Improving the world one byte at a time!
- University of Colorado Boulder Department of computer science, 4th
year undergraduate student.
- Accessibility enthusiast.
- Proud user of the NVDA screen reader.
- Open source enthusiast.
- Skier.
Personal website <http://derekriemer.com>
|
*currently takes a text and level string.
This comment has been minimized.
This comment has been minimized.
|
It seems the new aria grid system test is failing regularly. We'll take a look at it soon. For now I'll just kick off another PR build. |
This comment has been minimized.
This comment has been minimized.
|
Actually it looks like all builds are failing because of this test. We'll get to it. |
This comment has been minimized.
This comment has been minimized.
See test results for failed build of commit cb7b9b21b7 |
|
@feerrenrut were there any further things that needed to be addressed here? I merged master and fixed the remaining linting error. So with the system tests fixed, this now builds fine. |
|
Will this PR merge to NVDA 2020.4? |
|
Given the change in 50de939 I want to be sure that this was tested again after fixing that assert. In the context of the discussion in #9079 (comment), essentially to be sure that we aren't inadvertently saving off an object referring to a temporary (C++) variable. I think this is unlikely, but I'd like to be sure. |
|
If that is fine, I'm happy for this to be merged. |
|
Yes, this was tested afterwards.
|
feerrenrut
left a comment
There was a problem hiding this comment.
Thanks @LeonarddeR I'd forgotten this was ready to go.
This comment has been minimized.
This comment has been minimized.
|
It seems disabling reporting of dynamic content change might miss some cases: #12193 (comment) |
Link to issue number:
fixes #9077,
Unblocks #7756,
closes #7743
Summary of the issue:
This change separates the logic used for sending speech messages to core from the logic needed to send live region messages to core.
Description of how this pull request fixes the issue:
NVDAControllerInternal_speakMessage used to be used to send live regions at NVDA, which made it impossible to differentiate them from messages sent from external applications through the controller client. This change separates the passage of live regions into another function, nvdaControllerInternal_reportLiveRegion(wchar_t* text, wchar_t* level); which is part of the NVDAControllerInternal interface instead of the NVDAController interface. This is because I don't see a reason for non NVDA specific DLL's to call into this. This function is then used in place of speakTextt in ia2LiveRegions.cpp.
Testing performed:
Honestly, I have no idea how NVDA gets the message from the remote DLL to the local DLL. I performed manual testing to make sure this works, but there's magic going on I don't understand, so review with caution.
Known issues with pull request:
None
Change log entry: