1610461 - Add tests for HyperTextAccessible::ScrollSubstringToPoint

Status: NEW

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

Description

[James Teh [:Jamie]]

Bug 1597742 fixed ScrollSubstringToPoint so that it scrolls to the target point even when the object is already visible somewhere else. However, we didn't previously have tests for ScrollSubstringToPoint and that patch didn't add any either. I pushed an additional patch to add them, but even though it works for me (and on try for some platforms), it fails on others due to the coordinates being slightly off. See bug 1597742 comment 12 onwards. Rather than confusing things on that bug, I'm opening this new bug to deal with the tests.

Comment 1

[Colomban Wendling]

(In reply to James Teh [:Jamie] from comment #0)

[…] even though it works for me (and on try for some platforms), it fails on others due to the coordinates being slightly off.

So you say that some platforms always work, or that it is intermittent? It might be an important information towards fixing this.

Comment 2

[James Teh [:Jamie]]

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

Comment 3

[James Teh [:Jamie]]

It seems to work locally for me on Windows and on an Ubuntu 18.09 vm. I even submitted two try jobs where it passed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=28c5884edaa6b60a92d5c87f4beb50e0494597f3
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ee45e0beffe2e9699065da4d8b310b0e0de56b7
It's worth noting that none of these ran the full test suite. I'm starting to wonder whether some other test run before this interferes somehow. However, the last backout was due to a failure with a debug build, which I haven't tested.

Honestly, I don't know what's going on. I don't really have time to dig into it at the moment, unfortunately.

Comment 4

[Colomban Wendling]

Weird.

Manually testing suggests that the scrolling is not precise enough (test with Accerciser's console, acc being para1 of your patch):

In [1]: acc.queryComponent().getExtents(DESKTOP_COORDS)
[114, 175, 1252, 19]
In [2]: acc.queryText().scrollSubstringToPoint(0, 9, DESKTOP_COORDS, 114, 175)
True
In [3]: acc.queryComponent().getExtents(DESKTOP_COORDS)
[114, 168, 1252, 19]

See that although we tried to scroll to the same location (y=175), the component actually moved to y=168. And I back this up visually. If I choose y values close to that one, I see that some scroll coordinates "stick": scrolling to an y between 170 and 177 (inclusive) results in a y extent of 168, scrolling to an y between 162 and 169 (inclusive) results in a y extent of 160, etc.

This suggests Samuel was right and the precision is not good enough. I just manually tested his floating-point patch, and although he claimed the result were worse (probably because of the larger offset in one of the tests), I find this to be actually way better: we can now scroll almost pixel-perfect.

However, there still is a bug (which might be one of the causes for the test failures) with this patch: there can be an "small" offset in reported position and corresponding scroll. And this can add up, by a "get the position and scroll to it" loop:

n [1]: acc.queryComponent().getExtents(DESKTOP_COORDS)
[14, 200, 1252, 19]
In [2]: acc.queryText().scrollSubstringToPoint(0, 9, DESKTOP_COORDS, 14, 200)
True
In [3]: acc.queryComponent().getExtents(DESKTOP_COORDS)
[14, 199, 1252, 19]
In [4]: acc.queryText().scrollSubstringToPoint(0, 9, DESKTOP_COORDS, 14, 199)
True
In [5]: acc.queryComponent().getExtents(DESKTOP_COORDS)
[14, 198, 1252, 19]
[…]

See that the Y value is getting smaller and smaller despite the scrollSubstringToPoint() call asking for the same Y.
The amount is not necessarily the same: if I work on para1, I get an offset of 1, on para2 I get one of 2, and on the last paragraph (the one with 2000px bottom-padding) I get an offset of 3. However, I can move those per-pixel -- there just is this offset.

I'm not sure if that's a rounding issue somewhere (possible as the coordinates ultimately are full device or app units), or an offset between the component coordinates and the text ones (which wouldn't be crazy if there are padding involved, but merely adding a * {padding:0;} doesn't change anything), but that definitely makes things not perfect.
I tried to look this up a little, but with no luck: the only places where I saw that maybe rounding issues could add up was in layout code (e.g. layout/base/PresShell.cpp:ComputeWhereToScroll()) which I would neither dare touching nor know how to change. That however doesn't mean there isn't something I missed.

Comment 5

[Colomban Wendling]

Hum, I'm wondering if there isn't a conceptual issue here. What Firefox's scrollSubstringToPoint() does is that it moves the region containing the given offsets (so, a rect, likely with a non-0 height and possibly width), and it moves this percent-wise so that it is always visible when the value is between 0% and 100%. This comes from the fact that a scroll of 100% means "align the bottom of the element to the bottom of the view", and 0% means "align the top of the element to the top of the view" (on the Y coordinates, similar applies to the X one with left and right edges).

This means that using scrollSubstringToPoint() to scroll the top-left of the rect somewhere requires adjusting the values, we can't simply ask for the X/Y at the bottom edge, as that would scroll the bottom edge of the rect there. Similarly, scrolling to the middle of the page moves the middle of the rect to the middle of the page. It's unclear to me what ATK expects, but clearly I at least was confused by this behavior, and I think Joanmarie was as well.

So I'm wondering what should be adjusted here, but it seems overly complex to ask the caller to compute the X and Y positions taking this into account, if they want to place things at a very specific pixel position: they have to do something like y = round(y + range_rect.height * (range_rect.y - page_rect.y) / page_rect.height) (add the range height times the percentage of page rect they target).

Comment 6

[Samuel Thibault]

It's unclear to me what ATK expects

ATK expects that the top-left corner of the rectangle of the accessible be placed by scrolltopoint at the given (x,y) position, just like IAccessible2's interface: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/IAccessible2#scrollToPoint()

Comment 7

[Colomban Wendling]

OK, so I'd say there's a bug in Firefox here, probably around accessible/generic/HyperTextAccessible.cpp:HyperTextAccessible::ScrollSubstringToPoint(), where it should adjust the vPercent/hPercent accordingly. I'm not sure how I can get the range size (in app or device unit) in the function, but that done would boil down to pretty much the math I pasted above.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:Jamie, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

Redirect a needinfo that is pending on an inactive user to the triage owner.
:Jamie, since the bug has recent activity, could you have a look please?

For more information, please visit auto_nag documentation.