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

Improved text location information for UIA, Word and compound text infos #8572

Merged
merged 26 commits into from Dec 11, 2018

Conversation

Projects
None yet
6 participants
@leonardder
Copy link
Collaborator

commented Jul 30, 2018

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.

  1. 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:

      • The start of the range
      • The end of the range
      • The start and end of every line in the range.

    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.

  2. Specific for UIA:

    • It is now again possible to create a TextInfo object from a point. This support was disabled in the code as the client library was said to freeze under Windows 7. I've not been able to reproduce these freezes under Windows 10. I re-enabled this for now, however this requires some testing under Windows 7. @michaelDCurran pointed out that textarea nodes under Internet Explorer 11 might prove a good test case. @josephsl: WOuld you be able to assist with this?
    • STATE_OFFSCREEN is now properly added to UIA objects that have the OffSccreen property.
  3. 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.

  4. 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.

  5. 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:

    • The object isn't off screen
    • The object doesn't have the unavailable state
    • The location is not None
    • At least one location coordinate is nonzero

    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:

  1. Tested the boundingRect property and _getBoundingRectFromOffset implementations for the cases described above.
  2. IN Edge browse mode and Word, tested the move mouse to navigator object command. The mouse echo corresponded with the character at the caret.

Known issues with pull request:

  1. As noted above, when creating an UIA text info from a point under Windows 7 at least, freezes might occur. This has to be tested before this behavior can be enabled in release versions of NVDA.
  2. Especially for compound text, the performance of bounding rectangle retrieval might be slow.

