Improved text location information for UIA, Word and compound text infos #8572
Conversation
…Word, UIA, offsets and Mozilla compound documents
LeonarddeR
added
Compound documents
blocked
component/text-info
|
Hi, I'd like to recuse myself from reviewing this, as I'm not quite that versed in text infos and rectangles yet. Thanks. |
|
No problem. Would you be able to test the possible Windows 7 issue that
might apply?
|
|
Hi, I need a test case (hopefully the issue number) so I can test it on a VM. Thanks.
|
|
I'm afraid there's neither an issue nor a valid test case for this, as this is only based on a comment in the code:
Internet Explorer 11 text inffo ffor textarea nodes is based on UIA, so in Windows 7, that'd be te way to test this. |
|
Hi, I see. I guess what we need is steps to reproduce possible issues in Windows 7+IE11+specific website+specific content. By the way, is the latest master branch merged? Thanks. |
|
Hi, @michaelDCurran, do you happen to have the URL for a website where we use UIA for textarea node instead of MSHTML? If so, let us know so we can try the following: Requiremenets:
Steps:
import NVDAObjects.UIA Where "randomX" and "randomY" can be any coordinate. Ideally you should get a text info in return, but on my computer (tested with Edge on Windows 10 build 17713) with random coords set to (400, 400), I get: Traceback (most recent call last): Thanks. |
|
Hi, Regarding merging master commits: I see that it was rebased on master branch, so ignore my comment. Thanks. |
|
@josephsl commented on 30 Jul 2018, 20:04 CEST:
This should be NVDAObjects.IAccessible.MSHTML.UIAMSHTMLTextInfo
Wow, let me look into that one, this is certainly not what I expect to happen. Edit: Actually, you're probably testing this on an object without a text pattern, in which case UIATextInfo is not supported. |
|
This might be an easier test case.
If all goes well, NVDA should not freeze and mouse tracking should pronounce the character where the caret is located. |
|
Hi, Yes, no longer freezes in Windows 7+IE11. We still want to make sure older versions such as 9 and 10 are still covered. Thanks. |
|
According to Mick, this uia implementation is only used as part of IE11., if I understood him correctly.
… Op 30 jul. 2018 om 20:59 heeft Joseph Lee ***@***.***> het volgende geschreven:
Hi,
Yes, no longer freezes in Windows 7+IE11. We still want to make sure older versions such as 9 and 10 are still covered.
Thanks.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Actually, possibly internet Explorer 10 as well. I'm not sure if it was
introduced as early as 9 though.
However, Windows 7 sp1 can run Internet Explorer 11 I think, so I'm not
*too* worried.
|
|
Hi, in this case, I’d like to suggest that we publish some sort of documentation telling folks that, for those using IE, versio n11 is recommended (once this PR comes to master branch). Thanks.
|
| # Can't move forward any further. | ||
| break | ||
| if obj == self._endObj: | ||
| ti = self._end |
There was a problem hiding this comment.
why does ti need reassigned here?
There was a problem hiding this comment.
Because that ti corresponds with the end of the range and its endOffset is the end of the current range. The earlier ti that is assigned by the call of _findNextContent is a textInfo for the object, but it might contain text that is out of the current range. I will add a comment to clarify this.
| #p=POINT(position.x,position.y) | ||
| #self._rangeObj=self.obj.UIATextPattern.RangeFromPoint(p) | ||
| raise NotImplementedError | ||
| #rangeFromPoint used to cause a freeze in UIA client library! |
There was a problem hiding this comment.
Is this comment relevant to the new code?
There was a problem hiding this comment.
The code is not new, it is no longer commented out. I'm still not convinced that this bug in the UIA client library is fixed for all cases that we could encounter.
There was a problem hiding this comment.
If we think that the freeze could still occur, then is it really safe to re-introduce this code?
There was a problem hiding this comment.
Some tests are performed on Windows 7 to see whether these freezes still occur, and they haven't been found reproducible. If we don't reintroduce this, that will instantly break routing the mouse to text positions in Edge again, for example.
We could limit this to a particular version of UIA if desired. Unfortunately there is no reference to a specific list of steps to reproduce this.
source/NVDAObjects/UIA/__init__.py
Outdated
| """ | ||
| Fetches per line bounding rectangles from the given UI Automation text range. | ||
| Note that if the range object doesn't cover a whole line (e.g. a character), | ||
| the bounding rectangle will be restriked to the range. |
There was a problem hiding this comment.
restriked -> restricted
| res=watchdog.cancellableSendMessage(self.obj.windowHandle,winUser.EM_CHARFROMPOS,0,p) | ||
| offset=winUser.LOWORD(res) | ||
| lineNum=winUser.HIWORD(res) | ||
| if offset==0xFFFF and lineNum==0xFFFF: | ||
| raise LookupError("Point outside client aria") | ||
| raise LookupError("Point outside client area") |
| return RectLTWH.fromCollection(*rects).toPhysical(self.obj.windowHandle) | ||
|
|
||
| def _getFirstVisibleOffset(self): | ||
| return 0 |
There was a problem hiding this comment.
How do you know the first offset here will be 0?
There was a problem hiding this comment.
Because this is the display model, all characters within the model are visible on the screen.
| @rtype: L{locationHelper.RectLTWH}; C{None} if there is no rectangle. | ||
| @raise NotImplementedError: If not supported. | ||
| @raise LookupError: If not available (i.e. off screen, hidden, etc.) | ||
| """ |
There was a problem hiding this comment.
When your abstract properties come, should this be _abstract_boundingRect?
There was a problem hiding this comment.
No. Abstract properties should be implemented. A NotImplementedError or LookupError raised by this property should be caught. Not every TextInfo has position info.
source/textInfos/offsets.py
Outdated
| def _getPointFromOffset(self,offset): | ||
| # Purposely do not catch LookupError or NotImplementedError raised by _getBoundingRectFromOffset. | ||
| rect = self._getBoundingRectFromOffset(offset) | ||
| # For backwards compatiblity, keep using textInfos.Point instead of locationHelper.Point |
|
@LeonarddeR Is the blocked label still appropriate? I'm holding off on reviewing this, waiting for the comments by @derekriemer to be addressed. |
LeonarddeR
force-pushed
the
boundingRectangle
branch
from
d60d2b9
to
840ad27
Compare
|
@michaelDCurran: As this is a quite big change in terms of code additions, I'm also requesting review from you, since you know the textInfos code pretty well. |
…fixing merge conflicts
|
I have not read through the code too carefully yet, but I am concerned about having a boundingRect property on a textInfo rather than a boundingRects property. If you have a range that starts say a few characters in from the end of one line, and extends a few characters onto the second line, then the boundingRect would include the entirity of both lines. Is This really what you want for highlighting / magnification? I would have thought you'd want two rects, one for the end of the first line, and one for the beginning of the second? Most APIs I have seen expose boundingRects not a boundingRect for this reason. |
|
I think your point makes sense. The current code will indeed create a
bounding rect that may contain coordinates that fall outside the range.
I now see that having several bounding rectangles is also beneficial for
magnifiers when it comes to text selection (i.e. based on is selection
anchored at start).
Would you prefer a boundingRects property that behaves similar to the
UIA boundingRects property?
|
|
Similar, though I think UIA's boundingRects property is flattened to one
1d array. We can obviously do better than that and have a list of rects.
|
…findNextContent manipulates the textInfo instance
…o a pointAtStart implementation
|
@michaelDCurran commented on 2 Nov 2018, 00:27 CET:
@michaelDCurran: This is now addressed, so if you could have a look at this again, that'd be great! |
| else: | ||
| raise NotImplementedError | ||
| raise LookupError |
There was a problem hiding this comment.
This seems to be introducing a new exception that _getOffsetFromPoint might raise. It's quite hard to track down all the places that might encounter this exception, how have you determined that this won't change the control flow in an unintentional way?
There was a problem hiding this comment.
It is actually exactly the other way around. offsetFromPoint is expected to raise LookupErrors, see for example event_mouseMove in the base NVDAObject, which is responsible for speaking text under the mouse. In the case where a RuntimeError was raised, this error wasn't caught and would result into a massive amount of errors whenever you move the mouse within such an object. So I believe that RuntimeError was wrong here.
| except (NotImplementedError, LookupError): | ||
| pass | ||
| if obj == copy._endObj: | ||
| # We're ad the end of the range. |
| #p=POINT(position.x,position.y) | ||
| #self._rangeObj=self.obj.UIATextPattern.RangeFromPoint(p) | ||
| raise NotImplementedError | ||
| #rangeFromPoint used to cause a freeze in UIA client library! |
There was a problem hiding this comment.
If we think that the freeze could still occur, then is it really safe to re-introduce this code?
source/NVDAObjects/UIA/__init__.py
Outdated
| if not rectArray: | ||
| return rects | ||
| rects.extend( | ||
| RectLTWH.fromFloatCollection(*rectArray[i:i+4]) |
There was a problem hiding this comment.
I'm guessing this is trying to split up one long array of numbers into chunks of 4?
This is particularly hard to read the comprehension is split over multiple lines. I think this would be much clearer done as a few more statements:
rectIndexes = xrange(0, len(rectArray), 4)
rectList = RectLTWH.fromFloatCollection(*rectArray[i:i+4]) for i in rectIndexes
rects.extend(rectList)
source/NVDAObjects/window/edit.py
Outdated
| (left,top,width,height)=self.obj.location | ||
| point.x=point.x+left | ||
| point.y=point.y+top | ||
| if -1 in (point.x, point.y): |
There was a problem hiding this comment.
is -1 a magic value? Can you add a comment about where it comes from.
There was a problem hiding this comment.
See the remarks section here. That actually made me change the check to point.x<0 or point.y<0. Thanks for catching this!
source/NVDAObjects/window/winword.py
Outdated
| left = ctypes.c_int() | ||
| top = ctypes.c_int() | ||
| width = ctypes.c_int() | ||
| height = ctypes.c_int32() |
There was a problem hiding this comment.
Why is height the only c_int32()?
There was a problem hiding this comment.
Ugh, it doesn't matter at all since they evaluate both to c_long, but I fixed this, thanks!
|
@feerrenrut: re your question about different display scaling, I haven't yet tested this. I assume I should do this for Word and UIA. For IA2, we already had code for pointFromOffset |
There was a problem hiding this comment.
re your question about different display scaling, I haven't yet tested this. I assume I should do this for Word and UIA. For IA2, we already had code for pointFromOffset
Yes, that sound fair. If you believe the testing to be adequate here, then I'm happy for this to be merged.
|
Thanks for approving!
I tested in Word with and without UIA by moving the mouse to several
characters in a sentence. My colleague verified visually that the mouse
ended up at the right character. Also mouse echo reported the right
character.
I think and hope we're good to go.
|
Link to issue number:
Fixes #7916
Fixes #8370
Fixes #8371
Summary of the issue:
NVDA's location information for text info is primarily based on screen points. For example, offset based text info implementations use the _getPointFromOffset function to get the screen coordinates for a particular point.
By default however, the display model and IAccessible2 offer per character position information based on rectangles instead of points, which is more accurate than points. For example, when creating a virtual caret in Firefox and Chrome virtual buffers, the virtual caret has to be positioned at the right of the currently selected character. Furthermore, based on rectangle information, it is possible to calculate the bounding rectangle of an entire text range. This makes it possible to the get the bounding rectangle of a selection in order for a magnifier to track to, a highlight to be positioned correctly, a virtual selection to be shown visually, etc.
Non-offset based text controls, like UIA based controls and MS Word, do expose per text range bounding rectangle information. For Word, you can retrieve the bounding rectangle of a text range, whereas for UIA, there are per line bounding rectangles within the text range. These cases are currently not implemented in NVDA, which means that the route mouse to navigator object is only able to route the mouse to the object itself, rather than the exact character the review cursor is at.
Description of how this pull request fixes the issue:
This pr builds upon locationHelper and is the predecessor of another huge pr that will implement focus and browse mode caret highlighting in the core of NVDA. It will also add a framework that lays the foundation to implement support for third party magnifier software or APIs in the future.
This pr contains the following.
Added a boundingRect property to textInfos.
NVDAObjectTextInfo returns the location of the object.
The Word TextInfo object gets the bounding rectangle from the text range.
UIATextInfo gets the per line bounding rectangles from the text range and creates a bounding
rectangle of the full text range using locationHelper
DisplayModelTextInfo creates a bounding rectangle from the rectangles in the range.
OffsetsTextInfo contains the most complex implementation. It calculates a bounding rectangle of the text range based on the locations of:
As this is very performance heavy for huge text ranges, I added _getFirstVisibleOffset and _getLastVisibleOffset methods that can help in limiting the range to the characters that are visible on screen. This works properly for EditTextInfo, for IA2TextInfo the support is more limited. However, I tried to make the first and last visible offset methods as conservative as possible, (i.e. when the retrieved offset might not be reliable, a LookupError is raised.
TreeCompoundTextInfo calculates a bounding rectangle based on the bounding rectangles of the underlying text infos.
MozillaCompoundTextInfo tries to do the same, but its implementation differs from TreeCompoundTextInfo.
Specific for UIA:
The base implementation of TextInfo.pointAtStart now tries to get the top left corner of the boundingRect property. Also, for OffsetsTextInfo, pointAtStart first tries to get the upper left corner of the start offset bounding rectangle before using _getOffsetFromPoint.
For OffsetsTextInfo, the base implementation of _getPointFromOffset uses the center of the rectangle retrieved with _getBpoundingRectFromOffset. For IA2TextInfo and DisplayModelTextInfo, _getPointFromOffset has been removed and is replaced by an implementation of _getBoundingRectFromOffset.
A new hasRelevantLocation property has been added to NVDAObject. This property will be used to determine whether the location of an object can be used by a highlighter or magnifier to highlight or track to an object, respectively. This property returns True if the following conditions are met:
This property is used by CompoundTextInfos to ignore the bounding rectangle of objects that are off screen or unavailable while calculating the bounding rectangle of a compound text range.
Testing performed:
Known issues with pull request:
Change log entry:
Bug fixes
Changes for developers