Joanmarie Diggs Reporter
	Description • 9 years ago

Steps to reproduce:
1. Load the attached test case in Firefox
2. Launch DOM inspector and listen for (at least) focus events
3. Tab to the "Display a dialog" link
4. Sanity check: Press Ctrl+O to open native "Open" dialog. Press Escape.
5. Press Return on "Display a dialog" link. Press Escape.

Expected results: After both steps 4 and 5, there would be a focus event for the "Display a dialog" link which just re-gained focus after each dialog was dismissed.

Actual results: After step 4, there is a focus event for the "Display a dialog" link which just re-gained focus. However, after step 5, there is not a focus event for the "Display a dialog" link.

There are also missing window: accessibility events which I'll provide steps for in a subsequent comment.

Impact: Screen readers don't know where the user is after the ARIA dialog is dismissed.

	Joanmarie Diggs Reporter
	Comment 1 • 9 years ago

Steps to reproduce:
1. Launch the attached accessible-event listener in terminal
2. Perform the dialog opening and dismissing steps from the opening report

Results:
* When the native dialog gains focus, the Firefox main window emits a window:deactivate event. (Good)
* When the native dialog is dismissed, the Firefox main window emits a window:activate event. (Good)
* When the ARIA dialog gains focus, it emits a window:activate event. (Good) BUT the Firefox main window does not emit a window:deactivate event. (Not good; but workaroundable)
* When the ARIA dialog loses focus, it emits a window:deactivate event. (Good) BUT the Firefox main window does not emit a window:activate event. (BAD and hard to reliably work around, especially given the lack of focus events mentioned in the opening report. Basically, all Orca knows is that the user is no longer in the ARIA dialog. Orca has no clue where the user actually is because nothing claims to be the active window or the focused object.)

	Marco Zehe (:MarcoZ)
	Comment 2 • 9 years ago

Running the test case, I see that this apparently does not work as intended in the first place. In the Error Console, I see several of these errors:

attachment.cgi
Error
TypeError: okbutton is null
attachment.cgi
:39:3
Error
TypeError: pagebackground is null

So the intended logic doesn't kick in for me. The OK button doesn't gain focus when the dialog opens, and subsequently, the return to the other element doesn't work, either.

	Joanmarie Diggs Reporter
	Comment 3 • 9 years ago

Ok, this version of the test case corrects the errors. The focus event for the link is present, but the window events I mentioned in my comment are still missing and that really should be fixed. The original, in-the-wild test case is not for a link-triggered popup dialog, but for a time-triggered one. (I'll see if I can make another test case in which the window events are missing and the focus events are also missing.)

	Joanmarie Diggs Reporter
	Comment 4 • 9 years ago

Steps to reproduce:
1. Enable native caret navigation in Firefox
2. Load the test case
3. Tab to the "Line 1" link
4. Press Down arrow to move focus off that link to the text with "Line 2"
5. Wait 5 seconds for the dialog to appear
6. Press Escape

Expected results: At least one of the following (preferably both):
a. A window activate event for the Firefox window
b. A focus event for the document frame

Actual results: I don't see a or b. As a result, if the Orca user happens to not be on a focusable object at the time the dialog pops up, Orca gets no events indicating what has focus/is active after the dialog is dismissed.

In addition: The caret is no longer in the document frame, so the user cannot Down Arrow to Line 3.

This may be bad authoring, but it happens in the wild. If you (Firefox) happen to know the user is back in the document and the Firefox window is the active window, emitting the corresponding signals would be appreciated. Thanks!

	James Teh [:Jamie] Assignee
	Comment 5 • 6 years ago

Really simple test case:
data:text/html,<div role="dialog"><button autofocus onClick="this.parentNode.removeChild(this);">Close</button></div><button>Dummy</button>
Clicking the close button removes the dialog and fires no focus event. Happens all the time in the wild, and in many implementations, this can happen even if there *was* something focused before the dialog was opened. For NVDA, this is even worse because if the user cursors around, they'll still see the dialog content, since it's in a separate buffer.

Stated generally, the problem is that if focus is on an element which is removed (and blur isn't called before removal), no focus event is fired.

I think this is due to bug 559561. I thought there was a general a11y bug for this, but if there is, I can't find it.

It doesn't seem like bug 559561 is going anywhere and that'll probably require a lot of DOM/layout expertise to fix. I think we should just try to work around this in a11y for now.

Eitan, I seem to recall you saying you deal with this in AccessFu somehow. I'm guessing you watch for removals and adjust the virtual cursor? How do you manage this efficiently? Can we borrow some ideas more generally here?

Setting to p2 because this is super annoying for ARIA dialogs all over the place, but might need to adjust if a workaround turns out to be too complex. Marco, appreciate your thoughts on priority also. Have you seen this too?

	Marco Zehe (:MarcoZ)
	Comment 6 • 6 years ago

I always advise people to explicitly set focus to something after closing a dialog. Because browsers are in a habit of behaving inconsistently. So even if a button had focus and opened an ARIA dialog, which is only simulated anyway, the DOM doesn't remember where focus was before. Remember, ARIA dialogs aren't really dialogs for the browser. So, I tell people, if they make something close the dialog, make sure to set focus back to the control that opened it, or the control that makes most sense in the UX flow of the particular web application. And that generally works well. But as a web author, you have to consciously do that. And I think it's even documented like that in the authoring best practices.

	James Teh [:Jamie] Assignee
	Comment 7 • 6 years ago

(In reply to Marco Zehe (:MarcoZ) from comment #6)
> I always advise people to explicitly set focus to something after closing a
> dialog.

To clarify, I was more curious about what you see in the wild. I know what authors *should* do and I know the spec probably even *tells* them to do that. But in your experience, do real world sites actually follow that guidance? I've seen quite a lot in my time that don't, but maybe I'm an anomaly... and I can't think of specific examples right now, of course. :(

If our users are hitting this on a regular basis and we can alter behaviour to work around poor authoring without boiling the ocean, it makes sense to prioritise this. If, on the other hand, this kind of poor authoring is rare, this can e a p3 instead of a p2. (It's a p3 even aside from ARIA dialogs because focus should just never go to limbo at all.)

	Eitan Isaacson [:eeejay]
	Comment 8 • 6 years ago

I think this is an implementation bug on our part. When a focused element is destroyed, the activeElement is reset to the <body> tag, just like if you did a blur() on the focused element. Except that in the former case we don't fire an event that the document was focused.

In accessfu we do weird dirty tricks where we listen for new dialogs and store the pre-dialog cursor position. When the dialog is dismissed we wait half a second to allow the web app to Do The Right Thing and explicitly set focus. If it doesn't we use the stored location, and hopefully it still exists.

	Eitan Isaacson [:eeejay]
	Comment 9 • 6 years ago

Of course, Joanie already knew this was a bug on our part 3 years ago :)

	James Teh [:Jamie] Assignee
	Comment 10 • 6 years ago

(In reply to Eitan Isaacson [:eeejay] from comment #8)
> I think this is an implementation bug on our part. When a focused element is
> destroyed, the activeElement is reset to the <body> tag, just like if you
> did a blur() on the focused element. Except that in the former case we don't
> fire an event that the document was focused.

I assumed this was because DOM isn't firing blur events for this case, which is apparently a DOM bug. See bug 559561. Are you saying there's more explicit a11y code for this case?

> In accessfu we do weird dirty tricks where we listen for new dialogs and
> store the pre-dialog cursor position. When the dialog is dismissed

How do you detect dialog dismissal?

	Eitan Isaacson [:eeejay]
	Comment 11 • 6 years ago

(In reply to James Teh [:Jamie] from comment #10)
> (In reply to Eitan Isaacson [:eeejay] from comment #8)
> > I think this is an implementation bug on our part. When a focused element is
> > destroyed, the activeElement is reset to the <body> tag, just like if you
> > did a blur() on the focused element. Except that in the former case we don't
> > fire an event that the document was focused.
> 
> I assumed this was because DOM isn't firing blur events for this case, which
> is apparently a DOM bug. See bug 559561. Are you saying there's more
> explicit a11y code for this case?
> 

Yeah, not sure who is at fault. Looks like that bug got stuck because they want a destroyed element to send a blur event?
You had a similar bug open too: bug 1230420.

If this won't get resolved in DOM any time soon, I bet we can account for this in FocusManager.

> > In accessfu we do weird dirty tricks where we listen for new dialogs and
> > store the pre-dialog cursor position. When the dialog is dismissed
> 
> How do you detect dialog dismissal?

We listen for a hide event. If the vc position is defunct, or if it is in the subtree of the newly hidden accessible, we restore the pre-dialog position.

https://searchfox.org/mozilla-central/rev/97d488a17a848ce3bebbfc83dc916cf20b88451c/accessible/jsat/EventManager.jsm#338

	James Teh [:Jamie] Assignee
	Comment 12 • 6 years ago

I can fix this with a two line change, but it changes some assumptions we currently have in the code base.

In DocAccessible::UnbindFromDocument, we fire focus on the current focus if the active item was this accessible being unbound. However:
In FocusManager::ProcessFocusEvent, we currently only set mActiveItem if the DOM focus has a CurrentItem() (e.g. aria-activedescendant). When there's no CurrentItem, that means mActiveItem will be null.
Always setting mActiveItem to target, regardless of CurrentItem, fixes this bug.

The question is whether this is problematic. Tests pass and everything seems to work as expected.
However, this separation of active item and focus seems very deliberate; e.g. FocusManager::IsFocused vs FocusManager::IsActiveItem. Aside from being cautious about our focus not being updated correctly, is there any reason to keep this separation?

	alexander :surkov (:asurkov)
	Comment 14 • 6 years ago

(In reply to James Teh [:Jamie] from comment #12)
> I can fix this with a two line change, but it changes some assumptions we
> currently have in the code base.
> 
> In DocAccessible::UnbindFromDocument, we fire focus on the current focus if
> the active item was this accessible being unbound. However:
> In FocusManager::ProcessFocusEvent, we currently only set mActiveItem if the
> DOM focus has a CurrentItem() (e.g. aria-activedescendant). When there's no
> CurrentItem, that means mActiveItem will be null.
> Always setting mActiveItem to target, regardless of CurrentItem, fixes this
> bug.

mActiveItem is about an active item within a widget, for example, a listitem of a listbox will be an active item, but listbox itself is not.

could you describe what happens here and why setting mActiveItem to a focused accessible will fix the bug.

> The question is whether this is problematic. Tests pass and everything seems
> to work as expected.
> However, this separation of active item and focus seems very deliberate;
> e.g. FocusManager::IsFocused vs FocusManager::IsActiveItem. Aside from being
> cautious about our focus not being updated correctly, is there any reason to
> keep this separation?

I think this is because FocusManager relies on DOM to track the focus, however in case of complex widgets like menus and listboxes where DOM focus doesn't match with the accessible focus, we use mActiveItem to track the accessible focus. So, yeah I think the main reason is to not to dupe focus info.

	James Teh [:Jamie] Assignee
	Comment 15 • 6 years ago

(In reply to alexander :surkov (:asurkov) from comment #14)
> could you describe what happens here and why setting mActiveItem to a
> focused accessible will fix the bug.

When an accessible is unbound, we check if it is the currently active item. If it is, we fire a focus event for the accessible which now has the focus (e.g. the document). So, setting mActiveItem to focused accessibles allows this check to work for focused accessibles too, not just active items.

> I think this is because FocusManager relies on DOM to track the focus, ...
> I think the main reason is to not to dupe focus
> info.

The key problem is that DOM doesn't fire blur events when the focused node is removed. Ideally, this would be fixed in DOM, but that bug has been open for years and isn't getting any traction. Clearly, it isn't that important to anyone else, but it *is* important for a11y. So, I think we should just work around this in a11y.

If we don't want to mess with mActiveItem, one other way to do this would be to have an mLastFocused or similar which keeps track of the last accessible for which a focus event was fired. We can then check against that (instead of mActiveItem) when an accessible is unbound. Does this sound better to you?

	alexander :surkov (:asurkov)
	Comment 16 • 6 years ago

Sorry for delay. Still feel confusing on the bug.

I think I've been missing something here. So example from comment #5 has no active items involved, and it claims there's no focus on a document when dialog is closed. I don't get why. Is it because there's no blur event (bug 559561) and we don't process the focus properly when focused accessible shuts down?

Then, aria-descendants case (comment #14 scenario) looks different. FocusMgr()->ActiveItemChanged(nullptr) has to force us to fire an event for a DOM focused node if we have one. Thus I can think only of a problem if FocusedAccessible() returned null, but not sure this is the case here.

Jamie, can you help to connect dots?

	James Teh [:Jamie] Assignee
	Comment 17 • 6 years ago

(In reply to alexander :surkov (:asurkov) from comment #16)
> I think I've been missing something here. So example from comment #5 has no
> active items involved, and it claims there's no focus on a document when
> dialog is closed. I don't get why. Is it because there's no blur event (bug
> 559561) and we don't process the focus properly when focused accessible
> shuts down?

Correct.

> Then, aria-descendants case (comment #14 scenario) looks different.

This bug is not about aria-activedescendant. That already works. My patch just makes it so that mActiveItem is used for focus as well as active items; i.e. we *always* keep track of the focus rather than relying on querying DOM.

If we did go with the approach in my patch, we should probably rename mActiveItem to mFocus or similar and get rid of the concept of separate focus/active item.

An alternative approach is to introduce an mLastFocus or similar and check that as well when an accessible is unbound. That does mean introducing a new variable, but means we still rely on DOM most of the time, only using mLastFocus when checking if the DOM focus died without firing a blur.

Is that any clearer?

	alexander :surkov (:asurkov)
	Comment 18 • 6 years ago

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

> > Then, aria-descendants case (comment #14 scenario) looks different.
> 
> This bug is not about aria-activedescendant. That already works. My patch
> just makes it so that mActiveItem is used for focus as well as active items;
> i.e. we *always* keep track of the focus rather than relying on querying DOM.

I think I understand the patch idea, what I miss is why we fail to fire focus event for a DOM focused element, when active descendant gets unbound from the tree.

	James Teh [:Jamie] Assignee
	Comment 19 • 6 years ago

Because there's no DOM blur, nor is there some other DOM event that would cause us to fire an event. In DocAccessible::UnbindFromDocument, we handle this for the active item:

1323      // Fire focus event on accessible having DOM focus if active item was removed
1324      // from the tree.
1325      if (FocusMgr()->IsActiveItem(aAccessible)) {
1326        FocusMgr()->ActiveItemChanged(nullptr);

The previous focus wasn't the "active item", so this code doesn't apply. My patch changes things so that it does. The alternative approach I mentioned would change:
FocusMgr()->IsActiveItem(aAccessible)
to something like:
FocusMgr()->WasLastFocus(aAccessible)
Even that name is a bit confusing because of the confusion between DOM focus and accessibility focus (where the latter might be DOM focus or active item). Maybe FocusMgr()->FocusWasLastFiredOn?

	alexander :surkov (:asurkov)
	Comment 20 • 6 years ago

(In reply to James Teh [:Jamie] from comment #19)
> Because there's no DOM blur, nor is there some other DOM event that would
> cause us to fire an event. In DocAccessible::UnbindFromDocument, we handle
> this for the active item:
> 
> 1323      // Fire focus event on accessible having DOM focus if active item
> was removed
> 1324      // from the tree.
> 1325      if (FocusMgr()->IsActiveItem(aAccessible)) {
> 1326        FocusMgr()->ActiveItemChanged(nullptr);
> 
> The previous focus wasn't the "active item", so this code doesn't apply.

gotcha, storing mLastFocus should be an easy and performant way to fix it. However I think you may run into a rare bug, when a tree containing not yet fired focus gets shut down. But anyways you solve here the majority of the problem with mLastFocus thing, so you may leave this issue for a follow-up.

	Pulsebot
	Comment 22 • 5 years ago

Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/28b9af9efee6
If the focused accessible is removed from the tree, fire a11y focus on the document. r=eeejay