1739050 - If the focused Accessible is moved, the RemoteAccessible is recreated but focus isn't fired on it (AKA broken Google Search box on Windows + CTW)

Status: RESOLVED FIXED

(Core :: Disability Access APIs, task)

Description

[James Teh [:Jamie]]

STR:

1. Enable ctw.
2. Start NVDA.
3. Open this test case:
   data:text/html,<div id="div" role="presentation" style="height: 1px;"><input autofocus></div><script>setTimeout(() => div.style.overflow = "scroll", 1000);</script>
4. Wait 1 second.
5. Press NVDA+tab to report the focus.
   • Expected: "edit focused has autocomplete"
   • Actual: "unknown defunct"

I've been concerned about this possibility in theory for a while, but only just saw it in practice. This focus problem happens with the Google Search box! I'm not sure of the exact code in that case - it doesn't seem to be a scrollable container springing into existence - but the symptoms are the same, so I believe it to be the same underlying cause.

Here's what happens:

1. When the document is created, the intermediate div doesn't exist in the a11y tree because of role="presentation".
2. When the div is made scrollable, that overrides role="presentation" and the container gets created. After bug 686400, the input does not get recreated, but instead gets moved inside the new container.
3. Our remote implementation doesn't support moves. It works in terms of shows and hides. When it gets the hide, it removes the input from the tree. When it gets the show, it adds the input again, but it's a different RemoteAccessible now.
4. Because it's the same LocalAccessible, we don't fire focus for it, so no focus event is received by RemoteAccessible. (In contrast, if you recreate a LocalAccessible, the event code is smart enough to fire a focus event for the recreated LocalAccessible.)
5. So, the last focus reported to RemoteAccessible clients is dead and focu sis in limbo!

I actually don't understand why this isn't a problem for Mac and Linux even without CTW, since they rely on the RemoteAccessible tree. Maybe they never cache the focus based on an event and always query the focus on demand? Or perhaps it is a problem, but I would have thought someone would have hit this given it happens with the Google Search box.

