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

Get reliable points from offsets in virtual buffers #8479

Merged
merged 26 commits into from
Aug 20, 2018
Merged

Get reliable points from offsets in virtual buffers #8479

merged 26 commits into from
Aug 20, 2018

Conversation

LeonarddeR
Copy link
Collaborator

@LeonarddeR LeonarddeR commented Jul 4, 2018

First of all, many thanks to @jcsteh for sharing his ideas with regard to how to implement this.

Link to issue number:

Fixes #6460

Summary of the issue:

When calculating the position of text in virtual buffers, NVDA uses the position of the object the text belongs to, rather than the real position of the offset you're retrieving the point for.

Description of how this pull request fixes the issue:

  1. Added two attributes to virtual buffer text nodes as discussed with @jcsteh:

  2. the Gecko virtual buffer text info now has its own _getPointFromOffset implementation that uses the new virtual buffer attributes.

  3. VirtualBufferTextInfo.getTextWithFields is now split up into a helper function _getFieldsInRange, similar to DisplayModelTextInfo. This allows for easy retrieval of control/format fields in a particular range, regardless where the text info is positioned.

  4. pointFromOffset in the adobe acrobat virtual buffer now uses the _indexINParent attribute. It seems Adobe Acrobat has separate dom nodes for every word, so pointFromOffset now gets the center point of the word, which is far better than it was before.

Testing performed:

  • Tested several pages in Chrome and Firefox. Made sure that NVDA properly falls back to the center of an object when it has no text (i.e. a graphic or a link with an overridden aria-label).
  • Tested with Adobe Reader 2018.011.20040. Though the implementation has some exception handling, testing with older versions might be a good thing to do. I have also tested with Adobe Reader XI, and This implementation indeed, that doesn't provide per word objects.should handle that gracefully, though.

Known issues with pull request:

  • Chrome testing was somewhat limited because it seems that getOffsetFromPoint is broken for Chrome.

Change log entry:

@Adriani90
Copy link
Collaborator

fixes probably also #7915

@LeonarddeR LeonarddeR changed the title Get reliable points from offsets in Mozilla and Chrome virtual buffers Get reliable points from offsets in virtual buffers Jul 4, 2018
@LeonarddeR
Copy link
Collaborator Author

@Adriani90 commented on 4 jul. 2018 15:34 CEST:

fixes probably also #7915

Would you be able to verify that using this try build? Note that this try build doesn't contain the adobe code yet.

@Adriani90
Copy link
Collaborator

Adriani90 commented Jul 4, 2018

@LeonarddeR I have tested the issues and got following results:

#6460: I cannot reproduce it at all, neither with NVDA 2018.2.1. But the webpage from NV Access has been redesigned. I tried different selections but I get different offset points for x and y, both at the start and at the end of the offset.

#7853: I confirm that your try build solves the issue.

#7915: unfortunately there is no improvement of mouse routing to navigator object in google chrome at all.

#7916: I guess the getPointFromOffset in MS Edge is broken as well. Mouse routing dows not work either.

@LeonarddeR
Copy link
Collaborator Author

@Adriani90 commented on 4 jul. 2018 22:20 CEST:

#7915: unfortunately there is no improvement of mouse routing to navigator object in google chrome at all.

How did you test this?

@Adriani90
Copy link
Collaborator

Adriani90 commented Jul 5, 2018 via email

@LeonarddeR
Copy link
Collaborator Author

Note that you should not use object navigation to test this, because that will always set the mouse to the start of the object you selected. You should just move the browse mode caret character by character or word by word and than move the mouse to the navigator object. This script description is actually confusing, as conceptually, the mouse is moved to the review position.

@Adriani90
Copy link
Collaborator

I have tested as you adviced. There is still the same behavior like in NVDA 2018.2.1. I have tried on google, hello magazine, NV Access and other websites. I have also tried to route the mouse in focus and browse mode. It does not move the mouse to the object at all. It just reports "Window" or "Pane" but it does not behave like in firefox, or internet explorer where the mouse is routed much more accurately.

@Adriani90
Copy link
Collaborator

Adriani90 commented Jul 5, 2018

But there is a huge improvement in Mozilla Firefox, with your try build. The mouse can be routed from word to word which is quite reliable compared to NVDA 2018.2.1.

In Internet explorer, Google Chrome and MS Edge there is no difference between NVDA 2018.2.1 and the try build.

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Jul 5, 2018 via email

@Adriani90
Copy link
Collaborator

On some buttons on Github, when I try to route the mouse in Firefox, I get following error:

ERROR - scriptHandler.executeScript (19:26:22.085):
error executing script: <bound method GlobalCommands.script_moveMouseToNavigatorObject of <globalCommands.GlobalCommands object at 0x04F02A10>> with gesture u'Umschalt+NVDA+m'
Traceback (most recent call last):
  File "scriptHandler.pyc", line 187, in executeScript
  File "globalCommands.pyc", line 692, in script_moveMouseToNavigatorObject
  File "baseObject.pyc", line 21, in __get__
  File "textInfos\offsets.pyc", line 333, in _get_pointAtStart
  File "virtualBuffers\gecko_ia2.pyc", line 43, in _getPointFromOffset
  File "virtualBuffers\__init__.pyc", line 175, in _getPointFromOffset
