Changeset 244983 in webkit

Timestamp:

May 6, 2019 3:33:52 PM (5 years ago)

Author:

commit-queue@webkit.org

Message:

Hitpoint for link which spans two lines in web content is incorrect
​https://bugs.webkit.org/show_bug.cgi?id=197511
<rdar://problem/49971483>

Patch by Andres Gonzalez <Andres Gonzalez> on 2019-05-06
Reviewed by Chris Fleizach.

Source/WebCore:

• Special case for links to return first char location as clickPoint instead of middle point of bounding rect.
• Modified iOS ActivationPoint to use clickPoint. This way all code paths go through the same function.
• Made boundsForRects to return content coordinates in all platforms. Adjusted all callers, directly or indirectly, appropriately.

Tests: accessibility/ios-simulator/links-activation.html

accessibility/links-activation.html

• accessibility/AccessibilityRenderObject.cpp:

(WebCore::AccessibilityRenderObject::clickPoint):
(WebCore::AccessibilityRenderObject::boundsForRects):
(WebCore::AccessibilityRenderObject::boundsForRects const): Deleted.

• accessibility/AccessibilityRenderObject.h:
• accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:

(-[WebAccessibilityObjectWrapper accessibilityActivationPoint]):

• accessibility/mac/WebAccessibilityObjectWrapperMac.mm:

(-[WebAccessibilityObjectWrapper accessibilityAttributeValue:forParameter:]):

LayoutTests:

• Added LayoutTest.

• accessibility/ios-simulator/links-activation-expected.txt: Added.
• accessibility/ios-simulator/links-activation.html: Added.
• accessibility/links-activation-expected.txt: Added.
• accessibility/links-activation.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/accessibility/ios-simulator/links-activation-expected.txt (added)
• LayoutTests/accessibility/ios-simulator/links-activation.html (added)
• LayoutTests/accessibility/links-activation-expected.txt (added)
• LayoutTests/accessibility/links-activation.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/accessibility/AccessibilityRenderObject.cpp (modified) (4 diffs)
• Source/WebCore/accessibility/AccessibilityRenderObject.h (modified) (2 diffs)
• Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm (modified) (1 diff)
• Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm (modified) (3 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2019-05-06  Andres Gonzalez  <andresg_22@apple.com>
+
+        Hitpoint for link which spans two lines in web content is incorrect
+        https://bugs.webkit.org/show_bug.cgi?id=197511
+        <rdar://problem/49971483>
+
+        Reviewed by Chris Fleizach.
+
+        - Added LayoutTest.
+
+        * accessibility/ios-simulator/links-activation-expected.txt: Added.
+        * accessibility/ios-simulator/links-activation.html: Added.
+        * accessibility/links-activation-expected.txt: Added.
+        * accessibility/links-activation.html: Added.
+
 2019-05-06  Youenn Fablet  <youenn@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-05-06  Andres Gonzalez  <andresg_22@apple.com>
+
+        Hitpoint for link which spans two lines in web content is incorrect
+        https://bugs.webkit.org/show_bug.cgi?id=197511
+        <rdar://problem/49971483>
+
+        Reviewed by Chris Fleizach.
+
+        - Special case for links to return first char location as clickPoint instead of middle point of bounding rect.
+        - Modified iOS ActivationPoint to use clickPoint. This way all code paths go through the same function.
+        - Made boundsForRects to return content coordinates in all platforms. Adjusted all callers, directly or indirectly, appropriately.
+
+        Tests: accessibility/ios-simulator/links-activation.html
+               accessibility/links-activation.html
+
+        * accessibility/AccessibilityRenderObject.cpp:
+        (WebCore::AccessibilityRenderObject::clickPoint):
+        (WebCore::AccessibilityRenderObject::boundsForRects):
+        (WebCore::AccessibilityRenderObject::boundsForRects const): Deleted.
+        * accessibility/AccessibilityRenderObject.h:
+        * accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:
+        (-[WebAccessibilityObjectWrapper accessibilityActivationPoint]):
+        * accessibility/mac/WebAccessibilityObjectWrapperMac.mm:
+        (-[WebAccessibilityObjectWrapper accessibilityAttributeValue:forParameter:]):
+
 2019-05-06  Jer Noble  <jer.noble@apple.com>
 

trunk/Source/WebCore/accessibility/AccessibilityRenderObject.cpp

@@ -900 +900 @@
 }
 
