1779156 - [CTW] Tests that wait for a show event based on a cached property time out

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

We have several tests that wait for show events using DOM node id as the match criteria. For example:

let onShow = waitForEvent(Ci.nsIAccessibleEvent.EVENT_SHOW, "div");

With CTW enabled, this times out because the cache isn't sent until after the show event is fired, so the criteria can't match at the time the event is handled. This will also apply to any other cached property.

We could probably hack around this in the tests or the harness. However, this really does seem like a broader problem as I previously noted in bug 1748755 comment 5. It's not unreasonable that some client might get a show event and expect to be able to query data about the target. This is particularly true on Windows with synchronous in-process events, but even with async OOP events, there's a slight chance that a client call could be handled between the two IPDL calls.

Possible solutions:

1. Include the cache data as a parameter to PDocAccessible::ShowEvent. The disadvantage here is that this makes that single IPDL call potentially very bulky with no chance for other things to run while it's happening.
2. For new (not moved) Accessibles, instead of firing the event immediately in RecvShowEvent, we can add it to a set of pending show events. When the cache push is received for that Accessible in RecvCache, we can fire the event then.

Eitan, what do you think?

Comment 1

[Eitan Isaacson [:eeejay]]

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

2. For new (not moved) Accessibles, instead of firing the event immediately in RecvShowEvent, we can add it to a set of pending show events. When the cache push is received for that Accessible in RecvCache, we can fire the event then.

I get nervous storing pending events because we might get to a point where we can't be sure if they are vacated and dispatched (ie. can we always guarantee a successive cache push).

A third option could be having a flag in SendCache that would fire a show event with the first element in its list being the target.

The problem with all of these solutions is that they can hurt what I assume would be a perceived performance gain of firing a show event as early as possible. But like I said that is an assumption maybe not worth fighting for. If this hurts Windows already maybe we need to bite the bullet and delay the show event.

Comment 2

[James Teh [:Jamie]]

(In reply to Eitan Isaacson [:eeejay] from comment #1)

I get nervous storing pending events because we might get to a point where we can't be sure if they are vacated and dispatched (ie. can we always guarantee a successive cache push).

I think we can, but only because we know that's the only possible code path right now. I can understand why that feels fragile.

A third option could be having a flag in SendCache that would fire a show event with the first element in its list being the target.

From a client perspective, that could have the same problem as option 2; i.e. we might never deliver a show event if there wasn't a successive cache push? It does fix the problem of buffered events never being cleared though. I think it's cleaner too.

The problem with all of these solutions is that they can hurt what I assume would be a perceived performance gain of firing a show event as early as possible.

With the exception of live regions and tooltips, I don't think show events are really used to report anything to the user, as least not in clients I'm aware of. However, even if this were a perceived performance problem, firing the show event early doesn't solve it because the client will then get back null data from everything it tries to query. We might fire the event earlier, but the event is effectively useless.

If this hurts Windows already maybe we need to bite the bullet and delay the show event.

FWIW, I haven't clearly identified this being a problem for Windows in the wild. Right now, I'm more concerned about our tests, which definitely break because of this.

Comment 3

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1779156 - Defer show event until after the cache is populated. r?Jamie

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ff096ad2e95
Defer show event until after the cache is populated. r=Jamie

Comment 5

[Marian-Vasile Laza]

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

Comment 6

[James Teh [:Jamie]]

Attachment: Bug 1779156 part 1: Include the cache in PDocAccessible::ShowEvent.

Previously, the cache was sent in a separate IPDL call.
Even though we delayed the show event, there was a chance that a client would walk the subtree and find a lot of Accessibles with no data.
Because no event is fired for those changes inside the subtree, the client might not know it needs to update them.
This resulted in missing information in NVDA browse mode in some cases.
Now, the tree is always in sync with its cached data.

Comment 7

[James Teh [:Jamie]]

Attachment: Bug 1779156 part 2: Remove the aDispatchShowEvent argument to PDocAccessible::Cache.

This is no longer used.

Comment 8

[Phabricator Automation]

Comment on attachment 9319941 [details]
Bug 1779156 part 1: Include the cache in PDocAccessible::ShowEvent.

Revision D171046 was moved to bug 1818726. Setting attachment 9319941 [details] to obsolete.

Comment 9

[Phabricator Automation]

Comment on attachment 9319942 [details]
Bug 1779156 part 2: Remove the aDispatchShowEvent argument to PDocAccessible::Cache.

Revision D171047 was moved to bug 1818726. Setting attachment 9319942 [details] to obsolete.