Changeset 270609 in webkit

Timestamp:

Dec 9, 2020, 4:35:27 PM (6 years ago)

Author:

Andres Gonzalez

Message:

Fix for focus tracking in isolated tree mode.
​https://bugs.webkit.org/show_bug.cgi?id=219662

Reviewed by Chris Fleizach.

Covered by existing tests.

• AXIsolatedTree::setFocusedNodeID and applyPendingChanges now properly

handle the focused node ID update when the focused object changes.

• AccessibilityObject::setFocused sets focus and activates the

corresponding view. This was done in the wrapper baseAccessibilitySetFocus
method, but this is a more appropriate place for this core functionality.

• Some code cleanup, ASSERT checks of appropriate thread, and additional

logging.

• accessibility/AccessibilityObject.cpp:

(WebCore::AccessibilityObject::setFocused):

• accessibility/AccessibilityObject.h:
• accessibility/AccessibilityRenderObject.cpp:

(WebCore::AccessibilityRenderObject::setFocused):

• accessibility/AccessibilityScrollView.cpp:

(WebCore::AccessibilityScrollView::setFocused):

• accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:

(-[WebAccessibilityObjectWrapper _accessibilitySetFocus:]):

• accessibility/isolatedtree/AXIsolatedObject.cpp:

(WebCore::AXIsolatedObject::page const):
(WebCore::AXIsolatedObject::document const):
(WebCore::AXIsolatedObject::documentFrameView const):

• accessibility/isolatedtree/AXIsolatedTree.cpp:

(WebCore::AXIsolatedTree::setFocusedNodeID):
(WebCore::AXIsolatedTree::applyPendingChanges):

• accessibility/mac/WebAccessibilityObjectWrapperBase.h:
• accessibility/mac/WebAccessibilityObjectWrapperBase.mm:

(-[WebAccessibilityObjectWrapperBase baseAccessibilitySetFocus:]):
Deleted, not needed since core functionality is now in AccessibilityObject::setFocused.

• accessibility/mac/WebAccessibilityObjectWrapperMac.mm:

(-[WebAccessibilityObjectWrapper accessibilityAttributeValue:]):
(-[WebAccessibilityObjectWrapper _accessibilitySetValue:forAttribute:]):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• accessibility/AccessibilityObject.cpp (modified) (1 diff)
• accessibility/AccessibilityObject.h (modified) (1 diff)
• accessibility/AccessibilityRenderObject.cpp (modified) (3 diffs)
• accessibility/AccessibilityScrollView.cpp (modified) (1 diff)
• accessibility/ios/WebAccessibilityObjectWrapperIOS.mm (modified) (1 diff)
• accessibility/isolatedtree/AXIsolatedObject.cpp (modified) (3 diffs)
• accessibility/isolatedtree/AXIsolatedTree.cpp (modified) (2 diffs)
• accessibility/mac/WebAccessibilityObjectWrapperBase.h (modified) (1 diff)
• accessibility/mac/WebAccessibilityObjectWrapperBase.mm (modified) (2 diffs)
• accessibility/mac/WebAccessibilityObjectWrapperMac.mm (modified) (3 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-12-09  Andres Gonzalez  <andresg_22@apple.com>
+
+        Fix for focus tracking in isolated tree mode.
+        https://bugs.webkit.org/show_bug.cgi?id=219662
+
+        Reviewed by Chris Fleizach.
+
+        Covered by existing tests.
+
+        - AXIsolatedTree::setFocusedNodeID and applyPendingChanges now properly
+        handle the focused node ID update when the focused object changes.
+        - AccessibilityObject::setFocused sets focus and activates the
+        corresponding view. This was done in the wrapper baseAccessibilitySetFocus
+        method, but this is a more appropriate place for this core functionality.
+        - Some code cleanup, ASSERT checks of appropriate thread, and additional
+        logging.
+
+        * accessibility/AccessibilityObject.cpp:
+        (WebCore::AccessibilityObject::setFocused):
+        * accessibility/AccessibilityObject.h:
+        * accessibility/AccessibilityRenderObject.cpp:
+        (WebCore::AccessibilityRenderObject::setFocused):
+        * accessibility/AccessibilityScrollView.cpp:
+        (WebCore::AccessibilityScrollView::setFocused):
+        * accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:
+        (-[WebAccessibilityObjectWrapper _accessibilitySetFocus:]):
+        * accessibility/isolatedtree/AXIsolatedObject.cpp:
+        (WebCore::AXIsolatedObject::page const):
+        (WebCore::AXIsolatedObject::document const):
+        (WebCore::AXIsolatedObject::documentFrameView const):
+        * accessibility/isolatedtree/AXIsolatedTree.cpp:
+        (WebCore::AXIsolatedTree::setFocusedNodeID):
+        (WebCore::AXIsolatedTree::applyPendingChanges):
+        * accessibility/mac/WebAccessibilityObjectWrapperBase.h:
+        * accessibility/mac/WebAccessibilityObjectWrapperBase.mm:
+        (-[WebAccessibilityObjectWrapperBase baseAccessibilitySetFocus:]):
+        Deleted, not needed since core functionality is now in AccessibilityObject::setFocused.
+        * accessibility/mac/WebAccessibilityObjectWrapperMac.mm:
+        (-[WebAccessibilityObjectWrapper accessibilityAttributeValue:]):
+        (-[WebAccessibilityObjectWrapper _accessibilitySetValue:forAttribute:]):
+
 2020-12-09  Said Abou-Hallawa  <said@apple.com>
 

trunk/Source/WebCore/accessibility/AccessibilityObject.cpp

@@ -2562 +2562 @@
     return page && axObjectCache ? axObjectCache->focusedObjectForPage(page) : nullptr;
 }
-    
+
+void AccessibilityObject::setFocused(bool focus)
+{
+    if (focus) {
+        // Ensure that the view is focused and active, otherwise, any attempt to set focus to an object inside it will fail.
+        auto* document = this->document();
+        if (!document)
+            return;
+
+        auto* frame = document->frame();
+        if (frame && frame->selection().isFocusedAndActive())
+            return; // Nothing to do, already focused and active.
+
+        auto* page = document->page();
+        if (!page)
+            return;
+
+        ChromeClient& chromeClient = page->chrome().client();
+        chromeClient.focus();
+
+#if PLATFORM(COCOA)
+        auto* frameView = documentFrameView();
+        if (!frameView)
+            return;
+
+        // Legacy WebKit1 case.
+        if (frameView->platformWidget())
+            chromeClient.makeFirstResponder((NSResponder *)frameView->platformWidget());
+        else
+            chromeClient.assistiveTechnologyMakeFirstResponder();
+#endif
+    }
+}
+
 AccessibilitySortDirection AccessibilityObject::sortDirection() const
 {

trunk/Source/WebCore/accessibility/AccessibilityObject.h

@@ -458 +458 @@
     bool isInlineText() const override;
 
-    void setFocused(bool) override { }
+    // Ensures that the view is focused and active before attempting to set focus to an AccessibilityObject.
+    // Subclasses that override setFocused should call this base implementation first.
+    void setFocused(bool) override;
+
     void setSelectedText(const String&) override { }
     void setSelectedTextRange(const PlainTextRange&) override { }

trunk/Source/WebCore/accessibility/AccessibilityRenderObject.cpp

@@ -1850 +1850 @@
     if (!canSetFocusAttribute())
         return;
-    
+
     Document* document = this->document();
     Node* node = this->node();
@@ -1858 +1858 @@
         return;
     }
+
+    // Call the base class setFocused to ensure the view is focused and active.
+    AccessibilityObject::setFocused(on);
 
     // When a node is told to set focus, that can cause it to be deallocated, which means that doing
@@ -1863 +1866 @@
     // long enough for duration.
     RefPtr<AccessibilityObject> protectedThis(this);
-    
+
     // If this node is already the currently focused node, then calling focus() won't do anything.
     // That is a problem when focus is removed from the webpage to chrome, and then returns.

trunk/Source/WebCore/accessibility/AccessibilityScrollView.cpp

@@ -105 +105 @@
     return webArea && webArea->isFocused();
 }
-    
+
 void AccessibilityScrollView::setFocused(bool focused)
 {
+    // Call the base class setFocused to ensure the view is focused and active.
+    AccessibilityObject::setFocused(focused);
+
     if (AccessibilityObject* webArea = webAreaObject())
         webArea->setFocused(focused);

trunk/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm

@@ -2082 +2082 @@
 - (void)_accessibilitySetFocus:(BOOL)focus
 {
-    [self baseAccessibilitySetFocus:focus];
+    if (auto* backingObject = self.axBackingObject)
+        backingObject->setFocused(focus);
 }
 

trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp

@@ -1906 +1906 @@
 Page* AXIsolatedObject::page() const
 {
-    if (auto* object = associatedAXObject())
-        return object->page();
+    ASSERT(isMainThread());
+
+    if (auto* axObject = associatedAXObject())
+        return axObject->page();
+
     ASSERT_NOT_REACHED();
     return nullptr;
@@ -1914 +1917 @@
 Document* AXIsolatedObject::document() const
 {
-    if (auto* object = associatedAXObject())
-        return object->document();
+    ASSERT(isMainThread());
+
+    if (auto* axObject = associatedAXObject())
+        return axObject->document();
+
     ASSERT_NOT_REACHED();
     return nullptr;
@@ -1922 +1928 @@
 FrameView* AXIsolatedObject::documentFrameView() const
 {
-    if (auto* object = associatedAXObject())
-        return object->documentFrameView();
+    ASSERT(isMainThread());
+
+    if (auto* axObject = associatedAXObject())
+        return axObject->documentFrameView();
+
+    ASSERT_NOT_REACHED();
     return nullptr;
 }

trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp

@@ -389 +389 @@
     AXLOG(makeString("axID ", axID));
     ASSERT(isMainThread());
+
     LockHolder locker { m_changeLogLock };
     m_pendingFocusedNodeID = axID;
+
+    AXPropertyMap propertyMap;
+    propertyMap.set(AXPropertyName::IsFocused, true);
+    m_pendingPropertyChanges.append({ axID, propertyMap });
 }
 
@@ -437 +442 @@
     LockHolder locker { m_changeLogLock };
 
-    AXLOG(makeString("focusedNodeID ", m_focusedNodeID, " pendingFocusedNodeID ", m_pendingFocusedNodeID));
-    m_focusedNodeID = m_pendingFocusedNodeID;
+    if (m_pendingFocusedNodeID != m_focusedNodeID) {
+        AXLOG(makeString("focusedNodeID ", m_focusedNodeID, " pendingFocusedNodeID ", m_pendingFocusedNodeID));
+
+        if (m_focusedNodeID != InvalidAXID) {
+            // Set the old focused object's IsFocused property to false.
+            AXPropertyMap propertyMap;
+            propertyMap.set(AXPropertyName::IsFocused, false);
+            m_pendingPropertyChanges.append({ m_focusedNodeID, propertyMap });
+        }
+        m_focusedNodeID = m_pendingFocusedNodeID;
+    }
 
     while (m_pendingNodeRemovals.size()) {

trunk/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.h

@@ -75 +75 @@
 - (NSArray<NSString *> *)baseAccessibilitySpeechHint;
 
-- (void)baseAccessibilitySetFocus:(BOOL)focus;
 - (NSString *)ariaLandmarkRoleDescription;
 

trunk/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm

@@ -44 +44 @@
 #import "AccessibilityTableColumn.h"
 #import "AccessibilityTableRow.h"
-#import "Chrome.h"
-#import "ChromeClient.h"
 #import "ColorMac.h"
 #import "ContextMenuController.h"
@@ -468 +466 @@
 {
     return self.axBackingObject->ariaLandmarkRoleDescription();
-}
-
-- (void)baseAccessibilitySetFocus:(BOOL)focus
-{
-    // If focus is just set without making the view the first responder, then keyboard focus won't move to the right place.
-    if (focus && !self.axBackingObject->document()->frame()->selection().isFocusedAndActive()) {
-        FrameView* frameView = self.axBackingObject->documentFrameView();
-        Page* page = self.axBackingObject->page();
-        if (page && frameView) {
-            ChromeClient& chromeClient = page->chrome().client();
-            chromeClient.focus();
-
-            // Legacy WebKit1 case.
-            if (frameView->platformWidget())
-                chromeClient.makeFirstResponder(frameView->platformWidget());
-            else
-                chromeClient.assistiveTechnologyMakeFirstResponder();
-        }
-    }
-
-    self.axBackingObject->setFocused(focus);
 }
 

trunk/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm

@@ -2140 +2140 @@
 {
     AXTRACE(makeString("WebAccessibilityObjectWrapper accessibilityAttributeValue:", String(attributeName)));
+
     auto* backingObject = self.updateObjectBackingStore;
-    if (!backingObject)
+    if (!backingObject) {
+        AXLOG("No backingObject!!!");
         return nil;
+    }
 
     if (backingObject->isDetachedFromParent()) {
@@ -3361 +3364 @@
 }
 
-- (void)_accessibilitySetValue:(id)value forAttribute:(NSString*)attributeName
-{
+- (void)_accessibilitySetValue:(id)value forAttribute:(NSString *)attributeName
+{
+    AXTRACE(makeString("WebAccessibilityObjectWrapper _accessibilitySetValue: forAttribute:", String(attributeName)));
+
     auto* backingObject = self.updateObjectBackingStore;
-    if (!backingObject)
+    if (!backingObject) {
+        AXLOG("No backingObject!!!");
         return;
+    }
 
     AXTextMarkerRangeRef textMarkerRange = nil;
@@ -3393 +3400 @@
         });
     } else if ([attributeName isEqualToString: NSAccessibilityFocusedAttribute]) {
-        [self baseAccessibilitySetFocus:[number boolValue]];
+        backingObject->setFocused([number boolValue]);
     } else if ([attributeName isEqualToString: NSAccessibilityValueAttribute]) {
         if (number && backingObject->canSetNumericValue())