Closed Bug 1623286 Opened 5 years ago Closed 5 years ago

Missing accessible state-changed events switching active documents (AKA broken mouse review in Orca when switching tabs)

Categories

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

Product:
Core
Component:
Disability Access APIs
Platform:
Unspecified
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
86 Branch
Tracking Flags:
Tracking Status
firefox86 --- fixed

People

(Reporter: cwendling, Assigned: Jamie)

References

Details

Attachments

(3 files, 1 obsolete file)

atspi-cache-diff.py
4.23 KB, text/x-python
Details
ff-cache-diff-report
23.93 KB, text/plain
Details
Bug 1623286: Fire offscreen state change a11y events when a XUL tab panel is switched.
48 bytes, text/x-phabricator-request
Details | Review
Attached file atspi-cache-diff.py —

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

  1. Open two tabs on different websites
  2. Run the attached event listener with python3 atspi-cache-diff.py firefox
  3. Switch tabs (e.g. click on the non-active one)

Actual results:

The listener will spit a lot of info about AT-SPI cache issues. Those come from the AT-SPI state cache being outdated, because Firefox didn't emit the corresponding events (here, object:state-changed ones) that would have notified and triggered a cache update.

This goes beyond the cache issue, as the events are simply missing, preventing a client from getting notified they happened. In practice, the worse part is the active document not getting its active state notified, but all of those matter.

Expected results:

No cache issues should have been detected. Given proper events from the app, the cache gets updated automatically.

Attached file ff-cache-diff-report (obsolete) —

Example output I got switching from https://www.wikipedia.org/ to https://gitlab.gnome.org/GNOME/orca/issues/68

This is a somewhat wider problem as well, although less relevant: when moving the focus inside a page using <kbd>Tab</kbd>, the previously selected element doesn't receive an object:state-changed-focused(0, 0, 0) event notifying it lost its focused state. It's not as problematic as the event is there when gaining focus, and usually that's enough, but it still gives an incorrect view of the app state that could be misleading.

OS: Unspecified → Linux

I'm reporting this in the context of Orca mouse review feature that fails after switching tabs, because of incorrect data it is given, see Orca issue #70 (esp near the end and down).

Jamie, it looks like more events are missing from proxyAccessibles on Linux. From looking at the code, we don't seem to fire a general EVENT_STATE_CHANGE on proxies. We only fire it selectively when focus changes, menus appear or disappear, and some other changes, which definitely doesn't seem enough.

Flags: needinfo?(jteh)

Thanks for providing details on the user impact in comment 3. It makes it much easier to prioritise the issue.

Flags: needinfo?(jteh)
Priority: -- → P2
See Also: → https://gitlab.gnome.org/GNOME/orca/issues/70
Summary: Missing accessible state-changed events switching active documents → Missing accessible state-changed events switching active documents (AKA broken mouse review in Orca when switching tabs)