+IntPoint AccessibilityRenderObject::linkClickPoint()
+{
+    ASSERT(isLink());
+    /* A link bounding rect can contain points that are not part of the link.
+     For instance, a link that starts at the end of a line and finishes at the
+     beginning of the next line will have a bounding rect that includes the
+     entire two lines. In such a case, the middle point of the bounding rect
+     may not belong to the link element and thus may not activate the link.
+     Hence, return the middle point of the first character in the link if exists.
+     */
+    if (RefPtr<Range> range = elementRange()) {
+        VisiblePosition start = range->startPosition();
+        VisiblePosition end = nextVisiblePosition(start);
+        if (start.isNull() || !range->contains(end))
+            return AccessibilityObject::clickPoint();
+
+        RefPtr<Range> charRange = makeRange(start, end);
+        IntRect rect = boundsForRange(charRange);
+        return { rect.x() + rect.width() / 2, rect.y() + rect.height() / 2 };
+    }
+    return AccessibilityObject::clickPoint();
+}
+
 IntPoint AccessibilityRenderObject::clickPoint()
 {
     // Headings are usually much wider than their textual content. If the mid point is used, often it can be wrong.
-    if (isHeading() && children().size() == 1)
-        return children()[0]->clickPoint();
+    AccessibilityChildrenVector children = this->children();
+    if (isHeading() && children.size() == 1)
+        return children[0]->clickPoint();
+
+    if (isLink())
+        return linkClickPoint();
 
     // use the default position unless this is an editable web area, in which case we use the selection bounds.
@@ -913 +940 @@
     VisiblePositionRange range = VisiblePositionRange(visSelection.visibleStart(), visSelection.visibleEnd());
     IntRect bounds = boundsForVisiblePositionRange(range);
-#if PLATFORM(COCOA)
-    bounds.setLocation(m_renderer->view().frameView().screenToContents(bounds.location()));
-#endif        
-    return IntPoint(bounds.x() + (bounds.width() / 2), bounds.y() - (bounds.height() / 2));
+    return { bounds.x() + (bounds.width() / 2), bounds.y() + (bounds.height() / 2) };
 }
     
@@ -2043 +2067 @@
 }
 
-IntRect AccessibilityRenderObject::boundsForRects(LayoutRect& rect1, LayoutRect& rect2, RefPtr<Range> dataRange) const
+IntRect AccessibilityRenderObject::boundsForRects(LayoutRect const& rect1, LayoutRect const& rect2, RefPtr<Range> const& dataRange)
 {
     LayoutRect ourRect = rect1;
@@ -2055 +2079 @@
             ourRect = boundingBox;
     }
-    
-#if PLATFORM(MAC)
-    return m_renderer->view().frameView().contentsToScreen(snappedIntRect(ourRect));
-#else
+
     return snappedIntRect(ourRect);
-#endif
 }
 

trunk/Source/WebCore/accessibility/AccessibilityRenderObject.h

@@ -169 +169 @@
     IntRect boundsForVisiblePositionRange(const VisiblePositionRange&) const override;
     IntRect boundsForRange(const RefPtr<Range>) const override;
-    IntRect boundsForRects(LayoutRect&, LayoutRect&, RefPtr<Range>) const;
     void setSelectedVisiblePositionRange(const VisiblePositionRange&) const override;
     bool isVisiblePositionRangeInDifferentDocument(const VisiblePositionRange&) const;
@@ -293 +292 @@
     RenderObject* targetElementForActiveDescendant(const QualifiedName&, AccessibilityObject*) const;
     bool canHavePlainText() const;
+    // Special handling of click point for links.
+    IntPoint linkClickPoint();
+    // Rects utilities.
+    static IntRect boundsForRects(LayoutRect const&, LayoutRect const&, RefPtr<Range> const&);
 };
 

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

@@ -1600 +1600 @@
     if (![self _prepareAccessibilityCall])
         return CGPointZero;
-    
-    auto rect = FloatRect(snappedIntRect(m_object->boundingBoxRect()));
-    CGRect cgRect = [self convertRectToSpace:rect space:AccessibilityConversionSpace::Screen];
-    return CGPointMake(CGRectGetMidX(cgRect), CGRectGetMidY(cgRect));
+
+    IntPoint point = m_object->clickPoint();
+    return [self _accessibilityConvertPointToViewSpace:point];
 }
 

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

@@ -4260 +4260 @@
     if ([attribute isEqualToString:@"AXBoundsForTextMarkerRange"]) {
         RefPtr<Range> range = [self rangeForTextMarkerRange:textMarkerRange];
-        NSRect rect = m_object->boundsForRange(range);
+        auto bounds = FloatRect(m_object->boundsForRange(range));
+        NSRect rect = [self convertRectToSpace:bounds space:AccessibilityConversionSpace::Screen];
         return [NSValue valueWithRect:rect];
     }
@@ -4270 +4271 @@
             return nil;
         RefPtr<Range> range = cache->rangeForUnorderedCharacterOffsets(start, end);
-        NSRect rect = m_object->boundsForRange(range);
+        auto bounds = FloatRect(m_object->boundsForRange(range));
+        NSRect rect = [self convertRectToSpace:bounds space:AccessibilityConversionSpace::Screen];
         return [NSValue valueWithRect:rect];
     }
@@ -4505 +4507 @@
                 return nil;
             PlainTextRange plainTextRange = PlainTextRange(range.location, range.length);
-            NSRect rect = m_object->doAXBoundsForRangeUsingCharacterOffset(plainTextRange);
+            auto bounds = FloatRect(m_object->doAXBoundsForRangeUsingCharacterOffset(plainTextRange));
+            NSRect rect = [self convertRectToSpace:bounds space:AccessibilityConversionSpace::Screen];
             return [NSValue valueWithRect:rect];
         }