Fix for LayoutTests/accessibility/mac/value-change/value-change-user-info-contentedit...
authorandresg_22@apple.com <andresg_22@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 3 Mar 2020 04:07:42 +0000 (04:07 +0000)
committerandresg_22@apple.com <andresg_22@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 3 Mar 2020 04:07:42 +0000 (04:07 +0000)
https://bugs.webkit.org/show_bug.cgi?id=208462

Reviewed by Chris Fleizach.

Covered by LayoutTests/accessibility/mac/value-change/value-change-user-info-contenteditable.html.

- Updates the IsolatedTree on the TextStateChange notification.
- Renamed isAccessibilityScrollView to isAccessibilityScrollViewInstance
and used isScrollView instead everywhere it's appropriate. This makes
code like AXObjectCache::rootWebArea work for both AXObjects and IsolatedObjects.
- Moved several utility functions from WebAccessibilityObjectWrapperMac.mm
to AXObjectCacheMac.mm where they belong, so that they can be used by
AXObjectCache implementation in addition to by the wrapper.

* accessibility/AXObjectCache.cpp:
(WebCore::AXObjectCache::postTextStateChangeNotification):
(WebCore::AXObjectCache::rootWebArea):
* accessibility/AccessibilityObject.cpp:
(WebCore::AccessibilityObject::isOnScreen const):
(WebCore::AccessibilityObject::scrollToGlobalPoint const):
* accessibility/AccessibilityObject.h:
* accessibility/AccessibilityObjectInterface.h:
* accessibility/AccessibilityRenderObject.cpp:
(WebCore::AccessibilityRenderObject::getScrollableAreaIfScrollable const):
* accessibility/AccessibilityScrollView.h:
* accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:
(-[WebAccessibilityObjectWrapper accessibilityContainer]):
* accessibility/isolatedtree/AXIsolatedObject.cpp:
(WebCore::AXIsolatedObject::isAccessibilityScrollViewInstance const):
(WebCore::AXIsolatedObject::isAccessibilityScrollView const): Renamed.
* accessibility/isolatedtree/AXIsolatedObject.h:
* accessibility/mac/AXObjectCacheMac.mm:
(WebCore::AXObjectCache::postTextStateChangePlatformNotification): Uses
TextMarker utilities instead of calling into the wrapper. This fixes
the crash caused by the wrapper updating the backingObject that in turn
may change the wrapper.
(WebCore::AXTextMarkerRange): Moved from WebAccessibilityObjectWrapperMac.mm.
(WebCore::textMarkerRangeFromMarkers): Moved from WebAccessibilityObjectWrapperMac.mm.
(WebCore::textMarkerForVisiblePosition): Moved from WebAccessibilityObjectWrapperMac.mm.
(WebCore::textMarkerRangeFromVisiblePositions): Moved from WebAccessibilityObjectWrapperMac.mm.
* accessibility/mac/WebAccessibilityObjectWrapperMac.h:
* accessibility/mac/WebAccessibilityObjectWrapperMac.mm:
(AXTextMarkerRange): Moved.
(textMarkerForVisiblePosition): Moved.
(textMarkerRangeFromMarkers): Moved.
(textMarkerRangeFromVisiblePositions): Moved.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@257760 268f45cc-cd09-0410-ab3c-d52691b4dbfc

13 files changed:
Source/WebCore/ChangeLog
Source/WebCore/accessibility/AXObjectCache.cpp
Source/WebCore/accessibility/AccessibilityObject.cpp
Source/WebCore/accessibility/AccessibilityObject.h
Source/WebCore/accessibility/AccessibilityObjectInterface.h
Source/WebCore/accessibility/AccessibilityRenderObject.cpp
Source/WebCore/accessibility/AccessibilityScrollView.h
Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm
Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
Source/WebCore/accessibility/mac/AXObjectCacheMac.mm
Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.h
Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm

index 130f4d3..6077480 100644 (file)
@@ -1,3 +1,53 @@
+2020-03-02  Andres Gonzalez  <andresg_22@apple.com>
+
+        Fix for LayoutTests/accessibility/mac/value-change/value-change-user-info-contenteditable.html in IsolatedTree mode.
+        https://bugs.webkit.org/show_bug.cgi?id=208462
+
+        Reviewed by Chris Fleizach.
+
+        Covered by LayoutTests/accessibility/mac/value-change/value-change-user-info-contenteditable.html.
+
+        - Updates the IsolatedTree on the TextStateChange notification.
+        - Renamed isAccessibilityScrollView to isAccessibilityScrollViewInstance
+        and used isScrollView instead everywhere it's appropriate. This makes
+        code like AXObjectCache::rootWebArea work for both AXObjects and IsolatedObjects.
+        - Moved several utility functions from WebAccessibilityObjectWrapperMac.mm
+        to AXObjectCacheMac.mm where they belong, so that they can be used by
+        AXObjectCache implementation in addition to by the wrapper.
+
+        * accessibility/AXObjectCache.cpp:
+        (WebCore::AXObjectCache::postTextStateChangeNotification):
+        (WebCore::AXObjectCache::rootWebArea):
+        * accessibility/AccessibilityObject.cpp:
+        (WebCore::AccessibilityObject::isOnScreen const):
+        (WebCore::AccessibilityObject::scrollToGlobalPoint const):
+        * accessibility/AccessibilityObject.h:
+        * accessibility/AccessibilityObjectInterface.h:
+        * accessibility/AccessibilityRenderObject.cpp:
+        (WebCore::AccessibilityRenderObject::getScrollableAreaIfScrollable const):
+        * accessibility/AccessibilityScrollView.h:
+        * accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:
+        (-[WebAccessibilityObjectWrapper accessibilityContainer]):
+        * accessibility/isolatedtree/AXIsolatedObject.cpp:
+        (WebCore::AXIsolatedObject::isAccessibilityScrollViewInstance const):
+        (WebCore::AXIsolatedObject::isAccessibilityScrollView const): Renamed.
+        * accessibility/isolatedtree/AXIsolatedObject.h:
+        * accessibility/mac/AXObjectCacheMac.mm:
+        (WebCore::AXObjectCache::postTextStateChangePlatformNotification): Uses
+        TextMarker utilities instead of calling into the wrapper. This fixes
+        the crash caused by the wrapper updating the backingObject that in turn
+        may change the wrapper.
+        (WebCore::AXTextMarkerRange): Moved from WebAccessibilityObjectWrapperMac.mm.
+        (WebCore::textMarkerRangeFromMarkers): Moved from WebAccessibilityObjectWrapperMac.mm.
+        (WebCore::textMarkerForVisiblePosition): Moved from WebAccessibilityObjectWrapperMac.mm.
+        (WebCore::textMarkerRangeFromVisiblePositions): Moved from WebAccessibilityObjectWrapperMac.mm.
+        * accessibility/mac/WebAccessibilityObjectWrapperMac.h:
+        * accessibility/mac/WebAccessibilityObjectWrapperMac.mm:
+        (AXTextMarkerRange): Moved.
+        (textMarkerForVisiblePosition): Moved.
+        (textMarkerRangeFromMarkers): Moved.
+        (textMarkerRangeFromVisiblePositions): Moved.
+
 2020-03-02  Devin Rousso  <drousso@apple.com>
 
         Web Inspector: Items in the toolbar take up to much vertical space