Change log entry:

  • Bug fixes

    • The functionality of the move mouse to navigator object command has been majorly improved in Microsoft Word as well as for UIA controls, particularly Microsoft Edge. (#7916, #8371)
    • The echo of text under the mouse has been improved within Microsoft Edge and other UIA applications. (#8370)
  • Changes for developers

    • OffsetsTextInfo objects can now implement the _getBoundingRectFromOffset method to allow retrieval of bounding rectangles per characters instead of points.
    • Added a boundingRect property to TextInfo objects to retrieve the bounding rectangle of a range of text. (#8371)
Created bounding rectangle text info property and implemented it for …
…Word, UIA, offsets and Mozilla compound documents
@josephsl

This comment has been minimized.

Copy link
Collaborator

commented Jul 30, 2018

Hi,

I'd like to recuse myself from reviewing this, as I'm not quite that versed in text infos and rectangles yet.

Thanks.

@leonardder

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 30, 2018

@josephsl

This comment has been minimized.

Copy link
Collaborator

commented Jul 30, 2018

@leonardder

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 30, 2018

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:

#rangeFromPoint causes a freeze in UIA client library!

Internet Explorer 11 text inffo ffor textarea nodes is based on UIA, so in Windows 7, that'd be te way to test this.

@josephsl

This comment has been minimized.

Copy link
Collaborator

commented Jul 30, 2018

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.

@josephsl

This comment has been minimized.

Copy link
Collaborator

commented Jul 30, 2018

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:

  • One must be testing babbagecom/boundingRectangle branch (hopefully with latest master commits merged).
  • Must be running Windows 7 SP1, Windows 8.1, or Windows 10. Windows 8 does not qualify because support for it has ended in 2016.
  • If using Windows 10, preferably all releases (build 10240 onwards, although build 15063 or later is prefered due to support policy from Microsoft).

Steps:

  1. Navigate to the said website using Internet Explorer and Microsoft Edge.
  2. Locate the textarea.
  3. Verify that developer info says text info is a UIA text info instance.
  4. Open Python Console and type the following as exactly as shown:

import NVDAObjects.UIA
import textInfos
NVDAObjects.UIA.UIATextInfo(nav, textInfos.Point(randomX, randomY))

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):
File "", line 1, in
File "NVDAObjects\UIA_init_.pyc", line 316, in init
AttributeError: 'NoneType' object has no attribute 'RangeFromPoint'

Thanks.

@josephsl

This comment has been minimized.

Copy link
Collaborator

commented Jul 30, 2018

Hi,

Regarding merging master commits: I see that it was rebased on master branch, so ignore my comment.

Thanks.

@leonardder

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 30, 2018

@josephsl commented on 30 Jul 2018, 20:04 CEST:

  1. Verify that developer info says text info is a UIA text info instance.

This should be NVDAObjects.IAccessible.MSHTML.UIAMSHTMLTextInfo

I get:

Traceback (most recent call last):
File "", line 1, in
File "NVDAObjects\UIA_init_.pyc", line 316, in init
AttributeError: 'NoneType' object has no attribute 'RangeFromPoint'

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.

@leonardder

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 30, 2018

This might be an easier test case.

  1. In Internet Explorer, open this particular issue and login to github.
  2. Focus the comment text area
  3. Make sure the objects text info is an UIAMSHTMLTextInfo
  4. Focus the multi line edit control in focus mode
  5. Make sure mouse tracking is on and resolution is set to character.
  6. Type some random text
  7. Walk to the text with your right arrow key, and for every character, move the mouse to it with the move mouse to navigator object command (NVDA+numpad divide or laptop NVDA+shift+m).

If all goes well, NVDA should not freeze and mouse tracking should pronounce the character where the caret is located.

@josephsl

This comment has been minimized.

Copy link
Collaborator

commented Jul 30, 2018

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.

@leonardder

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 30, 2018

@michaelDCurran

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2018

@josephsl

This comment has been minimized.

Copy link
Collaborator

commented Jul 30, 2018

@derekriemer
Copy link
Collaborator

left a comment

Good job.

Show resolved Hide resolved source/NVDAObjects/IAccessible/__init__.py
Show resolved Hide resolved source/NVDAObjects/IAccessible/ia2TextMozilla.py
# Can't move forward any further.
break
if obj == self._endObj:
ti = self._end

This comment has been minimized.

Copy link
@derekriemer

derekriemer Aug 5, 2018

Collaborator

why does ti need reassigned here?

This comment has been minimized.

Copy link
@leonardder

leonardder Aug 20, 2018

Author Collaborator

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!

This comment has been minimized.

Copy link
@derekriemer

derekriemer Aug 5, 2018

Collaborator

Is this comment relevant to the new code?

This comment has been minimized.

Copy link
@leonardder

leonardder Aug 20, 2018

Author Collaborator

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.

This comment has been minimized.

Copy link
@feerrenrut

feerrenrut Dec 4, 2018

Contributor

If we think that the freeze could still occur, then is it really safe to re-introduce this code?

This comment has been minimized.

Copy link
@leonardder

leonardder Dec 5, 2018

Author Collaborator

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.

"""
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.

This comment has been minimized.

Copy link
@derekriemer

derekriemer Aug 5, 2018

Collaborator

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")

This comment has been minimized.

Copy link
@derekriemer

derekriemer Aug 5, 2018

Collaborator

Good catch.

return RectLTWH.fromCollection(*rects).toPhysical(self.obj.windowHandle)

def _getFirstVisibleOffset(self):
return 0

This comment has been minimized.

Copy link
@derekriemer

derekriemer Aug 5, 2018

Collaborator

How do you know the first offset here will be 0?

This comment has been minimized.

Copy link
@leonardder

leonardder Aug 20, 2018

Author Collaborator

Because this is the display model, all characters within the model are visible on the screen.

This comment has been minimized.

Copy link
@derekriemer

derekriemer Aug 26, 2018

Collaborator

ack

@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.)
"""

This comment has been minimized.

Copy link
@derekriemer

derekriemer Aug 5, 2018

Collaborator

When your abstract properties come, should this be _abstract_boundingRect?

This comment has been minimized.

Copy link
@leonardder

leonardder Aug 20, 2018

Author Collaborator

No. Abstract properties should be implemented. A NotImplementedError or LookupError raised by this property should be caught. Not every TextInfo has position info.

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

This comment has been minimized.

Copy link
@derekriemer

derekriemer Aug 5, 2018

Collaborator

compatiblity,

Show resolved Hide resolved source/textInfos/offsets.py
@feerrenrut

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2018

@leonardder Is the blocked label still appropriate?

I'm holding off on reviewing this, waiting for the comments by @derekriemer to be addressed.

@leonardder leonardder added Browse mode and removed blocked labels Aug 20, 2018

@leonardder leonardder force-pushed the BabbageCom:boundingRectangle branch from d60d2b9 to 840ad27 Oct 4, 2018

@leonardder leonardder requested a review from michaelDCurran Oct 5, 2018

@leonardder

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 5, 2018

@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.

@michaelDCurran

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2018

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.

@leonardder

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 2, 2018

@michaelDCurran

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2018

@leonardder

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 28, 2018

@michaelDCurran commented on 2 Nov 2018, 00:27 CET:

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.

@michaelDCurran: This is now addressed, so if you could have a look at this again, that'd be great!

@feerrenrut
Copy link
Contributor

left a comment

Have you tested this with different screen scaling in Windows?

else:
raise NotImplementedError
raise LookupError

This comment has been minimized.

Copy link
@feerrenrut

feerrenrut Dec 4, 2018

Contributor

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?

This comment has been minimized.

Copy link
@leonardder

leonardder Dec 5, 2018

Author Collaborator

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.

This comment has been minimized.

Copy link
@feerrenrut

feerrenrut Dec 4, 2018

Contributor

Typo: ad change to at

#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!

This comment has been minimized.

Copy link
@feerrenrut

feerrenrut Dec 4, 2018

Contributor

If we think that the freeze could still occur, then is it really safe to re-introduce this code?

if not rectArray:
return rects
rects.extend(
RectLTWH.fromFloatCollection(*rectArray[i:i+4])

This comment has been minimized.

Copy link
@feerrenrut

feerrenrut Dec 4, 2018

Contributor

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)
(left,top,width,height)=self.obj.location
point.x=point.x+left
point.y=point.y+top
if -1 in (point.x, point.y):

This comment has been minimized.

Copy link
@feerrenrut

feerrenrut Dec 4, 2018

Contributor

is -1 a magic value? Can you add a comment about where it comes from.

This comment has been minimized.

Copy link
@leonardder

leonardder Dec 5, 2018

Author Collaborator

See the remarks section here. That actually made me change the check to point.x<0 or point.y<0. Thanks for catching this!

left = ctypes.c_int()
top = ctypes.c_int()
width = ctypes.c_int()
height = ctypes.c_int32()

This comment has been minimized.

Copy link
@feerrenrut

feerrenrut Dec 4, 2018

Contributor

Why is height the only c_int32()?

This comment has been minimized.

Copy link
@leonardder

leonardder Dec 5, 2018

Author Collaborator

Ugh, it doesn't matter at all since they evaluate both to c_long, but I fixed this, thanks!

@leonardder

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 5, 2018

@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

@feerrenrut
Copy link
Contributor

left a 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.

@leonardder

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 7, 2018

leonardder and others added some commits Dec 10, 2018

@feerrenrut feerrenrut merged commit 7577804 into nvaccess:master Dec 11, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@nvaccessAuto nvaccessAuto added this to the 2018.4 milestone Dec 11, 2018

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