(In reply to Marco Zehe (:MarcoZ) from comment #4)

Jamie, it looks like more events are missing from proxyAccessibles on Linux. From looking at the code, we don't seem to fire a general EVENT_STATE_CHANGE on proxies. We only fire it selectively when focus changes, menus appear or disappear, and some other changes, which definitely doesn't seem enough.

I assume you're looking at a11y::ProxyEvent. That only deals with cases where Gecko doesn't fire a state change but ATK expects a state change; e.g. focus, menus appearing, etc. Actual state change events are handled in ProxyStateChangeEvent, which does get called for state changes.

My guess is that there is no Gecko event which is fired for active state changes, since we haven't needed it before now. I need to think about whether we should deal with this in the core layer or the ATK layer.

Colomban, can you clarify the expectations regarding ATK_STATE_ACTIVE on documents? Does Orca expect that this state is set on the active tab document?

The fact that it's being set on some tab documents right now actually looks like an unintentional (but happy) oversight in Gecko. For example, if you type about:support in the address bar, that document doesn't get the active state as far as I can tell. I'm guessing that breaks mouse review?

Flags: needinfo?(cwendling)

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

The fact that it's being set on some tab documents right now actually looks like an unintentional (but happy) oversight in Gecko.

See bug 566542, which only talks about ATK_STATE_ACTIVE on the root frame. When we switched to e10s, this code unintentionally started applying to remote tab documents as well.

So, as well as firing the right event, we also need to make sure we're exposing ATK_STATE_ACTIVE on all tab documents correctly.

Attached file ff-cache-diff-report —

Actually, active state on the web document is not as important as my initial report suggested; it was what I researched to try and fix things in Orca by properly detecting document changes and (h)ac(k)ting there; but the root cause that confuses Orca is actually the showing states not being cleared on non-showing objects, which leads to Orca selecting wrong elements, as it believes they are showing.

So if the active state is actually an accident here, you can simply get rid of it if you'd like, and "simply" fix all the showing ones not being notified. Basically the point of this report is that for AT-SPI, there always should be an event notifying when a state changed; not only does it allow the clients to know, but also the internal cache to get updated.

For the Orca issue at hand when switching documents, what matter most are the elements up to the web document, so that it has a clue up to there, and then Orca actually has a sad hack that can take over¹ (which we could remove if all events were properly fired though).

I see that the report I attached doesn't contain anything above the web document itself, not sure how I got lucky that time though -- but maybe that suggests part of this is an intermittent issue? Attached is a new one taken the same way (well, switching in different order, but that's all), that show missing things above the document web.

¹ Orca is clearing the cache for objects that don't report the showing state but for which Orca has reasonable reasons to believe they possibly should, to work around just this. It slows things down, so it's not reasonable to do this every time and just ignore the cache. Up to now Orca was lucky enough that not so many of such hacks had to be used, but any we can remove would wonderful. And no, clearing the cache for objects that report the state where they shouldn't is not really an option as it would mean we'd always clear it. Basically clearing the cache is not a viable option in any case.

Attachment #9134088 - Attachment is obsolete: true
Flags: needinfo?(cwendling)

So I totally understand that we're not firing the right events here and that this means the cache isn't updated. However, if we were to fire events for every object which changes its showing state, I really worry about performance. For example, when you switch tabs, the document loses its showing state, but so does all of its descendants. If we fired an event for every descendant, that would be a huge number of events. Similarly, when you scroll the page, many nodes change their showing state. Aside from the cost of Gecko calculating this information for every accessible on every change, there's the cost of firing the events (and ATK/Orca processing those events).

We should certainly fix things here so that the state change is correctly fired for documents. A generic fix for all showing states is more controversial/risky/complex.

I hear you about performance with all the document's objects involved. I'm not sure what is the right solution here indeed… Joanie, do you know what e.g. Chromium does in this area? Or what you'd hope/expect apps would do?

Flags: needinfo?(jdiggs)

As I stated in the original bug filed against Orca, I've not been able to reproduce the problem and thus debugging things or stating why the problem doesn't happen in Chromium is hard. If you could find a way to help me reproduce the problem then I could take a look and answer the question more accurately.

That said I agree with Jamie. We most definitely do not want event spam.

Flags: needinfo?(jdiggs)

OK, so let's forget about what's inside the web document itself, as anyway currently Orca has a working hack there, and with bug1598299 now fixed it's even easier to handle this part.

But we need a fix for the UI part, because for now the only "solution" I have is clearing the cache on a bunch of objects because there's no better heuristic I could find. The UI part should not be a problem performance-wise, and it would be a huge win for Orca and the user experience on Firefox (faster and more reliable tab switch).

What do you mean by "UI part"? There are still a lot of objects in the UI. We don't want to be unconditionally firing offscreen state changes for every single object, even if that were possible (which it probably isn't). The same argument as comment 10 applies to the UI.

Are there particular events you're missing in the UI that are more important than others?

