1421144 - Focus loss with NVDA after dismissing system menu with e10s (AKA accFocus broken on root accessible)

Assignee

Description

•

7 years ago

STR:
1. Start NVDA and Firefox.
2. Visit any website; e.g. www.mozilla.org.
3. Confirm that NVDA reads the document when you navigate with the cursor keys (via NVDA browse mode).
4. Press alt+space to open the System menu (with Minimize, Maximize, etc.).
5. Press escape twice to dismiss it.
6. Try to read the document with the arrow keys as in step 3.
Expected: You should be able to read the document.
Actual: NVDA reports nothing.

When you dismiss the System menu, focus ends up in limbo. According to NVDA, it's on the root accessible, which is wrong.

This is because IAccessible::accFocus doesn't work on the root accessible when a remote document has focus. This works just fine if you are viewing a local document; e.g. about:support.

Comment hidden (mozreview-request)

The base implementation of accFocus can't handle the case when a remote document has focus and just returns no focus (VT_EMPTY).
Override accFocus on the root accessible to try the accessible for the remote document in the active tab in this case.
This fixes focus loss with NVDA when dismissing the System menu.

Review commit: https://reviewboard.mozilla.org/r/203342/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203342/

James Teh [:Jamie]

Assignee

Comment 2

•

7 years ago

Testing performed:
1. Confirmed the STR in comment 0 work as expected for both remote and local docs when the document itself has focus.
2. Confirmed that focus is restored to the right accessible (via NVDA+tab) after dismissing the System menu when a descendant of the document has focus. Did this for both local and remote docs.
3. Confirmed that accFocus returns VT_EMPTY when a local document is the active tab but the Firefox window is not active (as it did previously).
4. Confirmed that accFocus returns VT_EMPTY when a remote document is the active tab but the Firefox window is not active.

alexander :surkov (:asurkov)

Comment 3

•

7 years ago

Jamie, why root accessible is special? Don't we expect to have get_accFocus working on any element from the main process, when the focus is inside of a remote content?

James Teh [:Jamie]

Assignee

Comment 4

•

7 years ago

