From 7d2201e688642f61a95c40396ab215e0fc2d1198 Mon Sep 17 00:00:00 2001 From: James Teh Date: Fri, 6 May 2022 23:59:43 +0000 Subject: [PATCH] Bug 1192108: Fire focus events after mutation events but before any other events. r=eeejay It's critical that we fire mutation events first because our RemoteAccessible tree is created thus and we can't fire events on RemoteAccessibles we haven't created yet. Beyond that, though, focus events are of primary importance. See the comments in EventQueue::ProcessEventQueue for the reasons. Differential Revision: https://phabricator.services.mozilla.com/D145319 --- accessible/base/EventQueue.cpp | 32 +++++++++++---- accessible/base/EventQueue.h | 3 ++ accessible/base/NotificationController.cpp | 4 +- .../events/docload/test_docload_root.html | 2 +- .../test_focus_aria_activedescendant.html | 40 ++++++++++++++----- .../events/test_focus_autocomplete.html | 2 +- .../geckoview/test/AccessibilityTest.kt | 4 ++ 7 files changed, 65 insertions(+), 22 deletions(-) diff --git a/accessible/base/EventQueue.cpp b/accessible/base/EventQueue.cpp index d62d8ecd3511..238323e59585 100644 --- a/accessible/base/EventQueue.cpp +++ b/accessible/base/EventQueue.cpp @@ -32,6 +32,11 @@ bool EventQueue::PushEvent(AccEvent* aEvent) { aEvent->Document() == mDocument, "Queued event belongs to another document!"); + if (aEvent->mEventType == nsIAccessibleEvent::EVENT_FOCUS) { + mFocusEvent = aEvent; + return true; + } + // XXX(Bug 1631371) Check if this should use a fallible operation as it // pretended earlier, or change the return type to void. mEvents.AppendElement(aEvent); @@ -302,28 +307,39 @@ void EventQueue::CoalesceSelChangeEvents(AccSelChangeEvent* aTailEvent, void EventQueue::ProcessEventQueue() { // Process only currently queued events. const nsTArray > events = std::move(mEvents); - uint32_t eventCount = events.Length(); #ifdef A11Y_LOG - if (eventCount > 0 && logging::IsEnabled(logging::eEvents)) { + if ((eventCount > 0 || mFocusEvent) && logging::IsEnabled(logging::eEvents)) { logging::MsgBegin("EVENTS", "events processing"); logging::Address("document", mDocument); logging::MsgEnd(); } #endif + if (mFocusEvent) { + // Always fire a pending focus event before all other events. We do this for + // two reasons: + // 1. It prevents extraneous screen reader speech if the name, states, etc. + // of the currently focused item change at the same time as another item is + // focused. If aria-activedescendant is used, even setting + // aria-activedescendant before changing other properties results in the + // property change events being queued before the focus event because we + // process aria-activedescendant async. + // 2. It improves perceived performance. Focus changes should be reported as + // soon as possible, so clients should handle focus events before they spend + // time on anything else. + RefPtr event = std::move(mFocusEvent); + if (!event->mAccessible->IsDefunct()) { + FocusMgr()->ProcessFocusEvent(event); + } + } + for (uint32_t idx = 0; idx < eventCount; idx++) { AccEvent* event = events[idx]; if (event->mEventRule != AccEvent::eDoNotEmit) { LocalAccessible* target = event->GetAccessible(); if (!target || target->IsDefunct()) continue; - // Dispatch the focus event if target is still focused. - if (event->mEventType == nsIAccessibleEvent::EVENT_FOCUS) { - FocusMgr()->ProcessFocusEvent(event); - continue; - } - // Dispatch caret moved and text selection change events. if (event->mEventType == nsIAccessibleEvent::EVENT_TEXT_SELECTION_CHANGED) { diff --git a/accessible/base/EventQueue.h b/accessible/base/EventQueue.h index e8a5d210a92a..900b8beaab48 100644 --- a/accessible/base/EventQueue.h +++ b/accessible/base/EventQueue.h @@ -68,6 +68,9 @@ class EventQueue { * SwapElements() on it. */ nsTArray> mEvents; + + // Pending focus event. + RefPtr mFocusEvent; }; } // namespace a11y diff --git a/accessible/base/NotificationController.cpp b/accessible/base/NotificationController.cpp index 7d7519ccdec9..91954b652b86 100644 --- a/accessible/base/NotificationController.cpp +++ b/accessible/base/NotificationController.cpp @@ -71,6 +71,7 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(NotificationController) cb.NoteXPCOMChild(list->ElementAt(i)); } } + NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mFocusEvent) NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mEvents) NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mRelocations) NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END @@ -103,6 +104,7 @@ void NotificationController::Shutdown() { mTextHash.Clear(); mContentInsertions.Clear(); mNotifications.Clear(); + mFocusEvent = nullptr; mEvents.Clear(); mRelocations.Clear(); mEventTree.Clear(); @@ -976,7 +978,7 @@ void NotificationController::WillRefresh(mozilla::TimeStamp aTime) { // Stop further processing if there are no new notifications of any kind or // events and document load is processed. if (mContentInsertions.Count() == 0 && mNotifications.IsEmpty() && - mEvents.IsEmpty() && mTextHash.Count() == 0 && + !mFocusEvent && mEvents.IsEmpty() && mTextHash.Count() == 0 && mHangingChildDocuments.IsEmpty() && mDocument->HasLoadState(DocAccessible::eCompletelyLoaded) && mPresShell->RemoveRefreshObserver(this, FlushType::Display)) { diff --git a/accessible/tests/mochitest/events/docload/test_docload_root.html b/accessible/tests/mochitest/events/docload/test_docload_root.html index 7947327c2805..dab62098f430 100644 --- a/accessible/tests/mochitest/events/docload/test_docload_root.html +++ b/accessible/tests/mochitest/events/docload/test_docload_root.html @@ -37,7 +37,7 @@ } this.eventSeq = [ - new invokerChecker(EVENT_REORDER, gRootAcc), + new asyncInvokerChecker(EVENT_REORDER, gRootAcc), // We use a function here to get the target because gDialog isn't set // yet, but it will be when the function is called. new invokerChecker(EVENT_FOCUS, () => gDialog.document) diff --git a/accessible/tests/mochitest/events/test_focus_aria_activedescendant.html b/accessible/tests/mochitest/events/test_focus_aria_activedescendant.html index 056f94c44274..067c9c0069d3 100644 --- a/accessible/tests/mochitest/events/test_focus_aria_activedescendant.html +++ b/accessible/tests/mochitest/events/test_focus_aria_activedescendant.html @@ -27,17 +27,18 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=429547 function changeARIAActiveDescendant(aContainer, aItem, aPrevItemId) { let itemID = aItem instanceof Node ? aItem.id : aItem; - this.eventSeq = [ - new stateChangeChecker(EXT_STATE_ACTIVE, true, true, aItem), - new focusChecker(aItem), - ]; + this.eventSeq = [new focusChecker(aItem)]; if (aPrevItemId) { - this.eventSeq.unshift( + this.eventSeq.push( new stateChangeChecker(EXT_STATE_ACTIVE, true, false, aPrevItemId) ); } + this.eventSeq.push( + new stateChangeChecker(EXT_STATE_ACTIVE, true, true, aItem) + ); + this.invoke = function changeARIAActiveDescendant_invoke() { getNode(aContainer).setAttribute("aria-activedescendant", itemID); }; @@ -53,7 +54,7 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=429547 ]; if (aPrevItemId) { - this.eventSeq.unshift( + this.eventSeq.push( new stateChangeChecker(EXT_STATE_ACTIVE, true, false, aPrevItemId) ); } @@ -81,7 +82,7 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=429547 ]; if (aPrevItemId) { - this.eventSeq.unshift( + this.eventSeq.push( new stateChangeChecker(EXT_STATE_ACTIVE, true, false, aPrevItemId) ); } @@ -98,17 +99,19 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=429547 function insertItemNFocus(aID, aNewItemID, aPrevItemId) { this.eventSeq = [ new invokerChecker(EVENT_SHOW, aNewItemID), - new stateChangeChecker(EXT_STATE_ACTIVE, true, true, aNewItemID), new focusChecker(aNewItemID), ]; if (aPrevItemId) { - this.eventSeq.splice( - 1, 0, + this.eventSeq.push( new stateChangeChecker(EXT_STATE_ACTIVE, true, false, aPrevItemId) ); } + this.eventSeq.push( + new stateChangeChecker(EXT_STATE_ACTIVE, true, true, aNewItemID) + ); + this.invoke = function insertItemNFocus_invoke() { var container = getNode(aID); @@ -138,8 +141,8 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=429547 */ function moveARIAActiveDescendantID(aFromID, aToID) { this.eventSeq = [ - new stateChangeChecker(EXT_STATE_ACTIVE, true, true, aToID), new focusChecker(aToID), + new stateChangeChecker(EXT_STATE_ACTIVE, true, true, aToID), ]; this.invoke = function moveARIAActiveDescendantID_invoke() { @@ -215,6 +218,7 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=429547 await evtProm; testStates("hiddenListOption", STATE_FOCUSED); + info("Testing active state changes when not focused"); testStates("listbox", 0, 0, STATE_FOCUSED); evtProm = Promise.all([ PromEvents.waitForStateChange("roaming3", EXT_STATE_ACTIVE, false, true), @@ -223,6 +227,20 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=429547 getNode("listbox").setAttribute("aria-activedescendant", "item1"); await evtProm; + info("Testing that focus is always fired first"); + const listbox = getNode("listbox"); + evtProm = PromEvents.waitForEvent(EVENT_FOCUS, "item1"); + listbox.focus(); + await evtProm; + const item1 = getNode("item1"); + evtProm = PromEvents.waitForOrderedEvents([ + [EVENT_FOCUS, "item2"], + [EVENT_NAME_CHANGE, item1], + ], "Focus then name change"); + item1.setAttribute("aria-label", "changed"); + listbox.setAttribute("aria-activedescendant", "item2"); + await evtProm; + SimpleTest.finish(); } diff --git a/accessible/tests/mochitest/events/test_focus_autocomplete.html b/accessible/tests/mochitest/events/test_focus_autocomplete.html index 7f5dee75102f..c179398cc093 100644 --- a/accessible/tests/mochitest/events/test_focus_autocomplete.html +++ b/accessible/tests/mochitest/events/test_focus_autocomplete.html @@ -22,7 +22,7 @@ EVENT_FOCUS, evt => evt.accessible.role == ROLE_COMBOBOX_OPTION ); - if (!event.accessible.name) { + while (!event.accessible.name) { // Sometimes, the name is null for a very short time after the focus // event. await waitForEvent(EVENT_NAME_CHANGE, event.accessible); diff --git a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/AccessibilityTest.kt b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/AccessibilityTest.kt index fb2747ca6015..f833cde4b93d 100644 --- a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/AccessibilityTest.kt +++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/AccessibilityTest.kt @@ -385,6 +385,7 @@ class AccessibilityTest : BaseSessionTest() { } }) + // This focuses the link. mainSession.finder.find("sweet", 0) sessionRule.waitUntilCalled(object : EventDelegate { @AssertCalled(count = 1) @@ -397,6 +398,9 @@ class AccessibilityTest : BaseSessionTest() { // reset caret position mainSession.evaluateJS(""" this.select(document.body, 0, 0); + // Changing DOM selection doesn't focus the document! Force focus + // here so we can use that to determine when this is done. + document.activeElement.blur(); """.trimIndent()) sessionRule.waitUntilCalled(object : EventDelegate { @AssertCalled(count = 1)