Since the unique id remains the same (it's based on LocalAccessible), the simple solution here would be to fire a focus event if an id is added to the tree that matches the last focus id.

A more complex solution would be keeping the same RemoteAccessibles. I don't love the fact that we get a new RemoteAccessible subtree here. That defeats the point of bug 686400 somewhat, although this only applies to moves which are probably more rare than other cases where we previously recreated LocalAccessibles. Anyway, there are two possibilities there:

1. Somehow make the remote tree handle moves. This will probably be tricky, since it's currently based on show and hide events and we don't have dedicated move events.
2. In the parent process, when we get a hide, don't destroy the RemoteAccessibles, but instead keep them in limbo for some period (e.g. a refresh tick). If we get a show within that time, we can re-attach the same RemoteAccessibles to the tree in their new places.
   • This is probably easier and less risky (tree fiddling is scary) than the first option.
   • One issue is figuring out where to put the limbo set. We don't have a NotificationController for DocAccessibleParent.
   • The major disadvantage here is that we'd still be sending a full cache push for the moved Accessibles... unless we had a similar limbo set on the LocalAccessible side to determine whether to send a full cache push or not.

Comment 1

[James Teh [:Jamie]]

Eitan, thoughts?

Comment 2

[Eitan Isaacson [:eeejay]]

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

I actually don't understand why this isn't a problem for Mac and Linux even without CTW, since they rely on the RemoteAccessible tree. Maybe they never cache the focus based on an event and always query the focus on demand?

Correct, at least in VoiceOver focused element is retrieved on demand and is not cached.
https://searchfox.org/mozilla-central/source/accessible/mac/mozAccessible.mm#226

Since the unique id remains the same (it's based on LocalAccessible), the simple solution here would be to fire a focus event if an id is added to the tree that matches the last focus id.

Using the new mFocused field? Yeah, I guess that is the easiest solution.

A more complex solution would be keeping the same RemoteAccessibles. I don't love the fact that we get a new RemoteAccessible subtree here. That defeats the point of bug 686400 somewhat, although this only applies to moves which are probably more rare than other cases where we previously recreated LocalAccessibles. Anyway, there are two possibilities there:

1. Somehow make the remote tree handle moves. This will probably be tricky, since it's currently based on show and hide events and we don't have dedicated move events.

I would say that is risky and hard. The show/hide events keep the tree in sync and that can't be taken for granted, I think redoing this will be hard. It relies on coalescing mutations correctly. Adding move semantics means we need to throw all that out, and will need to have a non-event async IPDL API that mirrors the local remove/insert/move calls.

2. In the parent process, when we get a hide, don't destroy the RemoteAccessibles, but instead keep them in limbo for some period (e.g. a refresh tick). If we get a show within that time, we can re-attach the same RemoteAccessibles to the tree in their new places.
   • This is probably easier and less risky (tree fiddling is scary) than the first option.

Agreed. On the other hand, it isn't a bomb-proof way to detect a move vs recreation. So if we do this, it will be to mitigate recreations, not avoid them completely. We will still need to re-fire a focus event in the case where the focused ID goes away and comes back.

- One issue is figuring out where to put the limbo set. We don't have a NotificationController for DocAccessibleParent.

No, but the local outer doc does. I don't think adhering to ticks will give us an accurate and dependable move. The subsequent hide/show IPC messages aren't synchronized with refresh driver ticks - or are they?

- The major disadvantage here is that we'd still be sending a full cache push for the moved Accessibles... unless we had a similar limbo set on the LocalAccessible side to determine whether to send a full cache push or not.

Yes - this would also be a very good reason to avoid recreations altogether.

Summary:
I am now conflicted about the best option. I think the "correct" solution would be to have a message sent with the sum of all mutations in NotificationController::ProcessMutationEvents. This wouldn't be easy, specifically since show/hide events are particular about the state of the tree when they are dispatched not to mention the existence of the target accessible in a hide event.'

On the other hand, the easier approach of not deleting remote accessibles immediately doesn't actually solve this bug because it isn't 100% dependable, and it won't allow us to avoid cache pushes.

Need to think more about how to do that. Maybe a quick workaround is to fire that focus event?

Comment 3

[James Teh [:Jamie]]

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

Since the unique id remains the same (it's based on LocalAccessible), the simple solution here would be to fire a focus event if an id is added to the tree that matches the last focus id.

Using the new mFocused field?

Yes.

One other possibility I just thought of is to keep a RemoteAccessible in limbo indefinitely but only if it matches the mFocus id. That would avoid an unnecessary focus event. I don't love double focus events; they tend to cause unnecessary verbosity/cursor jumping. Although that would mean any client cached ancestry would be invalid, so that's probably a bad idea actually. :(

2. In the parent process, when we get a hide, don't destroy the RemoteAccessibles, but instead keep them in limbo for some period (e.g. a refresh tick). If we get a show within that time, we can re-attach the same RemoteAccessibles to the tree in their new places.

it isn't a bomb-proof way to detect a move vs recreation.

I don't follow. Are you talking about LocalAccessible recreation or RemoteAccessible recreation? LocalAccessible recreation will always recreate the LocalAccessible, so it'll always get a new RemoteAccessible. The only time a limbo RemoteAccessible should get reused is if an Accessible with the same id got hidden and then shown within a short period of time, thus avoiding RemoteAccessible recreation when the LocalAccessible stayed the same.

So if we do this, it will be to mitigate recreations, not avoid them completely. We will still need to re-fire a focus event in the case where the focused ID goes away and comes back.

Why? The focus RemoteAccessible should be in the limbo set until it is shown.

- One issue is figuring out where to put the limbo set. We don't have a NotificationController for DocAccessibleParent.

No, but the local outer doc does. I don't think adhering to ticks will give us an accurate and dependable move. The subsequent hide/show IPC messages aren't synchronized with refresh driver ticks - or are they?

They're synchronised with refresh ticks when they're sent from the content process because we'll fire the hide and show events for the moved LocalAccessible in the same refresh tick. They possibly won't be synchronised with refresh ticks in the parent process depending on when the IPC messages arrive.

We could solve that in one of two ways:

1. In HandleAccEvent, where we send the IPC messages, instead queue them somewhere and have NotificationController push them as one bulk message after ProcessMutationEvents runs. This way, we know when it's safe to throw the limbo set away (when we've finished processing the bulk message).
2. Have another IPC message which we send in NotificationController after ProcessMutationEvents saying "events done". We throw the limbo set away when we receive that message. That's simpler to implement. It also means that big show events don't get bulked together, which might be an advantage in that the parent process could do other stuff between handling the big shows.

- The major disadvantage here is that we'd still be sending a full cache push for the moved Accessibles... unless we had a similar limbo set on the LocalAccessible side to determine whether to send a full cache push or not.

Yes - this would also be a very good reason to avoid recreations altogether.

Right, but that doesn't help moves, which aren't recreations. Or are you just saying that bug 686400 was good for CTW too? :) Or are there other recreations you're worried about?

On the other hand, the easier approach of not deleting remote accessibles immediately doesn't actually solve this bug because it isn't 100% dependable, and it won't allow us to avoid cache pushes.

I think it should be dependable; see above. If we had the limbo set on the LocalAccessible side as well, we could solve the cache push problem, but I'd suggest we don't do that initially.

Comment 4

[James Teh [:Jamie]]

I implemented the RemoteAccessible limbo set idea. It does indeed fix the issue here, including on google.com.

Comment 5

[James Teh [:Jamie]]

Attachment: Bug 1739050: If an Accessible is moved, reuse the RemoteAccessible and don't push the cache for it.

A move is sent to the parent process as a hide and then a show, even if the LocalAccessible wasn't recreated.
This means that a new RemoteAccessible is created and the original RemoteAccessible is destroyed.
This is particularly problematic if the Accessible had focus and the client caches focus.
In that case, the client cache will contain a dead Accessible, and since no focus event is fired (because it's the same Accessible in the content process), focus in the client will be broken.

To fix this, we keep removed Accessibles alive and reuse them if they are shown again.
After the last mutation event is sent from the content process, the content process sends a MutationEventsDone message.
When the parent process receives this, we know that any remaining removals are really removals, not moves, so we destroy them.

Note that this doesn't prevent the content process from unnecessarily sending cache data for moved Accessibles.
That's something we may need to address in future.

Comment 6

[James Teh [:Jamie]]

This causes several test failures. I think most of these are specific to automated testing and not a real problem. In short, things don't go defunct immediately after the reorder event as tests expect because we might not have received the MutationEventsDone message. It should be possible to work around this by doing something similar to untilCacheOk. However, I'm currently investigating another solution which might also prevent sending the cache, so we'll see where that lands first.

Comment 7

[James Teh [:Jamie]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa777271d47284df9192050396c4e099c743ffbb

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/27b558a19262
If an Accessible is moved, reuse the RemoteAccessible and don't push the cache for it. r=eeejay

Comment 9

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/27b558a19262