TypeError: 'NoneType' object is not iterable
INFO - globalCommands.GlobalCommands.script_navigatorObject_devInfo (19:26:25.341):
Developer info for navigator object:
name: None
role: ROLE_UNKNOWN
states: 
isFocusable: False
hasFocus: False
Python object: <NVDAObjects.IAccessible.IAccessible object at 0x05233A30>
Python class mro: (<class 'NVDAObjects.IAccessible.IAccessible'>, <class 'NVDAObjects.window.Window'>, <class 'NVDAObjects.NVDAObject'>, <class 'documentBase.TextContainerObject'>, <class 'baseObject.ScriptableObject'>, <class 'baseObject.AutoPropertyObject'>, <type 'object'>)
description: None
location: None
value: None
appModule: <'firefox' (appName u'firefox', process ID 13872) at address 50bdc10>
appModule.productName: u'Firefox'
appModule.productVersion: u'60.0.2'
TextInfo: <class 'NVDAObjects.NVDAObjectTextInfo'>
windowHandle: 197640
windowClassName: u'MozillaWindowClass'
windowControlID: 0
windowStyle: 399441920
windowThreadID: 7908
windowText: u'Get reliable points from offsets in virtual buffers by leonardder \xb7 Pull Request #8479 \xb7 nvaccess/nvda - Mozilla Firefox'
displayText: exception: 'NoneType' object is not iterable
IAccessibleObject: <POINTER(IAccessible) ptr=0x6866d00 at 51a6210>
IAccessibleChildID: -33560358
IAccessible event parameters: windowHandle=197640, objectID=-4, childID=-33560358
IAccessible accName: exception: (-2147024809, 'Falscher Parameter.', (None, None, None, 0, None))
IAccessible accRole: exception: (-2147024809, 'Falscher Parameter.', (None, None, None, 0, None))
IAccessible accState: exception: (-2147024809, 'Falscher Parameter.', (None, None, None, 0, None))
IAccessible accDescription: exception: (-2147024809, 'Falscher Parameter.', (None, None, None, 0, None))
IAccessible accValue: exception: (-2147024809, 'Falscher Parameter.', (None, None, None, 0, None))

The buttons are:
Add header text
Add bold text <ctrl+b>
Add italic text <ctrl+i>

Insert a quote
Insert code
Add a link <ctrl+k>

Add a bulleted list
Add a numbered list
Add a task list

Directly mention a user or team
Reference an issue or pull request
"Menu button" Insert a reply

@Adriani90
Copy link
Collaborator

on the top of the github page, for some elements I get following error:

ERROR - eventHandler.executeEvent (19:31:24.341):
error executing event: mouseMove on <NVDAObjects.IAccessible.mozilla.Mozilla object at 0x052BA7B0> with extra args of {'y': 251, 'x': 3200}
Traceback (most recent call last):
  File "eventHandler.pyc", line 152, in executeEvent
  File "eventHandler.pyc", line 92, in __init__
  File "eventHandler.pyc", line 100, in next
  File "NVDAObjects\__init__.pyc", line 930, in event_mouseMove
  File "NVDAObjects\IAccessible\__init__.pyc", line 145, in expand
AttributeError: 'NoneType' object has no attribute 'rindex'

This is the case for following elements:

Link Homepage

Button Create new…
Button View profile and more

@Adriani90
Copy link
Collaborator

Adriani90 commented Jul 5, 2018

The attribute "Rindex" error appears in Google Chrome everytime as well, much more often than in Firefox.

@Adriani90
Copy link
Collaborator

Adriani90 commented Jul 5, 2018

In Edge I don't get any entry in the log file unfortunately, there is no input registered.
The mouse routing in Edge seems to fail on simple text and in the footer of the websites. But it works quite well for links, headings and buttons.

@Adriani90
Copy link
Collaborator

In Internet explorer there is also no entry in the log file when mouse routing fails.

@LeonarddeR
Copy link
Collaborator Author

@Adriani90 commented on 5 jul. 2018 19:32 CEST:

On some buttons on Github, when I try to route the mouse in Firefox, I get following error:

AH, this bug was already there actually. Fixed it while at it.

on the top of the github page, for some elements I get following error:

Hmm this error seems to be quite unrelated to this pr.

In Edge I don't get any entry in the log file...

Could you please limit your comments and reports to the scope of this pr, thus, adobe reader, Firefox and Chrome? As I noted earlier, Edge will be dealt with at a later moment. For Internet Explorer, I"m afraid there's nothing we can do to improve mouse tracking.

@LeonarddeR LeonarddeR dismissed stale reviews from michaelDCurran and jcsteh via c19970e August 6, 2018 08:00
@LeonarddeR
Copy link
Collaborator Author

Here is a try build that should move the mouse to the center of the characters in Firefox and Chrome.

