Closed Bug 1597742 Opened 3 years ago Closed 3 years ago

atk_text_scroll_substring_to_point() only works if the accessible object is offscreen

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla74
Tracking Status
firefox74 --- fixed

People

(Reporter: jdiggs, Assigned: samuel.thibault)

References

Details

Attachments

(3 files, 1 obsolete file)

Attached file 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.

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

Flags: needinfo?(jteh)

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.

Flags: needinfo?(jteh)
Priority: -- → P3

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

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.

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

Attachment #9121342 - Attachment description: Bug 1597742 a11y: Make ScrollSubstringToPoint always scroll, not only when → Bug 1597742 a11y: Make ScrollSubstringToPoint always scroll, not only when invisible r=jamie
Assignee: nobody → samuel.thibault
Status: NEW → ASSIGNED
Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ba6cb6765a5d
a11y: Make ScrollSubstringToPoint always scroll, not only when invisible r=Jamie
Pushed by mzehe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1ad80659be06
Add tests for HyperTextAccessible::ScrollSubstringToPoint. r=MarcoZ

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)

Flags: needinfo?(samuel.thibault) → needinfo?(jteh)
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74

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

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)

Flags: needinfo?(jdiggs)

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

Flags: needinfo?(jdiggs)

Perhaps this could go into firefox 72 and 73?

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

Flags: needinfo?(jteh)
Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/093a40a347df
Add tests for HyperTextAccessible::ScrollSubstringToPoint. r=MarcoZ
Status: RESOLVED → REOPENED
Flags: needinfo?(samuel.thibault)
Resolution: FIXED → ---
Target Milestone: mozilla74 → ---

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

Flags: needinfo?(samuel.thibault)
Backout by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/62a1b7ad918e
Backed out changeset 093a40a347df for causing perma a11y failures in test_scrollSubstringToPoint.html CLOSED TREE

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.

Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74
See Also: → 1610461

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.

Attachment #9121441 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.