Closed Bug 1598299 Opened 4 years ago Closed 4 years ago

atk_component_ref_accessible_at_point() sometimes fails to return the accessible at point

Categories

(Core :: Disability Access APIs, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla76
Tracking Status
firefox76 --- fixed

People

(Reporter: jdiggs, Assigned: Jamie, NeedInfo)

References

Details

Attachments

(4 files)

Steps to reproduce:

  1. Load google.com in Firefox nightly
  2. Launch Accerciser
  3. In Accerciser locate the search button in the tree of accessibles (It can be tricky to find. For me, the path is 0, 24, 0, 0, 0, 0, 3, 0, 1, 2, 0, 0)
  4. With the search button highlighted, get its absolute coordinates from the Interface Viewer or iPython console (for me, it's 2302, 1345)
  5. In Accerciser, highlight the document-web object
  6. In Accerciser's iPython console, type the following USING COORDINATES FROM STEP 4:
    descendant = acc.queryComponent().getAccessibleAtPoint(2302, 1345, DESKTOP_COORDS)
  7. Print out the name and role of descendant:
    descendant.name, descendant.role

Expected result: descendant would be the search button

Actual results: Descendant is a nameless section

Notes: As an experiment, I drilled down in Accerciser, and for each new selection, repeated acc.queryComponent().getAccessibleAtPoint(2302, 1345, DESKTOP_COORDS).name. This continued to fail to return the search button (really, its name) until I was at the immediate parent of the push button.

Impact: Orca's mouse review feature is expected to speak the object under the mouse. Because of this bug, it doesn't do so for the search button. I tried to work around this by doing a full tree dive in Orca. While that solution works with respect to the problem at hand, the original reporter of the bug stated Orca was made extremely slow due to that change. :( Therefore, I've reverted my work around.

Since working around bugs such as this does not seem doable in Orca, it would be great if you could fix this in Gecko.

From what I can see, this is (currently) deliberate: ref_accessible_at_point returns the direct child at that point, rather than the deepest descendant at that point. The documentation for atk_component_ref_accessible_at_point says (emphasis mine):

Gets a reference to the accessible child, if one exists, at the coordinate point specified by x and y .
...
Returns
a reference to the accessible child, if one exists.

Note child, not descendant.

Of course, I'm happy to change this if that's what you want. However, is this likely to break any other ATK consumer (if there are any)? That is, would any other consumer be relying on the direct child behaviour? Also, I think it'd make sense to update the API documentation to say "descendant" instead of "child" so this is clearer.

Type: defect → enhancement
Priority: -- → P2

I don't think this is going to break anything if you return the deepest descendant. I think you'd be doing all consumers a great service in fact as you'll see from the description below. (TL;DR, bounding boxes cannot be trusted and multiple objects claim to be the accessible at point.)

For now, let's say we'll take the ATK docs literally as written. Therefore, I need to drill down using atk_component_ref_accessible_at_point(). That for the most part works:

  1. Start with the document (path: 0 24 0 0 0)
  2. Get its child at the specified point (today it's 2290, 834). I get the nameless section at path 0 24 0 0 0 0.
  3. From there I get the section at 0 24 0 0 0 3. Note this is a short little thing, with a height of 52 pixels. Its bounding box does not reflect that it contains the point we're looking for. That strikes me as a bug. But we'll ignore it.
  4. That takes me to the landmark at 0 24 0 0 0 3 0. Another small thing whose bounding box doesn't include the point we're looking for.
  5. Next is a section at 0 24 0 0 0 3 0 1. Another small thing.
  6. Next is a section at 0 24 0 0 0 3 0 1 2. This bounding box actually does contain the point in question.
  7. Next is a section at 0 24 0 0 0 3 0 1 2 0.
  8. Finally the button at 0 24 0 0 0 3 0 1 2 0 0.

So that's not too awful, if we ignore the bounding boxes of ancestors not containing the point of the descendant we're looking for. But I have another concern.

It turns out that there's another section whose bounding box physically includes the point 2290, 834. It's the section at path 0 24 0 0 0 0 4. If you ask it for the accessible at point 2290, 834 it returns itself. That to me suggests that we now have two candidate accessibles at the specified point. Which is the right one? Are we really over the search button, or are we over some empty section? Logic suggests the button is the winner, but is that guaranteed? But wait, there's more. Descendants of the section I just mentioned also have bounding boxes which contain the desired point: The child at path 0 24 0 0 0 0 4 0, and its child at path 0 24 0 0 0 0 4 0 0.

So I suppose if I need to ignore the bounding boxes of ancestors not containing the point of the descendant we're looking for, and also ignore bounding boxes of elements which claim to contain the point of the descendant we're looking for, that too can be done. (Though I hope you'll agree with me that this is a sad situation.)

That leaaves me with one last concern: Can I always count on a depth-first search giving me the real descendant at the specified point? While it worked in this particular case, there are those other accessible objects that, when asked for the accessible at point return themselves. What if those objects were earlier on in the DOM and as a consequence wound up earlier in the accessibility tree? Would atk_component_ref_accessible_at_point return it causing me to never find the button? Or should I also ignore objects when atk_component_ref_accessible_at_point() returns the object it was called on?

Given all of the above drilling down and sanity checking, which is non performant and causes Orca users to complain I'm making Orca slow, I'd really like to be able to ask the document for the deepest descendant at that point and get an answer I can rely upon. :)

A lot of the ugliness here is due to the obscurity of the web and the fact that the DOM/a11y tree doesn't map well to the visual representation of said obscure web.

Regarding the bounding boxes that don't contain the requested point, my guess is that this relates to CSS absolute positioning, floating, etc. which enable content inside this node to be outside of the bounds of the node itself. Gecko is trying to return the best answer it can given the conflicting requirements of "give me a node containing this point" and "the node must be a child". Often, both of those two requirements can't be satisfied on the obscure web. What we can do is at least return the right objects so that recursive hit testing will eventually return the correct descendant.

What I can tell you is that on Windows, hit testing seems to work pretty reliably, and the client helper function AccessibleObjectFromPoint basically just recursively hit tests through the tree. (On the Firefox side, MSAA accHitTest does the same as ATK: it only returns the child.) I'm fairly sure it doesn't do any bounds checking.

All of that said, since you're fairly sure it won't break anything for ATK clients, we will attempt to make the change to get the deepest descendant.

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

All of that said, since you're fairly sure it won't break anything for ATK clients, we will attempt to make the change to get the deepest descendant.

Thanks for the details about this issue Jamie. In which version we could expect to see this fixed? This issue currently prevents us to have a feature "speak object under the mouse" working in all case. Some button like the search button on Google isn't spoken without having this issue fixed.

Thanks in advance.

If you ask it for the accessible at point 2290, 834 it returns itself.

This sounds wrong given the documentation linked above, it should likely return NULL in this case, shouldn't it? It's easily worked around, but that might lead to nasty loops in innocent and naive clients.

Gecko is trying to return the best answer it can given the conflicting requirements of "give me a node containing this point" and "the node must be a child". Often, both of those two requirements can't be satisfied on the obscure web.

If there was a mean for Gecko to answer the "descendant at point" question, it could do the Right Thing™ without having to actually worry about the tree layout much, so long as the descendant is in that tree -- which I guess would be the case if the caller passed the document-web itself as the parent, or even the window or the app.

What we can do is at least return the right objects so that recursive hit testing will eventually return the correct descendant.

Yes, and from what I see it would be the minimum to get a correct answer, regardless of the performance issues.

What I can tell you is that on Windows, hit testing seems to work pretty reliably, and the client helper function AccessibleObjectFromPoint basically just recursively hit tests through the tree. (On the Firefox side, MSAA accHitTest does the same as ATK: it only returns the child.) I'm fairly sure it doesn't do any bounds checking.

If it's the case, then we should be able to compute the right answer indeed (or at least after some of the above issues have been dealt with). Now comes the performance concern. I'd be curious to know whether it comes from Orca being Python and the possible overhead in the bindings, in which case maybe we could have a ref_accessible_descendant_at_point() implemented in ATK, or whether the communication between ATK and Firefox is too slow (it being ATK, DBus, AT-SPI or Firefox's fault would be to be determined). Basically the question would be where is the bottleneck.

As I see it, we need to either change the semantics for ref_accessible_at_point() as Joanmarie asked (which doesn't actually seem problematic to me in practice), or add a new ATK method to answer it. Whether we'd need to extend the AT-SPI interfaces so the app can answer it or whether it's a mere ATK helper would depend on whether we can get meaningful results with the current APIs, and whether it's fast enough. However, it'll always allow for better results if the app has control over it -- and I think I saw a case in Evince where that would be very helpful as well.

Flags: needinfo?(apinheiro)

This bit us again a couple of times trying to fix various problems in mouse review in Orca, and more often than not there's not definite good way of doing that in the client side as was discussed above.
If it really works reliably on Windows with only direct children hit tests, I'd be happy to have access to the implementation of that AccessibleObjectFromPoint() helper function, because neither Joanmarie nor I could get a good, fast and corner-case-free implementation of this with what we currently have.

Anyway, apparently Chromium is now doing what Joanmaire is asking for, that is having atk_component_ref_accessible_at_point() return the deepest child, and it works nicely.

Attached WIP patch does this in Firefox, and mostly works well. It however fails to pass what I believe is the subprocess barrier, that is when called on the toplevel window it'll return an internal frame (right above the web document) instead of a child of the web document itself. But if you manually call it again on the children of that internal frame, it works. I guess that's a limitation of ChildAtPoint() that would have to be lifted some way or another (at worse, the ATK implementation could do the manual walk on children when required). There's also an inline FIXME in my patch because I couldn't easily figure out how to handle it, but I guess there's a way somehow.
Anyway, with this I can get Orca to just work without problems, be in with website menus or whatnot.

This really suggests we should do something about it. I see a few options:

  • Just make Firefox's implementation of atk_component_ref_accessible_at_point() return the deepest descendant, as my patch tries to do.
    • +Easy
    • -Might break caller expectations (although, again, I do not expect any breakage here)
    • -Not completely conforming to the documented semantics
  • Like above, but get AT-SPI/ATK to change the documented semantics
    • +Reasonably easy (have to convince AT-SPI/ATK)
    • -Changing semantics of current API might affect current or older apps (both implementors that would implement the old semantics, and users that would expect the old semantics)
  • Get a new method with the needed semantics in AT-SPI/ATK and implement that one.
    • +Not too hard (have to convince AT-SPI/ATK)
    • +Does not break anything
    • -Have to agree on an API
    • -Depends on newer AT-SPI/ATK

This doesn't change much in the Firefox side of things, it only changes whether replacing the current one, or adding a new one, which is pretty much the same amount of effort.

If we go with a new API, we need to see what it should be for everyone. Having discussed some of these APIs in the past months with GTK developers suggests asking for coordinates is probably not the best idea, because it could cause problems (Wayland doesn't provide everybody with absolute coords, stuck in a 2D coordinate system, etc.), so an API asking for "what's under the mouse pointer" might be better. Something like

/** atk_window_get_object_under_pointer:
 * @window: The #AtkWindow for which look up a child.
 *
 * Gets a reference to the deepest accessible descendant of @window under the
 * mouse pointer, or @window itself if it is under the mouse pointer but none
 * of its children are.
 *
 * Returns: (nullable) (transfer full): The accessible under the mouse pointer,
 *                                      or %NULL.
 */
AtkObject *atk_window_get_object_under_pointer(AtkWindow *window);

Opinions?

Flags: needinfo?(jteh)

Given that you/Joanmarie are happy with this for Orca and that Chromium is doing it too, I think we can just make the change. The ATK spec should probably be fixed too, but I'll let you pursue that (or not) as you choose.

I'll look into fixing this properly. I did some refactoring/fixing in bug 1621521 and bug 1614871 which will make this a lot easier, but I'll need to do a bit of refactoring in the ATK code and some further tweaks to the IPC stuff.

Assignee: nobody → jteh
Flags: needinfo?(jteh)
Blocks: 1622699
No longer blocks: 1622699
Depends on: 1622699

This changes the ATK code to use AccessibleOrProxy::ChildAtPoint, since that already handles walking into OuterDocAccessibles appropriately.
Previously, atk_component_ref_accessible_at_point didn't work at all on OuterDocAccessibles.
Also, for ProxyAccessibles, we no longer adjust the coordinates for ATK_XY_WINDOW in the content process, as this depends on stuff that doesn't exist cross-platform and thus can't be used with AccessibleOrProxy.
Instead, we now handle this in the parent process before making the IPC call.

This wasn't useful cross-platform.
ATK was the only consumer of this and it now uses ProxyAccessible::ChildAtPoint.
This also means the related aNeedsScreenCoords functionality in PDocAccessible::AccessibleAtPoint is no longer needed and has thus been removed.
Finally, this renames PDocAccessible::AccessibleAtPoint to PDocAccessible::ChildAtPoint for consistency with Accessible::ChildAtPoint now that the functionality is mirrored.

Colomban, is there any chance you could test the patches I posted in comment 8 and comment 9? I'm having a lot of frustrating trouble testing this, probably largely due to my unfamiliarity with Linux a11y. I can't even get Firefox to show up in Accerciser at all, and pressing control+alt+a to make it select the focused accessible doesn't seem to work. I have put a fair bit of time into figuring this out, but I have other stuff that needs my attention, so I'm going to need to abandon this soon.

You can choose raw diff in Phabricator to download the patches.
You'll need to go to about:config and set accessibility.xpcom.traverse_outerdoc to true. This won't be needed when this is finalised, but I'm currently blocked on another bug to remove that.

Thanks!

Flags: needinfo?(cwendling)

Sure James, I'll test this and give you feedback.

I'll also try and see if I can help with a working testcase you can easily use, if you have trouble with Accerciser -- which can indeed be tricky sometimes, especially when AT-SPI cache might be outdated (e.g. bgo1623286).

Flags: needinfo?(cwendling)

James: It seems to work pretty well, nicely done!

Actually with the accessibility.xpcom.traverse_outerdoc trick the non-deepest-child-first code seem to be able to give reasonable end results, yet it's highly confusing to the caller that some intermediate answers are not under the requested coordinates, which comforts me in thinking that it's really better to give the deepest child directly. And actually now, with your comments I even succeeded at wrapping my mind around all this and to get a recursive implementation that works without accessibility.xpcom.traverse_outerdoc by iterating over the children of the node I get allowing me to "pass through" the gap, but that starts to become really odd and counter-intuitive.
Anyway, I think we're going in the right direction, and this patch works great from my tests, yay -- but for the accessibility.xpcom.traverse_outerdoc part you mentioned :)

One odd thing is that Firefox's atk_component_ref_accessible_at_point() returns what I believe are text nodes, which are not exposed in the AT-SPI tree, and which are not children of their parent. It's not a problem to include the text nodes, but then they should include a meaningful role (text or static I guess, as it seems to be what Chromium uses), and be children of their parents. Currently I get a situation where the child I get fails the assertion child in child.parent -- which is quite unexpected I believe. Alternatively atk_component_ref_accessible_at_point() could avoid returning those nodes and return their exposed parent instead, which would be just fine as well as it's what we currently have.
Anyway, this is not catastrophic, but it's inconsistent and would be better to fix.

Also, attached is a sample app that requests the child under the mouse from all open Firefox windows, printing it with all its parent chain.

(In reply to Colomban Wendling from comment #12)

James: It seems to work pretty well, nicely done!

Thanks for testing!

Actually with the accessibility.xpcom.traverse_outerdoc trick the non-deepest-child-first code seem to be able to give reasonable end results,

Do you mean taking my patch but doing eDirectChild instead of eDeepestChild?

One odd thing is that Firefox's atk_component_ref_accessible_at_point() returns what I believe are text nodes, which are not exposed in the AT-SPI tree, and which are not children of their parent.

Oops. I can fix that, but I'm surprised we weren't doing this before (e.g. in current Nightly) with direct child. I don't see anything in the previous code that would have prevented that.

Do you mean taking my patch but doing eDirectChild instead of eDeepestChild?

I mean with unpatched Nightly. It's tricky because

  • as it gives the direct child, some intermediate elements are outside the expected region (which again makes sense if we're on the path to the deepest child at coords, but not if we want the direct child containing coords, which was source of a lot of confusion I guess)
  • one has to manually walk the children of the internal frame to "get through" to the web document

e.g. this seem to works with Nightly (and even 68):

def deepestAccessibleAtPoint(acc, x, y, coords=pyatspi.DESKTOP_COORDS):
    # like obj.queryComponent().getAccessibleAtPoint(), but gets the deepest
    # child and handles some oddities

    def recAccessibleAtPoint(obj):
        while obj:
            node = obj.queryComponent().getAccessibleAtPoint(x, y, coords)
            if node == obj:
                return obj
            if node is None:
                # unfortunately Firefox is not able to pass the internal frame
                # barrier (e10s subprocess bridge), so we manually re-try
                # children in this case.  In recent Firefoxes, config key
                # accessibility.xpcom.traverse_outerdoc controls this.
                for child in obj:
                    node = recAccessibleAtPoint(child)
                    if node:
                        return node
                return obj
            obj = node
        return None

    return recAccessibleAtPoint(acc)

It is not so bizarre (but the manual loop on children), but as mentioned above several things might be unexpected, and were at least conflicting with Orca code leading to frustration and failure. I think now I dug deep enough to understand why some results were weird and how, we could use the current implementation -- and I understand how IA2 helper could succeed. But again, as I said above I really think we'll be better off returning the deepest child directly and remove a lot of the oddities.

Oops. I can fix that, but I'm surprised we weren't doing this before (e.g. in current Nightly) with direct child. I don't see anything in the previous code that would have prevented that.

It didn't change, sorry if I was not clear. It's just that I guess nobody noticed because nobody seemed to actually be using the accessible_at_point() API because they didn't understand the results :)
So to clarify: this is not something new at all, but it still seem unexpected.

Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7951281fed61
part 1: Make atk_component_ref_accessible_at_point return the deepest child instead of the direct child. r=yzen
https://hg.mozilla.org/integration/autoland/rev/6ff6a0401a22
part 2: Remove ProxyAccessible::AccessibleAtPoint. r=yzen,nika
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla76
Regressions: 1631276
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: