Summary: | AX: Make m_deferredFocusedNodeChange a single item rather than a list to make AXObjectCache::handleFocusedUIElementChanged easier to reason about | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tyler Wilcock <tyler_w> | ||||||||||||||||||||
Component: | Accessibility | Assignee: | Tyler Wilcock <tyler_w> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | aboxhall, andresg_22, apinheiro, cdumez, cfleizach, dmazzoni, esprehn+autocc, ews-watchlist, jcraig, jdiggs, kangil.han, samuel_white, webkit-bug-importer | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | Other | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Attachments: |
|
Description
Tyler Wilcock
2023-02-26 12:41:10 PST
Created attachment 465186 [details]
Patch
Comment on attachment 465186 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=465186&action=review > Source/WebCore/accessibility/AXObjectCache.cpp:1380 > + m_deferredFocusedNodeChange->second = newNode; Do we need to handle case where Old = a New = b Then 2nd Old = b New = a Do we avoid any work here? (In reply to chris fleizach from comment #3) > Comment on attachment 465186 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=465186&action=review > > > Source/WebCore/accessibility/AXObjectCache.cpp:1380 > > + m_deferredFocusedNodeChange->second = newNode; > > Do we need to handle case where > > Old = a > New = b > > Then 2nd > Old = b > New = a > > Do we avoid any work here? Ah, good point. I would interpret that sequence as no focus change happening (because we never were actually able to commit `b` as the new focused element before `a` regained focus). I'll update the patch. Created attachment 465188 [details]
Patch
Created attachment 465189 [details]
Patch
Created attachment 465193 [details]
Patch
(In reply to Tyler Wilcock from comment #7) > Created attachment 465193 [details] > Patch --- a/Source/WebCore/accessibility/AXObjectCache.cpp +++ b/Source/WebCore/accessibility/AXObjectCache.cpp // Recompute isIgnored after a focus change in case that altered visibility. - recomputeIsIgnored(deferredFocusedChangeContext.first); - recomputeIsIgnored(deferredFocusedChangeContext.second); + recomputeIsIgnored(m_deferredFocusedNodeChange->first.get()); + recomputeIsIgnored(m_deferredFocusedNodeChange->second.get()); in the case of a -> b -> c, in rapid submission, you end up with (a, c), so you never do above for b. Would that have any side effect? --- a/LayoutTests/accessibility/ios-simulator/focus-change-notifications.html +++ b/LayoutTests/accessibility/ios-simulator/focus-change-notifications.html +var rootElement = 0; +var rootElement = null; While you're at this test, can we modernize it? Created attachment 465222 [details]
Patch
(In reply to Tyler Wilcock from comment #9) > Created attachment 465222 [details] > Patch --- a/LayoutTests/accessibility/ios-simulator/focus-change-notifications.html +++ b/LayoutTests/accessibility/ios-simulator/focus-change-notifications.html +successfullyParsed = true; I don't think this variable is needed at all. Created attachment 465224 [details]
Patch
Created attachment 465229 [details]
Patch
Created attachment 465231 [details]
Patch
Created attachment 465232 [details]
Patch
Committed 261037@main (c87b5bf70b93): <https://commits.webkit.org/261037@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 465232 [details]. |