1597742 - atk_text_scroll_substring_to_point() only works if the accessible object is offscreen

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P3)

Description

[Joanmarie Diggs]

Attachment: non-sticky-header.html

Steps to reproduce:

1. Load the attached test case and size the window so that Heading #1, #2, and #3 are the only visible headings.
2. In Accerciser, perform the following in the iPython console:

# Select the document and get its bounding box.
In: acc.queryComponent().getExtents(DESKTOP_COORDS)
Out: [1970, 161, 876, 498]

# Select the on-screen "Heading #3" and scroll its substring to the top.
In: acc.queryText().scrollSubstringToPoint(0, 9, DESKTOP_COORDS, 1970, 161)
Out: True

^^^ Heading #3 hasn't actually moved though ^^^

# Select the off-screen "Heading #4" and scroll its substring to the top.
In: acc.queryText().scrollSubstringToPoint(0, 9, DESKTOP_COORDS, 1970, 161)
Out: True

^^^ Heading #4 is now at the top of the screen as expected ^^^

# Select the on-screen "Heading #5" and scroll its substring to the top.
In: acc.queryText().scrollSubstringToPoint(0, 9, DESKTOP_COORDS, 1970, 161)
Out: True

^^^ Heading #5 hasn't actually moved though ^^^

# Select the off-screen "Heading #3" and scroll its substring to the top.
In: acc.queryText().scrollSubstringToPoint(0, 9, DESKTOP_COORDS, 1970, 161)
Out: True

^^^ Heading #3 is now at the top of the screen as expected ^^^

Impact: Orca cannot reliably scroll substrings to the desired location.

Comment 1

[Joanmarie Diggs]

Hey Jamie. Is this behavior intentional? I think it's a bug, but it's so consistent as to seem like it's on purpose. :)

Comment 2

[James Teh [:Jamie]]

Ug. Looking at the code, I don't see anything that does this "on purpose", nor can I think of any reason we'd enforce this deliberately. That said, I'm far from familiar with this area of the code, especially as it calls into layout.

Comment 3

[Colomban Wendling]

Attachment: 1-a11y__Fix_ScrollSubstringToPoint_when_the_region_is_visible.patch

I could not test this yet (will try and do so in the upcoming days), but this should fix ScrollSubstringToPoint() so that it actually does scroll to the requested location in all cases.

Currently, ScrollSubstringToPoint() implicitly uses the WhereToScroll::IfNotFullyVisible flag, which leads to the described behavior: if the target location is fully visible, nothing happens. This really sounds like a bug in all platforms to me as this method explicitly gets a precise location to where the content is expected to be scrolled to; but if it's actually what is expected by a11y layers other than ATK, then fixing this will be a little more involved (either duplicating some of the code or adding an extra flag somewhere to control what the caller expects).

Comment 4

[Samuel Thibault]

Oh, that looks like a very good culprit indeed, you have my +1 on that change :) (after checking that it does fix it)

I believe that on all platforms *ToPoint() are supposed to always scroll: their documentation talk about bringing the piece to a given position on the screen, they do not talk about visibility.

Comment 5

[Colomban Wendling]

I just tested this, and the patch works nicely as promised on my end, and fixes the issue.

Comment 6

[Samuel Thibault]

Attachment: Bug 1597742 a11y: Make ScrollSubstringToPoint always scroll, not only when invisible r=jamie

invisible r=jamie

Comment 7

[Samuel Thibault]

Here is the patch on phabricator, a try is going on on https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c47245b297a4fecc7431f3a833f8a3c752ec92c&selectedJob=285223388 , it's looking good.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba6cb6765a5d
a11y: Make ScrollSubstringToPoint always scroll, not only when invisible r=Jamie

Comment 9

[James Teh [:Jamie]]

Attachment: Bug 1597742: Add tests for HyperTextAccessible::ScrollSubstringToPoint.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ad80659be06
Add tests for HyperTextAccessible::ScrollSubstringToPoint. r=MarcoZ

Comment 11

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 1ad80659be06 (bug 1597742) for mochitest failures at test_scrollSubstringToPoint.html.
https://hg.mozilla.org/integration/autoland/rev/b207525a697640caf0084673cbf4c5378067df2d

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=285334334&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=1ad80659be06562493395e5c346abe323fb05962

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=285334334&repo=autoland&lineNumber=3180

Comment 12

[Samuel Thibault]

So the test shows a discrepancy of a dozen pixels. I am actually not very surprised, since HyperTextAccessible::ScrollSubstringToPoint converts the position into an integer number of percent and requests that amount of scrolling, so there is some rounding here, we can not expect a completely perfect scrolling. Perhaps just reducing the number of pixel of the padding (currently 10000) could help here.

Very tall webpages however aren't that uncommon, 10000 pixels is just worth about a dozen pages , and much taller pages can be found online. Since integer values are not precise enough, perhaps we can make WhereToScroll a floating-point value, or perhaps a fixed-point value if performance really matters here? (but that'd make the code more confusing)

Comment 13

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/ba6cb6765a5d

Comment 14

[Samuel Thibault]

I've made that in https://treeherder.mozilla.org/#/jobs?repo=try&revision=f396eab55c1740aa24c4e7efb17b736054abf5c3&selectedJob=285337625 , just to see how well it goes.

Comment 15

[Samuel Thibault]

Hum, it's worse, more investigation needed :)

Comment 16

[Samuel Thibault]

Joanmarie, can you perhaps verify that today's nightly build, which contains the fix, behaves as you wish? (except the precision part for very long pagaes)

Comment 17

[Joanmarie Diggs]

Yeah, that seems to work nicely! Way to go guys, and thanks!!

Comment 18

[Samuel Thibault]

Perhaps this could go into firefox 72 and 73?

Comment 19

[James Teh [:Jamie]]

(In reply to Samuel Thibault from comment #18)

Perhaps this could go into firefox 72 and 73?

Definitely not going to happen for 72. It might be possible for 73 if I can get the test to pass.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/093a40a347df
Add tests for HyperTextAccessible::ScrollSubstringToPoint. r=MarcoZ

Comment 21

[Stefan Hindli [:stefan_hindli]]

Backed out changeset 093a40a347df (Bug 1597742) for causing perma a11y failures in test_scrollSubstringToPoint.html CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=e0a14dad032f7327df45fcd119494f241d2476aa&selectedJob=285742535

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=285742535&repo=autoland&lineNumber=3658

Comment 22

[James Teh [:Jamie]]

Ug. I ran this against try and it passed. I don't get it.

Comment 23

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/62a1b7ad918e
Backed out changeset 093a40a347df for causing perma a11y failures in test_scrollSubstringToPoint.html CLOSED TREE

Comment 24

[James Teh [:Jamie]]

I'm going to close this and spin off a separate bug for the tests, since the actual patch has already landed and reopening this is confusing.

Comment 25

[Phabricator Automation]

Comment on attachment 9121441 [details]
Bug 1597742: Add tests for HyperTextAccessible::ScrollSubstringToPoint.

Revision D60207 was moved to bug 1610461. Setting attachment 9121441 [details] to obsolete.