(In reply to alexander :surkov from comment #3)
> Jamie, why root accessible is special? Don't we expect to have get_accFocus
> working on any element from the main process, when the focus is inside of a
> remote content?

Generally, you only call accFocus when you're trying to find the focus. You start at the focused HWND and keep calling accFocus from there until you reach the deepest point. Since Gecko only has one HWND, the only place we really need to support this is the root accessible.

If we want to be super strict about the spec, we need to support it on all objects, but only if the focus is a descendant of that object. Even for local documents, we don't do this at present. accFocus will answer for a direct child, but, for example, if you ask the grandparent of about:support for accFocus, it will return VT_EMPTY.

Personally, I don't think the extra complexity is worthwhile, especially as no client is probably going to use it. Also, we'd have to work out whether the document is really a descendant of the object in question, and I assume that involves tree walking.

alexander :surkov (:asurkov)

Comment 5

•

7 years ago

I buy the argument "no one needs", but if we followed the spec/guidelines/practices, and it wouldn't cost us much, then it would be my preference anyways. If you implemented accFocus on generic accessible, then it wouldn't involve any extra traversing I believe. So would this approach do a trick?

AccessibleWrap::get_accFocus(
{
...
Accessible* focusedAccessible = FocusedChild();
if (!focusedAccessible) {
  ProxyAccessible* docProxy = GetPrimaryRemoteTopLevelContentDoc();
  if (docProxy) {
    Accessible* docAcc = WrapperFor(docProxy);
    if (docAcc) {
      focusedAccessible = docAcc->FocusedChild();
    }
  } 
}

James Teh [:Jamie]

Assignee

Comment 6

•

7 years ago

(In reply to alexander :surkov from comment #5)
> it wouldn't involve any extra traversing I believe. So would this approach
> do a trick?
...
> Accessible* focusedAccessible = FocusedChild();
> if (!focusedAccessible) {
>   ProxyAccessible* docProxy = GetPrimaryRemoteTopLevelContentDoc();
...
>       focusedAccessible = docAcc->FocusedChild();

The problem with this is that it will cause the document's focus to be returned even if it isn't a descendant of the accessible on which it is called. For example, if you called it on the Address bar while a content document had focus, it would return the content document. This is a spec violation, since accFocus is only supposed to return a child:

> VT_DISPATCH
> pdispVal member is the address of the IDispatch interface for the child object that has the keyboard focus.
(https://msdn.microsoft.com/en-us/library/windows/desktop/dd318479(v=vs.85).aspx)

As far as I know, the only way to get around this would be to check for common ancestors.

Also, I don't think you can get the focused child from a content document in this way. As far as I know, on windows, you can only get a native IAccessible from a remote document itself, not any of its descendants. To get a descendant, you have to make native COM calls to the document's IAccessible.

alexander :surkov (:asurkov)

Comment 7

•

7 years ago

(In reply to James Teh [:Jamie] from comment #6)
> (In reply to alexander :surkov from comment #5)
> > it wouldn't involve any extra traversing I believe. So would this approach
> > do a trick?
> ...
> > Accessible* focusedAccessible = FocusedChild();
> > if (!focusedAccessible) {
> >   ProxyAccessible* docProxy = GetPrimaryRemoteTopLevelContentDoc();
> ...
> >       focusedAccessible = docAcc->FocusedChild();
> 
> The problem with this is that it will cause the document's focus to be
> returned even if it isn't a descendant of the accessible on which it is
> called. For example, if you called it on the Address bar while a content
> document had focus, it would return the content document. This is a spec
> violation, since accFocus is only supposed to return a child:

fair enough. Could you file a follow up on it to obey the spec and also consider the approach I suggested above with a tiny change of 
if (!focusedAccessible && IsRootAcc()) {
with a good comment what we do there.

alexander :surkov (:asurkov)

Comment 8

•

7 years ago

mozreview-review

Comment on attachment 8932317 [details]
Bug 1421144: Fix IAccessible::accFocus on the root accessible for remote content.

https://reviewboard.mozilla.org/r/203342/#review210594

r=me with comments fixed/addressed.

::: accessible/windows/msaa/RootAccessibleWrap.cpp:171
(Diff revision 1)
> +  // Get the document in the active tab.
> +  ProxyAccessible* docProxy = GetPrimaryRemoteTopLevelContentDoc();
> +  if (!docProxy) {
> +    return hr;
> +  }
> +  Accessible* docAcc = WrapperFor(docProxy);

can't you just static_cast to AccessibleWrap* and then call get_accFocus right on it?

::: accessible/windows/msaa/RootAccessibleWrap.cpp:186
(Diff revision 1)
> +  MOZ_ASSERT(SUCCEEDED(hr));
> +  MOZ_ASSERT(docIa);
> +
> +  // Ask this document for its focused descendant.
> +  VARIANT docChild;
> +  hr = docIa->get_accFocus(&docChild);

why you can't simply return get_accFocus(pvarChild)?

Attachment #8932317 - Flags: review?(surkov.alexander) → review+

alexander :surkov (:asurkov)

Comment 9

•

7 years ago

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

> Also, I don't think you can get the focused child from a content document in
> this way. As far as I know, on windows, you can only get a native
> IAccessible from a remote document itself, not any of its descendants. To
> get a descendant, you have to make native COM calls to the document's
> IAccessible.

I see, it's creepy :) It probably also invalidates the first question in comment #8

James Teh [:Jamie]

Assignee

Comment 10

•

7 years ago

(In reply to alexander :surkov from comment #7)
> Could you file a follow up on it to obey the spec and also
> consider the approach I suggested above with a tiny change of 
> if (!focusedAccessible && IsRootAcc()) {
> with a good comment what we do there.

Do you mean you want me to move the implementation in this bug to AccessibleWrap::get_accFocus and check IsRoot, rather than putting it in RootAccessibleWrap? Is there an advantage to that as compared with overriding in RootAccessibleWrap?

James Teh [:Jamie]

Assignee

Comment 11

•

7 years ago

mozreview-review-reply

Comment on attachment 8932317 [details]
Bug 1421144: Fix IAccessible::accFocus on the root accessible for remote content.

https://reviewboard.mozilla.org/r/203342/#review210594

> can't you just static_cast to AccessibleWrap* and then call get_accFocus right on it?

ProxyAccessible doesn't inherit from AccessibleWrap; you have to create a wrapper with WrapperFor.
Even once you do create a wrapper, that wrapper isn't a normal Gecko AccessibleWrap as far as I know. On Windows, the only thing you can do with it is get a native interface from it. Platforms other than Windows talk to the ProxyAccessible (instead of the AccessibleWrap) to get what they need.

> why you can't simply return get_accFocus(pvarChild)?

The document's accFocus might answer CHILDID_SELF, which means the document itself has focus. However, the original call was not on the document; it was on the root. So, we need to convert CHILDID_SELF to the document accessible in this case.

alexander :surkov (:asurkov)

Comment 12

•

7 years ago

(In reply to James Teh [:Jamie] from comment #10)
> (In reply to alexander :surkov from comment #7)
> > Could you file a follow up on it to obey the spec and also
> > consider the approach I suggested above with a tiny change of 
> > if (!focusedAccessible && IsRootAcc()) {
> > with a good comment what we do there.
> 
> Do you mean you want me to move the implementation in this bug to
> AccessibleWrap::get_accFocus and check IsRoot, rather than putting it in
> RootAccessibleWrap? Is there an advantage to that as compared with
> overriding in RootAccessibleWrap?

I hoped to share some common code. It's not that easy though as I supposed.

(In reply to James Teh [:Jamie] from comment #11)
> Comment on attachment 8932317 [details]
> Bug 1421144: Fix IAccessible::accFocus on the root accessible for remote
> content.
> 
> https://reviewboard.mozilla.org/r/203342/#review210594
> 
> > can't you just static_cast to AccessibleWrap* and then call get_accFocus right on it?
> 
> ProxyAccessible doesn't inherit from AccessibleWrap; you have to create a
> wrapper with WrapperFor.

right, I was talking about accessible object returned by WrapperFor()

> Even once you do create a wrapper, that wrapper isn't a normal Gecko
> AccessibleWrap as far as I know. On Windows, the only thing you can do with
> it is get a native interface from it. Platforms other than Windows talk to
> the ProxyAccessible (instead of the AccessibleWrap) to get what they need.

I take your word on that. It feels knotty.

> > why you can't simply return get_accFocus(pvarChild)?
> 
> The document's accFocus might answer CHILDID_SELF, which means the document
> itself has focus. However, the original call was not on the document; it was
> on the root. So, we need to convert CHILDID_SELF to the document accessible
> in this case.

I see, a small comment provided would make this part clearer. Anyway, you could save one 'if' if you used 'pvarChild' instead 'docChild', right?

Comment hidden (mozreview-request)

Comment on attachment 8932317 [details]
Bug 1421144: Fix IAccessible::accFocus on the root accessible for remote content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203342/diff/1-2/

James Teh [:Jamie]

Assignee

Comment 14

•

7 years ago

(In reply to alexander :surkov from comment #12)
> > Do you mean you want me to move the implementation in this bug to
> > AccessibleWrap::get_accFocus and check IsRoot, rather than putting it in
> > RootAccessibleWrap? Is there an advantage to that as compared with
> > overriding in RootAccessibleWrap?
> I hoped to share some common code. It's not that easy though as I supposed.

Pretty much all of the duplication is avoided by calling the superclass first. All the rest is going to be different either way. More on that below.

> > Even once you do create a wrapper, that wrapper isn't a normal Gecko
> > AccessibleWrap as far as I know. On Windows, the only thing you can do with
> > it is get a native interface from it. Platforms other than Windows talk to
> > the ProxyAccessible (instead of the AccessibleWrap) to get what they need.
> I take your word on that. It feels knotty.

The reason is that Gecko's IPC stuff can't serialise the whole of the AccessibleWrap class; too many of its methods require stuff that doesn't get serialised. So, we have ProxyAccessible, which is purely for IPC. On Windows, we only really use the proxy to get a COM proxy and then we let COM handle the IPC. On Linux/Mac, the ProxyAccessible calls methods in content.

> > > why you can't simply return get_accFocus(pvarChild)?
> > The document's accFocus might answer CHILDID_SELF, which means the document
> > itself has focus. ...
> I see, a small comment provided would make this part clearer.

Done.

> Anyway, you
> could save one 'if' if you used 'pvarChild' instead 'docChild', right?

You're right. That also allowed me to clean up the other if block as well.

Patch amended with these changes, re-tested (as per comment 2) and updated on Mozreview.

Pulsebot

Comment 15

•

7 years ago

Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d856b4067e80
Fix IAccessible::accFocus on the root accessible for remote content. r=surkov

alexander :surkov (:asurkov)

Comment 16

•

7 years ago

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

> The reason is that Gecko's IPC stuff can't serialise the whole of the
> AccessibleWrap class; too many of its methods require stuff that doesn't get
> serialised. So, we have ProxyAccessible, which is purely for IPC. On
> Windows, we only really use the proxy to get a COM proxy and then we let COM
> handle the IPC. On Linux/Mac, the ProxyAccessible calls methods in content.

we probably should have a helper method for that, it has to be a common pattern then.

> > Anyway, you
> > could save one 'if' if you used 'pvarChild' instead 'docChild', right?
> 
> You're right. That also allowed me to clean up the other if block as well.
> 
> Patch amended with these changes, re-tested (as per comment 2) and updated
> on Mozreview.

thank you

Cosmin Sabou [:CosminS]

Comment 17

•

7 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/d856b4067e80

Status: NEW → RESOLVED

Closed: 7 years ago

status-firefox59: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla59

Ryan VanderMeulen [:RyanVM]

Comment 18

•

6 years ago

backout

This was reverted from Fx59 to work around the crashes in bug 1424505. It remains fixed for Fx60 and newer.
https://hg.mozilla.org/releases/mozilla-release/rev/f1d078c9252a05bf96eb71451838b69132c07993

status-firefox59: fixed → disabled

status-firefox60: --- → fixed

status-firefox61: --- → fixed

Ryan VanderMeulen [:RyanVM]

Comment 19

•

6 years ago

backout

This has been reverted from Fx60 as well for the same reason. It remains fixed for Fx61.
https://hg.mozilla.org/releases/mozilla-beta/rev/7d6400c2038cf35259e42d30541654a94e7a7b9a

status-firefox60: fixed → disabled

James Teh [:Jamie]

Assignee

Updated

•

2 years ago

Regressions: 1783098