Send AX focus changed events for modal dialog open/close

Send AX focus changed events for modal dialog open/close

Otherwise, accessibility technology doesn't interact
well with modal <dialog>. For example, JAWS would continue
navigating through inert content after a modal is opened.

This patch makes the following changes:
1) Call AXObjectCache::handleFocusedUIElementChanged
from inertSubtreesChanged. This is necessary because a
focus event for some AX object id may have been queued up and
became invalid after throwing away the AX tree.

2) Call AXObjectCache::handleFocusedUIElementChanged
in Document::setFocusedElement even if m_focusedElement
is null. This is necessary because we set m_focusedElement
to null when a modal dialog opens, removing focus from the
previous focused element in CheckFocusedElementTask.

3) When AXObjectCache is about to send a focus changed
event on the docment, send one for the active modal dialog
instead. This is a bit of a workaround until the HTML spec bug
for dialogs and focus is resolved (http://crbug.com/298079).
It's necessary because JAWS seems to ignore the focus
event if it is for the document, for the purpose of refreshing
the AT tree.

No test because a layout test won't be able to test what
AX events are sent by Chrome.

BUG=324401, 304779

[falken, 2013-12-03 08:57:39]

dmazzoni@, could you please review?

I also saw your comment about firing an event on the document's body. I'll give
that a try in the meantime to see how it works on the various screenreaders.

[falken, 2013-12-03 09:26:13]

On 2013/12/03 08:57:39, falken wrote:
> dmazzoni@, could you please review?
> 
> I also saw your comment about firing an event on the document's body. I'll
give
> that a try in the meantime to see how it works on the various screenreaders.

Hm, changing focusedUIElementForPage to fire on document->body() doesn't
immediately work: JAWS keeps reading inert content. But the focus event is
not showing up in AccEvent at all, so something is wrong.

[dmazzoni, 2013-12-10 08:07:53]

https://codereview.chromium.org/101193002/diff/20001/Source/core/accessibilit...
File Source/core/accessibility/AXObjectCache.cpp (right):

https://codereview.chromium.org/101193002/diff/20001/Source/core/accessibilit...
Source/core/accessibility/AXObjectCache.cpp:171: // If a modal dialog is active,
let it have default focus instead of the document. JAWS
I'd eliminate the comment about JAWS here - I think this change is reasonable
either way in that it makes sense to place focus in the dialog for all assistive
technology.

(If JAWS is getting stale content if we don't do this, that's a bug - but this
wouldn't be the right workaround if we didn't want focus otherwise.)

https://codereview.chromium.org/101193002/diff/20001/Source/core/accessibilit...
Source/core/accessibility/AXObjectCache.cpp:174: if (focusedNode ==
focusedDocument) {
Would it be possible that focus is outside the dialog, but not the document, at
this point?

https://codereview.chromium.org/101193002/diff/20001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/101193002/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.cpp:3413: if (m_focusedElement != oldFocusedElement) {
Did you mean to get rid of the test for !focusChangeBlocked? Why?

[falken, 2013-12-10 12:19:57]

Thanks for the review! I looked more closely at how it's changing focus event
behavior and think something's still wrong. Under this patch, we're sending
three OBJ_FOCUS events when a modal dialog is opened, and they can come in any
order:

OBJ_FOCUS on PARAGRAPH
OBJ_FOCUS on DIALOG
OBJ_FOCUS on DOCUMENT

This looks wrong. We just want one focus event on DIALOG or at least ensure that
it comes last.

The DOCUMENT event is puzzling to me. Blink is generating a blur event on
DOCUMENT that seems to get transformed into a focus event by Chrome. Perhaps
here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ac...

I kind of see where PARAGRAPH comes from. Immediately after the "Open dialog"
button is clicked, in focusedUIElementForPage, the document believes the focused
element is the button, but we see the button is ignored by accessibility, so
take the nearest non-inert ancestor: the PARAGRAPH (which also is a parent of
the DIALOG and hence non-inert).

Without this patch, just one focus event is sent, on DOCUMENT.

https://codereview.chromium.org/101193002/diff/20001/Source/core/accessibilit...
File Source/core/accessibility/AXObjectCache.cpp (right):

https://codereview.chromium.org/101193002/diff/20001/Source/core/accessibilit...
Source/core/accessibility/AXObjectCache.cpp:174: if (focusedNode ==
focusedDocument) {
On 2013/12/10 08:07:53, Dominic Mazzoni wrote:
> Would it be possible that focus is outside the dialog, but not the document,
at
> this point?

It's possible if you have something like:
<body>
  <div tabindex=0>
    <dialog>

The <div> can have focus.

https://codereview.chromium.org/101193002/diff/20001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/101193002/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.cpp:3413: if (m_focusedElement != oldFocusedElement) {
On 2013/12/10 08:07:53, Dominic Mazzoni wrote:
> Did you mean to get rid of the test for !focusChangeBlocked? Why?

Hum, I remember needing to get rid of it (and thinking it makes sense to always
tell AT if focus changed), but upon re-testing it's OK to keep it in. Probably
it's best to leave it in then.

[dmazzoni, 2013-12-10 15:15:02]

On Tue, Dec 10, 2013 at 4:19 AM, <falken@chromium.org> wrote:

> Thanks for the review! I looked more closely at how it's changing focus
> event
> behavior and think something's still wrong. Under this patch, we're sending
> three OBJ_FOCUS events when a modal dialog is opened, and they can come in
> any
> order:
>
> OBJ_FOCUS on PARAGRAPH
> OBJ_FOCUS on DIALOG
> OBJ_FOCUS on DOCUMENT
>
> This looks wrong. We just want one focus event on DIALOG or at least
> ensure that
> it comes last.
>
> The DOCUMENT event is puzzling to me. Blink is generating a blur event on
> DOCUMENT that seems to get transformed into a focus event by Chrome.
> Perhaps
> here:
> https://code.google.com/p/chromium/codesearch#chromium/
> src/content/browser/accessibility/browser_accessibility_manager.cc&q=
> browser_accessibility_manager.cc&sq=package:chromium&type=cs&l=144


Yes, that does seem like a bug. It's intentional that the blur event in
blink translates to a focus event on the document because we need to always
have something focused, and the convention on the web is that the body
(according to JS) is focused if nothing else is - and in the AX tree, that
gets translated to the root web area, or "document" on windows.

Oh! This blur is coming from
RendererAccessibilityComplete::FocusedNodeChanged. Perhaps that's coming
later, and overriding the code you did in dialog - because Blink still
reports the focused node is the body.

You may have to fix it there. See if commenting out that function fixes it.

Perhaps the right fix is for the blur to be fired from Blink rather than
from RendererAccessibilityComplete::FocusedNodeChanged, which I admit was a
bit of a hack. My fault there.

I kind of see where PARAGRAPH comes from. Immediately after the "Open
> dialog"
> button is clicked, in focusedUIElementForPage, the document believes the
> focused
> element is the button, but we see the button is ignored by accessibility,
> so
> take the nearest non-inert ancestor: the PARAGRAPH (which also is a parent
> of
> the DIALOG and hence non-inert).
>

Yes, that one makes sense and seems harmless.


> Without this patch, just one focus event is sent, on DOCUMENT.


Yep, it sounds like the blur is the problem, because it happens last and
it's generated from code that isn't dialog-aware.

- Dominic



>
>
>
> https://codereview.chromium.org/101193002/diff/20001/
> Source/core/accessibility/AXObjectCache.cpp
> File Source/core/accessibility/AXObjectCache.cpp (right):
>
> https://codereview.chromium.org/101193002/diff/20001/
> Source/core/accessibility/AXObjectCache.cpp#newcode174
> Source/core/accessibility/AXObjectCache.cpp:174: if (focusedNode ==
> focusedDocument) {
> On 2013/12/10 08:07:53, Dominic Mazzoni wrote:
>
>> Would it be possible that focus is outside the dialog, but not the
>>
> document, at
>
>> this point?
>>
>
> It's possible if you have something like:
> <body>
>   <div tabindex=0>
>     <dialog>
>
> The <div> can have focus.
>
>
> https://codereview.chromium.org/101193002/diff/20001/
> Source/core/dom/Document.cpp
> File Source/core/dom/Document.cpp (right):
>
> https://codereview.chromium.org/101193002/diff/20001/
> Source/core/dom/Document.cpp#newcode3413
> Source/core/dom/Document.cpp:3413: if (m_focusedElement !=
> oldFocusedElement) {
> On 2013/12/10 08:07:53, Dominic Mazzoni wrote:
>
>> Did you mean to get rid of the test for !focusChangeBlocked? Why?
>>
>
> Hum, I remember needing to get rid of it (and thinking it makes sense to
> always tell AT if focus changed), but upon re-testing it's OK to keep it
> in. Probably it's best to leave it in then.
>
> https://codereview.chromium.org/101193002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[eseidel, 2013-12-10 17:04:29]

FYI, I'm also working on splitting FocusController in issue 102033007.  That has
little bearing on this work, but you might find it interesting.

[falken, 2013-12-11 03:56:46]

On 2013/12/10 15:15:02, Dominic Mazzoni wrote:
> On Tue, Dec 10, 2013 at 4:19 AM, <mailto:falken@chromium.org> wrote:
> 
> > Thanks for the review! I looked more closely at how it's changing focus
> > event
> > behavior and think something's still wrong. Under this patch, we're sending
> > three OBJ_FOCUS events when a modal dialog is opened, and they can come in
> > any
> > order:
> >
> > OBJ_FOCUS on PARAGRAPH
> > OBJ_FOCUS on DIALOG
> > OBJ_FOCUS on DOCUMENT
> >
> > This looks wrong. We just want one focus event on DIALOG or at least
> > ensure that
> > it comes last.
> >
> > The DOCUMENT event is puzzling to me. Blink is generating a blur event on
> > DOCUMENT that seems to get transformed into a focus event by Chrome.
> > Perhaps
> > here:
> > https://code.google.com/p/chromium/codesearch#chromium/
> > src/content/browser/accessibility/browser_accessibility_manager.cc&q=
> > browser_accessibility_manager.cc&sq=package:chromium&type=cs&l=144
> 
> 
> Yes, that does seem like a bug. It's intentional that the blur event in
> blink translates to a focus event on the document because we need to always
> have something focused, and the convention on the web is that the body
> (according to JS) is focused if nothing else is - and in the AX tree, that
> gets translated to the root web area, or "document" on windows.
> 
> Oh! This blur is coming from
> RendererAccessibilityComplete::FocusedNodeChanged. Perhaps that's coming
> later, and overriding the code you did in dialog - because Blink still
> reports the focused node is the body.
> 
> You may have to fix it there. See if commenting out that function fixes it.
> 
> Perhaps the right fix is for the blur to be fired from Blink rather than
> from RendererAccessibilityComplete::FocusedNodeChanged, which I admit was a
> bit of a hack. My fault there.
> 
> I kind of see where PARAGRAPH comes from. Immediately after the "Open
> > dialog"
> > button is clicked, in focusedUIElementForPage, the document believes the
> > focused
> > element is the button, but we see the button is ignored by accessibility,
> > so
> > take the nearest non-inert ancestor: the PARAGRAPH (which also is a parent
> > of
> > the DIALOG and hence non-inert).
> >
> 
> Yes, that one makes sense and seems harmless.
> 
> 
> > Without this patch, just one focus event is sent, on DOCUMENT.
> 
> 
> Yep, it sounds like the blur is the problem, because it happens last and
> it's generated from code that isn't dialog-aware.

Thanks for the info! Well, a problem is the three events can be sent in any
order-- there's no consistent ordering.

BUT, Hixie just updated the spec: http://html5.org/r/8338

Now the first control in the dialog gets focus, if nothing was autofocused. So
suddenly this focus forcing is for the rare case where a dialog has no focusable
controls. I'm going to try to implement the new spec change and see if I can get
it working well for NVDA. 

> 
> - Dominic
> 
> 
> 
> >
> >
> >
> > https://codereview.chromium.org/101193002/diff/20001/
> > Source/core/accessibility/AXObjectCache.cpp
> > File Source/core/accessibility/AXObjectCache.cpp (right):
> >
> > https://codereview.chromium.org/101193002/diff/20001/
> > Source/core/accessibility/AXObjectCache.cpp#newcode174
> > Source/core/accessibility/AXObjectCache.cpp:174: if (focusedNode ==
> > focusedDocument) {
> > On 2013/12/10 08:07:53, Dominic Mazzoni wrote:
> >
> >> Would it be possible that focus is outside the dialog, but not the
> >>
> > document, at
> >
> >> this point?
> >>
> >
> > It's possible if you have something like:
> > <body>
> >   <div tabindex=0>
> >     <dialog>
> >
> > The <div> can have focus.
> >
> >
> > https://codereview.chromium.org/101193002/diff/20001/
> > Source/core/dom/Document.cpp
> > File Source/core/dom/Document.cpp (right):
> >
> > https://codereview.chromium.org/101193002/diff/20001/
> > Source/core/dom/Document.cpp#newcode3413
> > Source/core/dom/Document.cpp:3413: if (m_focusedElement !=
> > oldFocusedElement) {
> > On 2013/12/10 08:07:53, Dominic Mazzoni wrote:
> >
> >> Did you mean to get rid of the test for !focusChangeBlocked? Why?
> >>
> >
> > Hum, I remember needing to get rid of it (and thinking it makes sense to
> > always tell AT if focus changed), but upon re-testing it's OK to keep it
> > in. Probably it's best to leave it in then.
> >
> > https://codereview.chromium.org/101193002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

[falken, 2013-12-12 07:58:01]

On 2013/12/11 03:56:46, falken wrote:
> BUT, Hixie just updated the spec: http://html5.org/r/8338
> 
> Now the first control in the dialog gets focus, if nothing was autofocused. So
> suddenly this focus forcing is for the rare case where a dialog has no
focusable
> controls. I'm going to try to implement the new spec change and see if I can
get
> it working well for NVDA. 

Let me pursue the new focusing steps at:
https://codereview.chromium.org/112553002/

This way we don't need to force a focus event, it just happens, ignoring the
rare
case of a modal dialog with no focusable elements.