Flags: needinfo?(cwendling)

OK, so let's be specific for the problem I want to solve now I (believe) have narrowed it as much as I could: I need the state showing up-to-date on what's in bold, while it currently isn't:

  • frame (the window)
    • panel
      • scroll pane (i.e. not showing)
        • internal frame
          • web document
      • scroll pane
        • internal frame
          • web document (currently viewed)

You can also see it another way: I need a dive down from the frame checking what has state showing to reach the currently active document, and not any other document.

So when a document becomes active, I need its parents (internal frame and scroll pane) to become showing, and I need the previously active document's scroll pane not to be showing anymore.

PS: In practice I need both active and showing to be there for what's relevant, but from what I see all nodes I mentioned are active and in this case we can pretend we only need the active state to be present on active elements, rather than also not being there for inactive elements.

Flags: needinfo?(cwendling)

There are still a lot of objects in the UI. We don't want to be unconditionally firing offscreen state changes for every single object, even if that were possible (which it probably isn't). The same argument as comment 10 applies to the UI.

More generally that my previous comment which only focuses on the actual issue I'm having right now, if the event for state showing could be fired everywhere it's not a terrible performance issue would most likely prevent the kind of trouble I'm having right now. As I don't believe any offscreen objects should be showing, it should have little impact, unless a whole bunch of elements appear or disappear. In such cases, maybe for the part that is disappearing it could be mitigated by only updating the topmost element that changed state (e.g. if it changed its state but its parent didn't). That wouldn't really help for the parts that become visible, but it's still half a mitigation, and to think of a better idea I'd probably need a real example.

Thanks for the details.

(In reply to Colomban Wendling from comment #15)

You can also see it another way: I need a dive down from the frame checking what has state showing to reach the currently active document, and not any other document.

At risk of getting off-topic, could you use the ATK_RELATION_EMBEDS relation on the frame instead? That should directly give you the active tab document if that's what you want, no tree walking required.

(In reply to Colomban Wendling from comment #16)

More generally that my previous comment which only focuses on the actual issue I'm having right now, if the event for state showing could be fired everywhere it's not a terrible performance issue would most likely prevent the kind of trouble I'm having right now. As I don't believe any offscreen objects should be showing, it should have little impact, unless a whole bunch of elements appear or disappear.

There are quite a few ways an object can go on/off screen. I'd need to dig into it more, but I think the cases you've listed have pretty specific code paths. Whether we get notifications for them is an open question.

At risk of getting off-topic, could you use the ATK_RELATION_EMBEDS relation on the frame instead? That should directly give you the active tab document if that's what you want, no tree walking required.

Orca does use that to get the active document, but it's a little more complicated than that here. I'll leave to Joanie to answer on the details if she wants because she probably knows them a lot better than I do, but the code to get the object under the mouse for "regular" UI (e.g. not web content) partly relies on tree walking and performing several checks along the way. It's like that for all UIs, including Firefox's, Chrome's, GTK, etc., and that takes care of several oddities including menus and alike.

A quick test trying to get rid of this for Firefox shows me that at least the Firefox hamburger menu doesn't get considered by a ref_accessible_at_point() (which is probably a bug, but it doesn't matter much for Orca ATM), but I believe it takes care of more than this kind of things.

So based on my current understanding of the problem, would this issue be fixed if there was a single accessibility event, namely object:state-changed:showing set to false on documents which are no longer active? Then Orca would not descend the document which claims to not be showing.

I don't think that would work, because Orca would find the internal objects (scroll pane + internal frame) and from there would get "stuck" in that path: it would believe the internal frame is the interesting element.

As mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1623286#c15, what we need is correct states on at least the top-most element (the scroll pane) not to dive into a trap. We could probably get away with only having that scroll pane up-to-date, and let Orca's hack of "clearing the cache when something is not showing yet probably should be" handle the actually active tab's elements not being up-to-date.

Ok, then maybe both the scroll pane and the document could have the showing states updated? I don't see how that would hurt (i.e. in terms of performance, event floods, etc.). And it would make the document's state set more correct, right?

Yes. The only thing I'm not sure about from the top of my head is whether the Orca hack for clearing the cache on erroneously non-showing things is triggered outside of a web document, so maybe we would need the intermediate internal frame also updated.

And anyway I agree that even updating all these should be pretty harmless: it should be at most 4 more events per tab switch:

  • object:state-changed:showing on the previously visible scroll pane
  • object:state-changed:showing on the now visible scroll pane
  • object:state-changed:showing on the now visible internal frame
  • object:state-changed:showing on the now visible web document (that one I might already be OK, I'd have to re-check to be sure)

Any news on this?

(In reply to Colomban Wendling from comment #23)

Any news on this?

Jamie, without a fix here, the current situation is really annoying for the end-user, I need to restart Firefox a lot to read a new tab with the mouse review feature of Orca, which is really painful for the user experience.

Without this fix, we'd be forced to apply a ugly hack into Orca which will clear the tab cache and could lead to poor performance on less powerful computer.

If it's impossible for you to provide a patch in the near future, could you please just indicate it here? Without that, the Orca maintainer waits your fix to progress.

Thanks in advance and happy new year.

Flags: needinfo?(jteh)

Without this, the AT-SPI cache isn't updated and Orca mouse review fails after switching tabs.

Really sorry for the delay.

I've just pushed a patch which fires state change events for the previous and current scroll panes when switching tabs. It does not fire events for the internal frame or document, though. The internal frame does get the correct states; it just doesn't fire events.

I'm curious as to whether this helps. I realise this is still buggy, but if it's sufficient for Orca, it's a good start and I'd like to get it landed. Fixing the problem for the internal frame and/or document are both entirely different code paths and I'm not sure where to start with those at this stage.

What do you need to test these? The patch is attached to this bug and there's also a try run which will produce Linux builds when done:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0f1aa6b295e963a4a16ce410cedbb980693d7bc

Flags: needinfo?(jteh)

What do you need to test these? The patch is attached to this bug and there's also a try run which will produce Linux builds when done:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0f1aa6b295e963a4a16ce410cedbb980693d7bc

Could you please guide me to find how to download the build or paste me a wiki for that? I'm completely lost with the treeherder interface.

Yeah, sorry, Treeherder is ridiculously confusing if you aren't familiar with it. Here's a direct link to the build (I couldn't provide this before I left yesterday because it hadn't finished yet, but it has now):
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/GhH-kG6hTvulvIkkSo5GoQ/runs/0/artifacts/public/build/target.tar.bz2

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

Yeah, sorry, Treeherder is ridiculously confusing if you aren't familiar with it. Here's a direct link to the build (I couldn't provide this before I left yesterday because it hadn't finished yet, but it has now):
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/GhH-kG6hTvulvIkkSo5GoQ/runs/0/artifacts/public/build/target.tar.bz2

Many thanks for the direct link.

So I confirm the issue is fixed with your patch, I'm able to switch from one tab to another and mouse review still works, really awesome. To be sure it was your version figuring out the issue, I proceed to the same test scenario on Firefox Nightly without your patch and I confirm I have still the issue.

Assignee: nobody → jteh

(In reply to Alex ARNAUD from comment #29)

So I confirm the issue is fixed with your patch, I'm able to switch from one tab to another and mouse review still works, really awesome.

Great! Thanks for your help... and thanks to both Joanie and Colomban for their help, too. I'll proceed with getting this reviewed and landed.

Pushed by jteh@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5b3447ef3f0b Fire offscreen state change a11y events when a XUL tab panel is switched. r=eeejay

https://hg.mozilla.org/mozilla-central/rev/5b3447ef3f0b

Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
status-firefox86: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch
See Also: → 1763195
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: