1620510 - If previously focused element is removed after FocusManager queues new focus event but before event is processed, new focus is lost

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P1)

Description

[James Teh [:Jamie]]

I discovered this in testing the new select dropdown implementation in bug 1118086.

STR:

1. If it isn't landed, apply the patch from bug 1118086.
2. Open this test case:
   data:text/html,<select id="sel"></select><script>let html = "";for (let i = 0; i < 1000; ++i) {html += `<option>${i}</option>`;}sel.innerHTML = html;sel.value = "0";</script>
3. Tab to the select.
4. Press alt+downArrow to open it.
5. Press downArrow to select "1".
6. Press end to jump to the last item.
   • Expected: Accessible focus should go to "999".
   • Actual: Accessible focus goes to the combo box accessible.

here's what's happening:

1. Option "1" gets focused. a11y::FocusManager::mLastFocus is set to option "1".
2. When you press end, focus gets queued (due to aria-activedescendant) for option "999".
3. Before that focus event can be processed, option "1" gets unbound from the a11y tree.
4. As part of that unbinding, we check whether the accessible being unbound is the last focus (mLastFocus). As per (1), it is.
5. So we then tell FocusManager to reset the active item, causing it to throw away the active item set in (2) and fire focus on the DOM focus instead.

We can fix this by setting mLastFocus when queuing the focus event instead of when processing that event.

Comment 1

[Marco Zehe (:MarcoZ)]

Note that testcase the way you have written it produces an error "Expected expression, got ;", and the list is not populated. The <option>... bit must be a string, and the i inside must be written differently, or it won't be picked up. The above testcase looks like some kind of React dialect artifact.

Comment 2

[James Teh [:Jamie]]

Fixed. It's a Javascript template string, but it rendered incorrectly due to a Markdown fail on my part.

Comment 3

[Marco Zehe (:MarcoZ)]

Yup, fixed now. I can also confirm that moving the assignment of mLastFocus from ProcessFocusEvent to DispatchFocusEvent fixes this test case.

Comment 4

[James Teh [:Jamie]]

Attachment: Bug 1620510: Set the last a11y focus when queuing a focus event instead of when processing it.

Previously, the following scenario was possible:

1. aria-activedescendant is used on a menupopup to set a11y focus. When the event is processed, FocusManager::mLastFocus gets set accordingly.
2. aria-activedescendant on the menupopup is changed, so a focus event gets queued.
3. Before that focus event can be processed, the Accessible in (1) gets unbound from the a11y tree.
4. DocAccessible::UnbindFromDocument checks whether the Accessible is the last focus. As per (1), it is.
5. It then tells FocusManager to reset the active item, causing it to throw away the active item set in (2) and fire focus on the DOM focus (which is not the menupopup).

This breaks the new select dropdown implementation.

To fix this, we now set mLastFocus in DispatchFocusEvent (when the event is queued) instead of ProcessFocusEvent.
This way, we don't override a pending new focus change unintentionally.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ce4886b43c8
Set the last a11y focus when queuing a focus event instead of when processing it. r=MarcoZ

Comment 6

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/9ce4886b43c8