1933119 - Parent process document doesn't get a11y focus correctly

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

STR (with NVDA):

1. Open a new tab.
2. Ensure the address bar is focused.
3. Type about:support and press enter.
   • Expected: NVDA should start reading the "Troubleshooting Information" document.
   • Actual: NVDA says just "collapsed" or sometimes reads the title of the document and then says "unknown".
4. Press the alt key to open the menu bar, then alt again to dismiss it.
   • Expected: NVDA should say "Troubleshooting Information document"
   • Actual: NVDA says just "frame"

Tabbing around or alt+tabbing out and back in again fixes this, but it's still rather annoying.

5:51.15 INFO: Last good revision: 75b3c6c4437624f59fcaff278bb8f5b88358e3a6 (2023-03-23)
5:51.15 INFO: First bad revision: f476897a6e6ac50ecebdf7f8bda821d40aab066c (2023-03-24)
5:51.15 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=75b3c6c4437624f59fcaff278bb8f5b88358e3a6&tochange=f476897a6e6ac50ecebdf7f8bda821d40aab066c

Comment 1

[James Teh [:Jamie]]

Only obvious focus related change is bug 1823984. Only a11y change is bug 1823294, but that doesn't touch focus.

Comment 2

[James Teh [:Jamie]]

Reverting the patch from bug 1823984 "fixes" this. I don't understand why yet.

Comment 3

[James Teh [:Jamie]]

With the patch reverted (when things work as expected), A11YLOG=focus reports this:

A11Y FOCUS: DOM focus; 39:38.010
  {
    Target: 00000208D54A1800; role: document, name: 'Troubleshooting Information';
    Target node: 0x208d89a8a00, #document, idx in parent <Nothing>
    Document: 00000208D54A1800, document node: 00000208D89A8A00
    Document uri: about:support
  }

A11Y FOCUS: DOM focus; 39:38.010
  {
    Target: 00000208D547E000, window
  }

A11Y FOCUS: process DOM focus; 39:38.016
  {
    Target: 0x208d89a8a00, #document, idx in parent <Nothing>
  }
  {
    A11y target: 00000208D54A1800; role: document, name: 'Troubleshooting Information';
    A11y target node: 0x208d89a8a00, #document, idx in parent <Nothing>
    Document: 00000208D54A1800, document node: 00000208D89A8A00
    Document uri: about:support
  }

A11Y FOCUS: fire focus event; 39:38.017
  {
    Target: 00000208D54A1800; role: document, name: 'Troubleshooting Information';
    Target node: 0x208d89a8a00, #document, idx in parent <Nothing>
    Document: 00000208D54A1800, document node: 00000208D89A8A00
    Document uri: about:support
  }

Without the patch reverted (when things break), a11y never seems to get a DOM focus notification for the about:support document at all.

Comment 4

[James Teh [:Jamie]]

Curiously, this problem only occurs when changing from a remote document to a local (parent process document) in the same tab. If you visit about:support, experience the failure and immediately visit about:about in the same tab, about:about gets focus just fine. Same if you flip those URLs around.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

:emilio, since you are the author of the regressor, bug 1823984, could you take a look?

For more information, please visit BugBot documentation.

Comment 6

[James Teh [:Jamie]]

This seems to be related to the browser element. With this patch against central, the problem goes away:

diff --git a/dom/base/nsFocusManager.cpp b/dom/base/nsFocusManager.cpp
index 54cf5e2647a8f..72dad846cc07d 100644
--- a/dom/base/nsFocusManager.cpp
+++ b/dom/base/nsFocusManager.cpp
@@ -5561,7 +5561,7 @@ bool nsFocusManager::CanSkipFocus(nsIContent* aContent) {
     return false;
   }
 
-  if (mFocusedElement == aContent) {
+  if (mFocusedElement == aContent && !aContent->IsXULElement(nsGkAtoms::browser)) {
     return true;
   }
 

Comment 7

[James Teh [:Jamie]]

1. For a remote document, the <browser> itself has DOM focus in the parent process.
2. When we change from remote to non-remote, we use the same <browser> element.
3. For a non-remote document, the parent process DOM focus needs to be inside the document itself.
4. I haven't worked out where this happens, but when we focus a <browser> for a non-remote document, DOM bounces focus to the document inside it.
5. My guess is that we're relying on (4) to set focus correctly, even though the <browser> already had focus in (1). However, because we're skipping focus for this element because it already had focus, focus doesn't get set up correctly and thus remains on the <browser> element. From a11y's perspective, that effectively means we get no focus change. That's also why NVDA reports "frame" (that's the <browser> element) when a11y forces a focus event (based on the current DOM focus) after dismissing the menu bar.

Comment 8

[James Teh [:Jamie]]

Attachment: Bug 1933119: Add a11y test for focus when changing a tab from remote to non-remote.

Otherwise, when a <browser> has focus and changes from remote to non-remote, we don't redirect focus to the document within.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1933119 - Fix non-remote frame focus after frameloader change too. r=Jamie

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1933119 - Remove no-op focus() calls. r=#tabbrowser-reviewers

Since bug 1823984, this focus() call has been a no-op on remote frames.

The previous patch also fixes the non-remote frame focus switch.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1933119 - Minor nsFocusManager clean-ups. r=hsivonen,smaug,#dom-core

No behavior change.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4162f8eaef34
Fix non-remote frame focus after frameloader change too. r=Jamie,smaug

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/90929265e20e
Remove no-op focus() calls. r=tabbrowser-reviewers,dao

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1823984

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/931259ba116a
Fix some junit tests.

Comment 16

[amarc]

Backed out changeset 90929265e20e for causing bc failures @ browser_keyword

• Backout link
• Push with failures
• Failure Log
• Failure line:

TEST-UNEXPECTED-FAIL | browser/components/urlbar/tests/browser/browser_keyword.js | Test timed out -

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ca977cd1cc39
Minor nsFocusManager clean-ups. r=hsivonen,dom-core,edgar

Comment 18

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/4162f8eaef34
https://hg.mozilla.org/mozilla-central/rev/931259ba116a
https://hg.mozilla.org/mozilla-central/rev/ca977cd1cc39

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0229729dd998
Add a11y test for focus when changing a tab from remote to non-remote. r=emilio,nlapre

Comment 20

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/0229729dd998

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Moved the other patch to bug 1933769.

Comment 22

[Phabricator Automation]

Comment on attachment 9439684 [details]
Bug 1933119 - Remove no-op focus() calls. r=#tabbrowser-reviewers

Revision D230098 was moved to bug 1933769. Setting attachment 9439684 [details] to obsolete.

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:emilio, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox134 to wontfix.

For more information, please visit BugBot documentation.

Comment 24

[Emilio Cobos Álvarez (:emilio)]

301 Jamie, but my guess is that this can probably ride the trains :)

Comment 25

[James Teh [:Jamie]]

I'd let it ride the trains. It's an annoying bug, but it's existed for over a year apparently without anyone other than me noticing.