index d374003..5051347 100644 (file)
@@ -1401,9 +1401,7 @@ void AXObjectCache::postTextStateChangeNotification(AccessibilityObject* object,
 
 void AXObjectCache::postTextStateChangeNotification(Node* node, AXTextEditType type, const String& text, const VisiblePosition& position)
 {
-    if (!node)
-        return;
-    if (type == AXTextEditTypeUnknown)
+    if (!node || type == AXTextEditTypeUnknown)
         return;
 
     stopCachingComputedObjectAttributes();
@@ -1416,6 +1414,10 @@ void AXObjectCache::postTextStateChangeNotification(Node* node, AXTextEditType t
         object = object->observableObject();
     }
 
+#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
+    updateIsolatedTree(object, AXValueChanged);
+#endif
+
     postTextStateChangePlatformNotification(object, type, text, position);
 #else
     nodeTextChangePlatformNotification(object, textChangeForEditType(type), position.deepEquivalent().deprecatedEditingOffset(), text);
@@ -3247,7 +3249,7 @@ bool isNodeAriaVisible(Node* node)
 AXCoreObject* AXObjectCache::rootWebArea()
 {
     AXCoreObject* rootObject = this->rootObject();
-    if (!rootObject || !rootObject->isAccessibilityScrollView())
+    if (!rootObject || !rootObject->isScrollView())
         return nullptr;
     return rootObject->webAreaObject();
 }
index 72a2370..23a15fd 100644 (file)
@@ -2942,7 +2942,7 @@ bool AccessibilityObject::isOnScreen() const
         const AccessibilityObject* inner = objects[i - 1];
         // FIXME: unclear if we need LegacyIOSDocumentVisibleRect.
         const IntRect outerRect = i < levels ? snappedIntRect(outer->boundingBoxRect()) : outer->getScrollableAreaIfScrollable()->visibleContentRect(ScrollableArea::LegacyIOSDocumentVisibleRect);
-        const IntRect innerRect = snappedIntRect(inner->isAccessibilityScrollView() ? inner->parentObject()->boundingBoxRect() : inner->boundingBoxRect());
+        const IntRect innerRect = snappedIntRect(inner->isScrollView() ? inner->parentObject()->boundingBoxRect() : inner->boundingBoxRect());
         
         if (!outerRect.intersects(innerRect)) {
             isOnscreen = false;
@@ -3038,13 +3038,13 @@ void AccessibilityObject::scrollToGlobalPoint(const IntPoint& globalPoint) const
 
         ScrollableArea* scrollableArea = outer->getScrollableAreaIfScrollable();
 
-        LayoutRect innerRect = inner->isAccessibilityScrollView() ? inner->parentObject()->boundingBoxRect() : inner->boundingBoxRect();
+        LayoutRect innerRect = inner->isScrollView() ? inner->parentObject()->boundingBoxRect() : inner->boundingBoxRect();
         LayoutRect objectRect = innerRect;
         IntPoint scrollPosition = scrollableArea->scrollPosition();
 
         // Convert the object rect into local coordinates.
         objectRect.move(offsetX, offsetY);
-        if (!outer->isAccessibilityScrollView())
+        if (!outer->isScrollView())
             objectRect.move(scrollPosition.x(), scrollPosition.y());
 
         int desiredX = computeBestScrollOffset(
@@ -3059,7 +3059,7 @@ void AccessibilityObject::scrollToGlobalPoint(const IntPoint& globalPoint) const
             point.y(), point.y());
         outer->scrollTo(IntPoint(desiredX, desiredY));
 
-        if (outer->isAccessibilityScrollView() && !inner->isAccessibilityScrollView()) {
+        if (outer->isScrollView() && !inner->isScrollView()) {
             // If outer object we just scrolled is a scroll view (main window or iframe) but the
             // inner object is not, keep track of the coordinate transformation to apply to
             // future nested calculations.
@@ -3068,7 +3068,7 @@ void AccessibilityObject::scrollToGlobalPoint(const IntPoint& globalPoint) const
             offsetY -= (scrollPosition.y() + point.y());
             point.move(scrollPosition.x() - innerRect.x(),
                        scrollPosition.y() - innerRect.y());
-        } else if (inner->isAccessibilityScrollView()) {
+        } else if (inner->isScrollView()) {
             // Otherwise, if the inner object is a scroll view, reset the coordinate transformation.
             offsetX = 0;
             offsetY = 0;
index dc35aa9..d5ebd64 100644 (file)
@@ -110,7 +110,7 @@ public:
     bool isAccessibilityNodeObject() const override { return false; }
     bool isAccessibilityRenderObject() const override { return false; }
     bool isAccessibilityScrollbar() const override { return false; }
-    bool isAccessibilityScrollView() const override { return false; }
+    bool isAccessibilityScrollViewInstance() const override { return false; }
     bool isAccessibilitySVGRoot() const override { return false; }
     bool isAccessibilitySVGElement() const override { return false; }
     bool isAccessibilityTableInstance() const override { return false; }
index cafdd66..6b3d00a 100644 (file)
@@ -485,7 +485,7 @@ public:
     virtual bool isAccessibilityNodeObject() const = 0;
     virtual bool isAccessibilityRenderObject() const = 0;
     virtual bool isAccessibilityScrollbar() const = 0;
-    virtual bool isAccessibilityScrollView() const = 0;
+    virtual bool isAccessibilityScrollViewInstance() const = 0;
     virtual bool isAccessibilitySVGRoot() const = 0;
     virtual bool isAccessibilitySVGElement() const = 0;
     virtual bool isAccessibilityTableInstance() const = 0;
index 90da1a7..688bb03 100644 (file)
@@ -3858,8 +3858,10 @@ String AccessibilityRenderObject::passwordFieldValue() const
 ScrollableArea* AccessibilityRenderObject::getScrollableAreaIfScrollable() const
 {
     // If the parent is a scroll view, then this object isn't really scrollable, the parent ScrollView should handle the scrolling.
-    if (parentObject() && parentObject()->isAccessibilityScrollView())
-        return nullptr;
+    if (auto* parent = parentObject()) {
+        if (parent->isScrollView())
+            return nullptr;
+    }
 
     if (!is<RenderBox>(renderer()))
         return nullptr;
index 6f1999c..8928cef 100644 (file)
@@ -50,7 +50,7 @@ private:
     ScrollableArea* getScrollableAreaIfScrollable() const override;
     void scrollTo(const IntPoint&) const override;
     bool computeAccessibilityIsIgnored() const override;
-    bool isAccessibilityScrollView() const override { return true; }
+    bool isAccessibilityScrollViewInstance() const override { return true; }
     bool isEnabled() const override { return true; }
     
     bool isAttachment() const override;
@@ -84,4 +84,4 @@ private:
     
 } // namespace WebCore
 
-SPECIALIZE_TYPE_TRAITS_ACCESSIBILITY(AccessibilityScrollView, isAccessibilityScrollView())
+SPECIALIZE_TYPE_TRAITS_ACCESSIBILITY(AccessibilityScrollView, isAccessibilityScrollViewInstance())
index 80636c7..5dee1fb 100644 (file)
@@ -1666,10 +1666,10 @@ static void appendStringToResult(NSMutableString *result, NSString *string)
     // Mock objects can have their parents detached but still exist in the cache.
     if (self.axBackingObject->isDetachedFromParent())
         return nil;
-    
+
     // The only object without a parent wrapper at this point should be a scroll view.
-    ASSERT(self.axBackingObject->isAccessibilityScrollView());
-    
+    ASSERT(self.axBackingObject->isScrollView());
+
     // Verify this is the top document. If not, we might need to go through the platform widget.
     FrameView* frameView = self.axBackingObject->documentFrameView();
     Document* document = self.axBackingObject->document();
index c3bfa26..ac940d0 100644 (file)
@@ -952,11 +952,9 @@ bool AXIsolatedObject::isAccessibilityScrollbar() const
     return false;
 }
 
-bool AXIsolatedObject::isAccessibilityScrollView() const
+bool AXIsolatedObject::isAccessibilityScrollViewInstance() const
 {
-    // FIXME: this should be ASSERT_NOT_REACHED, but it is called by
-    // AXObjectCache::rootWebArea, that in turn is called by
-    // AXObjectCache::postTextStateChangePlatformNotification.
+    ASSERT_NOT_REACHED();
     return false;
 }
 
index 3476c79..cc99b06 100644 (file)
@@ -714,7 +714,7 @@ private:
     bool isAccessibilityNodeObject() const override;
     bool isAccessibilityRenderObject() const override;
     bool isAccessibilityScrollbar() const override;
-    bool isAccessibilityScrollView() const override;
+    bool isAccessibilityScrollViewInstance() const override;
     bool isAccessibilitySVGRoot() const override;
     bool isAccessibilitySVGElement() const override;
     bool isAccessibilityTableInstance() const override;
index 6229306..c51b185 100644 (file)
@@ -34,6 +34,7 @@
 #import "RenderObject.h"
 #import "WebAccessibilityObjectWrapperMac.h"
 #import <pal/spi/cocoa/NSAccessibilitySPI.h>
+#import <pal/spi/mac/HIServicesSPI.h>
 
 #if USE(APPLE_INTERNAL_SDK)
 #include <ApplicationServices/ApplicationServicesPriv.h>
@@ -403,7 +404,7 @@ void AXObjectCache::postTextStateChangePlatformNotification(AXCoreObject* object
         }
     }
     if (!selection.isNone()) {
-        if (id textMarkerRange = [object->wrapper() textMarkerRangeFromVisiblePositions:selection.visibleStart() endPosition:selection.visibleEnd()])
+        if (id textMarkerRange = textMarkerRangeFromVisiblePositions(this, selection.visibleStart(), selection.visibleEnd()))
             [userInfo setObject:textMarkerRange forKey:NSAccessibilitySelectedTextMarkerRangeAttribute];
     }
 
@@ -541,6 +542,48 @@ void AXObjectCache::platformPerformDeferredCacheUpdate()
 {
 }
 
+// TextMarker utility functions.
+
+static id AXTextMarkerRange(id startMarker, id endMarker)
+{
+    ASSERT(startMarker);
+    ASSERT(endMarker);
+    ASSERT(CFGetTypeID((__bridge CFTypeRef)startMarker) == AXTextMarkerGetTypeID());
+    ASSERT(CFGetTypeID((__bridge CFTypeRef)endMarker) == AXTextMarkerGetTypeID());
+    return CFBridgingRelease(AXTextMarkerRangeCreate(kCFAllocatorDefault, (AXTextMarkerRef)startMarker, (AXTextMarkerRef)endMarker));
+}
+
+id textMarkerRangeFromMarkers(id textMarker1, id textMarker2)
+{
+    if (!textMarker1 || !textMarker2)
+        return nil;
+
+    return AXTextMarkerRange(textMarker1, textMarker2);
+}
+
+id textMarkerForVisiblePosition(AXObjectCache* cache, const VisiblePosition& visiblePos)
+{
+    ASSERT(cache);
+    if (!cache)
+        return nil;
+
+    auto textMarkerData = cache->textMarkerDataForVisiblePosition(visiblePos);
+    if (!textMarkerData)
+        return nil;
+
+    return CFBridgingRelease(AXTextMarkerCreate(kCFAllocatorDefault, (const UInt8*)&textMarkerData.value(), sizeof(textMarkerData.value())));
+}
+
+id textMarkerRangeFromVisiblePositions(AXObjectCache* cache, const VisiblePosition& startPosition, const VisiblePosition& endPosition)
+{
+    if (!cache)
+        return nil;
+
+    id startTextMarker = textMarkerForVisiblePosition(cache, startPosition);
+    id endTextMarker = textMarkerForVisiblePosition(cache, endPosition);
+    return textMarkerRangeFromMarkers(startTextMarker, endTextMarker);
+}
+
 }
 
 #endif // ENABLE(ACCESSIBILITY) && PLATFORM(MAC)
index c19cf21..d3504af 100644 (file)
 @end
 
 #endif // PLATFORM(MAC)
+
+namespace WebCore {
+
+// TextMarker utility functions.
+
+id textMarkerRangeFromMarkers(id textMarker1, id textMarker2);
+id textMarkerForVisiblePosition(AXObjectCache*, const VisiblePosition&);
+id textMarkerRangeFromVisiblePositions(AXObjectCache*, const VisiblePosition&, const VisiblePosition&);
+
+}
+
index 48325c8..dcae488 100644 (file)
@@ -577,15 +577,6 @@ static inline BOOL AXObjectIsTextMarkerRange(id object)
     return object && CFGetTypeID((__bridge CFTypeRef)object) == AXTextMarkerRangeGetTypeID();
 }
 
-static id AXTextMarkerRange(id startMarker, id endMarker)
-{
-    ASSERT(startMarker != nil);
-    ASSERT(endMarker != nil);
-    ASSERT(CFGetTypeID((__bridge CFTypeRef)startMarker) == AXTextMarkerGetTypeID());
-    ASSERT(CFGetTypeID((__bridge CFTypeRef)endMarker) == AXTextMarkerGetTypeID());
-    return CFBridgingRelease(AXTextMarkerRangeCreate(kCFAllocatorDefault, (AXTextMarkerRef)startMarker, (AXTextMarkerRef)endMarker));
-}
-
 static id AXTextMarkerRangeStart(id range)
 {
     ASSERT(range != nil);
@@ -918,19 +909,6 @@ static CharacterOffset characterOffsetForTextMarker(AXObjectCache* cache, CFType
     return characterOffsetForTextMarker(self.axBackingObject->axObjectCache(), (__bridge CFTypeRef)textMarker);
 }
 
-static id textMarkerForVisiblePosition(AXObjectCache* cache, const VisiblePosition& visiblePos)
-{
-    ASSERT(cache);
-    if (!cache)
-        return nil;
-    
-    auto textMarkerData = cache->textMarkerDataForVisiblePosition(visiblePos);
-    if (!textMarkerData)
-        return nil;
-
-    return CFBridgingRelease(AXTextMarkerCreate(kCFAllocatorDefault, (const UInt8*)&textMarkerData.value(), sizeof(textMarkerData.value())));
-}
-
 - (id)textMarkerForVisiblePosition:(const VisiblePosition &)visiblePos
 {
     return textMarkerForVisiblePosition(self.axBackingObject->axObjectCache(), visiblePos);
@@ -977,14 +955,6 @@ static VisiblePosition visiblePositionForEndOfTextMarkerRange(AXObjectCache* cac
     return visiblePositionForTextMarker(cache, (__bridge CFTypeRef)AXTextMarkerRangeEnd(textMarkerRange));
 }
 
-static id textMarkerRangeFromMarkers(id textMarker1, id textMarker2)
-{
-    if (!textMarker1 || !textMarker2)
-        return nil;
-    
-    return AXTextMarkerRange(textMarker1, textMarker2);
-}
-
 // When modifying attributed strings, the range can come from a source which may provide faulty information (e.g. the spell checker).
 // To protect against such cases the range should be validated before adding or removing attributes.
 static BOOL AXAttributedStringRangeIsValid(NSAttributedString *attrString, NSRange range)
@@ -1322,16 +1292,6 @@ static NSString* nsStringForReplacedNode(Node* replacedNode)
     });
 }
 
-static id textMarkerRangeFromVisiblePositions(AXObjectCache* cache, const VisiblePosition& startPosition, const VisiblePosition& endPosition)
-{
-    if (!cache)
-        return nil;
-    
-    id startTextMarker = textMarkerForVisiblePosition(cache, startPosition);
-    id endTextMarker = textMarkerForVisiblePosition(cache, endPosition);
-    return textMarkerRangeFromMarkers(startTextMarker, endTextMarker);
-}
-
 - (id)textMarkerRangeFromVisiblePositions:(const VisiblePosition&)startPosition endPosition:(const VisiblePosition&)endPosition
 {
     auto* backingObject = self.updateObjectBackingStore;