@@ -132,7 +132,7 @@ def _getPointFromOffsetInObject(cls,obj,offset):
# believes that it belongs to offset+1.
# This means that, when you use l{_getOffsetFromPoint}, the result would be offset+1.
# We yet stay with the center of the character, though.
point=textInfos.Point(res[0]+res[2]/2,res[1]+res[3]/2)
point=textInfos.Point(res[0]+(res[2]/2),res[1]+(res[3]/2))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than doing the math here, can you use the location helper classes? It would make it clearer that characterExtents returns (x,y,width,height)

except COMError:
raise NotImplementedError
# Normally, L{_getPointFromOffset} implementations that rely on
# character bounding rectangles use the center point of the rectangle.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment still correct? It makes it sound like this will not use the center, which is what it appears to do.

@feerrenrut
Copy link
Contributor

I'm testing out this new build (nvda_snapshot_try-geckoPointFromOffset-15793,c19970ec.exe)

Indeed in chrome and firefox the middle of the word / character is now selected.

After NVDA moves the mouse the paragraph is read out,

You can change this in NVDA's mouse settings, setting the resolution to character or word.

One point is that I do not have mouse tracking enabled, should it still read the paragraph? The amount reported does obey the resolution setting.

Yes, that is, when mouse tracking is on and you selected the desired text unit resolution. Note that, if NVDA somehow reads the wrong information, this is strictly spoken unrelated, i.e. getPointFromOffset is used when the mouse is moved to a particular character, and getOffsetFromPoint is used when calculating the offset a character belongs to. Offset from point calculation happens at the object level, not add the tree interceptor/virtual buffer level.

For some reason I can't seem to get anything intelligible when using Navigate to the object under the mouse I wanted to test that selecting the center (rather than the top left) does not break getOffsetFromPoint for the same position. I think that the behaviour of these should be symmetrical.

@LeonarddeR
Copy link
Collaborator Author

@feerrenrut commented on 7 aug. 2018 06:48 CEST:

After NVDA moves the mouse the paragraph is read out,

You can change this in NVDA's mouse settings, setting the resolution to character or word.

One point is that I do not have mouse tracking enabled, should it still read the paragraph? The amount reported does obey the resolution setting.

Hmm yes, it turns out that disabling mouse tracking will still report mouse changes triggered by NVDA. I did not expect this honestly, and I'm not sure whether this is defined behavior. I'd expect it not to do this.

For some reason I can't seem to get anything intelligible when using Navigate to the object under the mouse I wanted to test that selecting the center (rather than the top left) does not break getOffsetFromPoint for the same position. I think that the behaviour of these should be symmetrical.

I tend to use this website for testing random things. Try the text that reads "Similarly, the open-ended nature of the game" for example. That will report the character that belongs to offset+1 when I move the mouse to the text. Now, your eyes should be able to determine whether pointFromOffset or offsetFromPoint fails here.

@jcsteh
Copy link
Contributor

jcsteh commented Aug 7, 2018

@leonardder commented on Aug 7, 2018, 3:14 PM GMT+10:

Hmm yes, it turns out that disabling mouse tracking will still report mouse changes triggered by NVDA. I did not expect this honestly, and I'm not sure whether this is defined behavior. I'd expect it not to do this.

I can't remember whether there was a filed issue for this change, but without this behaviour, moving mouse to review would just report nothing when mouse tracking is disabled. Silence is confusing/misleading here, IMO. We could report "Moved" or something, but if we're going to do that, we may as well report where it moved.

@feerrenrut
Copy link
Contributor

Using your link, and arrowing around (to the last letter of "players are free to attack at will") moving the mouse to the 'L' with "Move mouse to current navigator object", and using the "Navigate to the object under the mouse" then "Paragraph" is reported. Pressing "Report current word in review" and "Similarly," is reported. Which is the start of the paragraph. I don't know how to report the word under the mouse in this way.

We could report "Moved" or something, but if we're going to do that, we may as well report where it moved.

I'm happy with this, perhaps we can document it somewhere as part of this pull request? Also that the amount reported does obey the resolution setting in the mouse setting dialog.

@LeonarddeR
Copy link
Collaborator Author

@feerrenrut commented on 7 aug. 2018 11:48 CEST:

Using your link, and arrowing around (to the last letter of "players are free to attack at will") moving the mouse to the 'L' with "Move mouse to current navigator object", and using the "Navigate to the object under the mouse" then "Paragraph" is reported. Pressing "Report current word in review" and "Similarly," is reported. Which is the start of the paragraph. I don't know how to report the word under the mouse in this way.

Does the mouse end up at the exact center position of the "L" though?

@feerrenrut
Copy link
Contributor

Does the mouse end up at the exact center position of the "L" though?

Yes, in Firefox and Chrome. In Adobe reader the mouse moves to the center of the word as expected.

@LeonarddeR
Copy link
Collaborator Author

@feerrenrut: Are you done with reviewing this, or have I missed something that I haven't yet addressed?

feerrenrut
feerrenrut previously approved these changes Aug 20, 2018
Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app/chrome app/firefox BabbageWork Pull requests filed on behalf of Babbage B.V. component/text-info TextInfo objects and text review feature/browse-mode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect offset points generated in tree interceptor text info
6 participants