Fixing incorrect line reporting in Scintilla controls when line wrapping is on.#9427
Conversation
LeonarddeR
left a comment
There was a problem hiding this comment.
Thanks for testing this thoroughly, also with several DPI levels. I will push a try build for people to test.
LeonarddeR
left a comment
There was a problem hiding this comment.
Actually, after reading the code again, there is something that seems odd to me.
-
We're fetching the location of the control, which is in screen coordinates.
-
Then, when calling self._getOffsetFromPoint, you are adding the result of SCI_POINTYFROMPOSITION to the top of the window. However, according to the Scintilla docs:
These messages return the x and y display pixel location of text at position pos in the document.
-
I also noticed that _getOffsetFromPoint throws client coordinates at scintilla, wwhereas _getPointFromOffset doesn't do anything with client to screen conversion. Could you look into this while at it?
|
@LeonarddeR Regarding your second point: it also sounded strange to me at first, but I tried several combinations and only this one was giving the right results. I claim that it's pixel perfect! Finally, |
|
_getPointFromOffset gets called when you use the move mouse to navigator object command. My testing seemed to reveal some issues with this..
|
|
Hi again @LeonarddeR, So... Anything else? |
…nge objects between NvDA and nvdaHelper inproc code, rather than letting Windows do it, as the previous implementation was failing on Office 2007 because CoInitialize was never called on the RPC worker thread.
| return watchdog.cancellableSendMessage(self.obj.windowHandle,SCI_POSITIONFROMPOINT,x,y) | ||
|
|
||
| def _getPointFromOffset(self,offset): | ||
| location = self.obj.location |
There was a problem hiding this comment.
You're basically converting client to screen coordinates here, doing it manually. Is there a specific reason why you aren't using clientToScreen here? Does it fail?
There was a problem hiding this comment.
Hi Leonard,
You are right! I just forgot about it. This way makes it more explicit.
| @@ -193,9 +194,9 @@ def _getLineOffsets(self,offset): | |||
| # Lines in Scintilla refer to document lines, not wrapped lines. | |||
| # There's no way to retrieve wrapped lines, so use screen coordinates. | |||
| y=watchdog.cancellableSendMessage(self.obj.windowHandle,SCI_POINTYFROMPOSITION,None,offset) | |||
There was a problem hiding this comment.
You now can retrieve y with _getPointFromOffset. That gives you an x you won't use, but it at least ensures that you're following an uniform codepath to get screen coordinates from scintilla offsets.
| start = self._getOffsetFromPoint(0,y) | ||
| end=self._getOffsetFromPoint(width,y) | ||
| location=self.obj.location | ||
| start = self._getOffsetFromPoint(location.left, location.top+y) |
There was a problem hiding this comment.
If you use _getPointFromOffset above, this will be:
| start = self._getOffsetFromPoint(location.left, location.top+y) | |
| start = self._getOffsetFromPoint(location.left, y) |
| end=self._getOffsetFromPoint(width,y) | ||
| location=self.obj.location | ||
| start = self._getOffsetFromPoint(location.left, location.top+y) | ||
| end=self._getOffsetFromPoint(location.right, location.top+y) |
There was a problem hiding this comment.
When using _getPointFromOffset above, this becomes:
| end=self._getOffsetFromPoint(location.right, location.top+y) | |
| end=self._getOffsetFromPoint(location.right, y) |
There was a problem hiding this comment.
Hi Leonard,
Yes it makes sense. I initially didn't write it like this because I thought a useless call to watchdog.cancellableSendMessage to get the X coordinate wouldn't be accepted. But I don't really know how expensive is that type of call.
|
While the current approach involves more system calls, I think it is much less error prone. I'll have another look at your code later this week and then it's ready for Mick to get looked at.
|
|
hi |
|
I have approved it, but approval from a NV Access member is required before this can be merged. |
|
Hi, |
Link to issue number:
Fixes #9424.
Summary of the issue:
When navigating inside a Scintilla control with line wrapping enabled, NVDA fails to report the correct line (either when navigating with the keyboard or the mouse).
Description of how this pull request fixes the issue:
In
_getLineOffsets:Firstly
self.obj.locationwere not properly deconstructed (top and left were swapped),_getOffsetFromPointwere called withx=0to retrieve the start point of the line, ignoring the real x coordinate of the Scintilla control,finally,
_getOffsetFromPointwere called withx=widthto retrieve the endpoint of the line, again ignoring the margin on the left of the control.Testing performed:
Navigated through a document in Notepad++ with line wrapping enabled. Made sure that it worked with multiple DPI factors, on short lines as well as wrapped ones.
Known issues with pull request:
None
Change log entry:
None