Hitpoint for link which spans two lines in web content is incorrect
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 6 May 2019 22:33:52 +0000 (22:33 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 6 May 2019 22:33:52 +0000 (22:33 +0000)
https://bugs.webkit.org/show_bug.cgi?id=197511
<rdar://problem/49971483>

Patch by Andres Gonzalez <andresg_22@apple.com> 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.

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

LayoutTests/ChangeLog
LayoutTests/accessibility/ios-simulator/links-activation-expected.txt [new file with mode: 0644]
LayoutTests/accessibility/ios-simulator/links-activation.html [new file with mode: 0644]
LayoutTests/accessibility/links-activation-expected.txt [new file with mode: 0644]
LayoutTests/accessibility/links-activation.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/accessibility/AccessibilityRenderObject.cpp
Source/WebCore/accessibility/AccessibilityRenderObject.h
Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm
Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm

index 4474d63..1648c6b 100644 (file)
@@ -1,3 +1,18 @@
+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>
 
         WebAudio Node JS wrappers should not be collected if events can be fired
diff --git a/LayoutTests/accessibility/ios-simulator/links-activation-expected.txt b/LayoutTests/accessibility/ios-simulator/links-activation-expected.txt
new file mode 100644 (file)
index 0000000..14492be
--- /dev/null
@@ -0,0 +1,19 @@
+This is a very long, long, long, long, line that contains a link that expands multiple lines: Apple
+
+Inc. is based in Cupertino California.
+
+
+
+A singleline link: Apple Inc. is based in Cupertino California.
+
+This test ensures that the activation point (or click point) for a link is inside the bounding rect for the link. .
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS clickPointInsideFrame(link) is true
+PASS clickPointInsideFrame(link) is true
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/accessibility/ios-simulator/links-activation.html b/LayoutTests/accessibility/ios-simulator/links-activation.html
new file mode 100644 (file)
index 0000000..eb52158
--- /dev/null
@@ -0,0 +1,59 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src="../../resources/js-test-pre.js"></script>
+<script>
+if (window.testRunner)
+    testRunner.dumpAsText();
+</script>
+</head>
+<body>
+
+<p>This is a very long, long, long, long, line that contains a link that expands multiple lines: <a id="multiline-link" href="#">Apple<br><br>Inc.</a> is based in Cupertino California.
+</p>
+
+<br><br>
+
+<p>A singleline link: <a id="singleline-link" href="#">Apple Inc.</a> is based in Cupertino California.
+</p>
+
+<p id="description"></p>
+<div id="console"></div>
+
+<script>
+    description("This test ensures that the activation point (or click point) for a link is inside the bounding rect for the link. .");
+
+    function clickPointInsideFrame(el) {
+        var clickX = el.clickPointX;
+        var clickY = el.clickPointY;
+
+        var frameX = el.x;
+        var frameY = el.y;
+        var frameW = el.width;
+        var frameH = el.height;
+
+        return (clickX >= frameX && clickX <= frameX + frameW
+                && clickY >= frameY && clickY <= frameY + frameH);
+    };
+
+    if (window.accessibilityController) {
+        window.jsTestIsAsync = true;
+
+        var link = accessibilityController.accessibleElementById("multiline-link");
+        shouldBeTrue("clickPointInsideFrame(link)");
+        link.press();
+
+        link = accessibilityController.accessibleElementById("singleline-link");
+        shouldBeTrue("clickPointInsideFrame(link)");
+        link.press();
+
+        setTimeout(
+            function() {
+                finishJSTest();
+            }, 10);
+    }
+</script>
+
+<script src="../../resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/accessibility/links-activation-expected.txt b/LayoutTests/accessibility/links-activation-expected.txt
new file mode 100644 (file)
index 0000000..faa4dd2
--- /dev/null
@@ -0,0 +1,19 @@
+This is a very long, long, long, long, line that contains a link that expands multiple lines: Apple
+
+Inc. is based in Cupertino California.
+
+
+
+A singleline link: Apple Inc. is based in Cupertino California.
+
+This test ensures that a link is activated with a simulated accessibility press regardless whether its bounding rect encompasses points outside the link. The multiline-link is an example of a bounding rect that contains points that are not part of the link, in particular the middle point of the rect.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+click event [object MouseEvent] for element A with id multiline-link
+click event [object MouseEvent] for element A with id singleline-link
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/accessibility/links-activation.html b/LayoutTests/accessibility/links-activation.html
new file mode 100644 (file)
index 0000000..0d3e929
--- /dev/null
@@ -0,0 +1,51 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src="../resources/js-test-pre.js"></script>
+<script>
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+function handleClick(event) {
+    debug("click event " + event
+          + " for element " + event.target.nodeName
+          + " with id " + event.target.id);
+    return false;
+}
+</script>
+</head>
+<body>
+
+<p>This is a very long, long, long, long, line that contains a link that expands multiple lines: <a id="multiline-link" onclick="return handleClick(event);" href="#">Apple<br><br>Inc.</a> is based in Cupertino California.
+</p>
+
+<br><br>
+
+<p>A singleline link: <a id="singleline-link" onclick="return handleClick(event);" href="#">Apple Inc.</a> is based in Cupertino California.
+</p>
+
+<p id="description"></p>
+<div id="console"></div>
+
+<script>
+    description("This test ensures that a link is activated with a simulated accessibility press regardless whether its bounding rect encompasses points outside the link. The multiline-link is an example of a bounding rect that contains points that are not part of the link, in particular the middle point of the rect.");
+
+    if (window.accessibilityController) {
+        window.jsTestIsAsync = true;
+
+        var link = accessibilityController.accessibleElementById("multiline-link");
+        link.press();
+
+        link = accessibilityController.accessibleElementById("singleline-link");
+        link.press();
+
+        setTimeout(
+            function() {
+                finishJSTest();
+            }, 10);
+    }
+</script>
+
+<script src="../resources/js-test-post.js"></script>
+</body>
+</html>
index db6841e..e57b5ac 100644 (file)
@@ -1,3 +1,28 @@
+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>
 
         Adopt AVStreamDataParser.audiovisualMIMETypes
index 0da4f27..de2a005 100644 (file)
@@ -899,11 +899,38 @@ Path AccessibilityRenderObject::elementPath() const
     return Path();
 }
 
+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.
     if (!isWebArea() || !canSetValueAttribute())
@@ -912,10 +939,7 @@ IntPoint AccessibilityRenderObject::clickPoint()
     VisibleSelection visSelection = selection();
     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) };
 }
     
 AccessibilityObject* AccessibilityRenderObject::internalLinkElement() const
@@ -2042,7 +2066,7 @@ bool AccessibilityRenderObject::nodeIsTextControl(const Node* node) const
     return false;
 }
 
-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;
     ourRect.unite(rect2);
@@ -2054,12 +2078,8 @@ IntRect AccessibilityRenderObject::boundsForRects(LayoutRect& rect1, LayoutRect&
         if (rangeString.length() > 1 && !boundingBox.isEmpty())
             ourRect = boundingBox;
     }
-    
-#if PLATFORM(MAC)
-    return m_renderer->view().frameView().contentsToScreen(snappedIntRect(ourRect));
-#else
+
     return snappedIntRect(ourRect);
-#endif
 }
 
 IntRect AccessibilityRenderObject::boundsForVisiblePositionRange(const VisiblePositionRange& visiblePositionRange) const
index 19ad8fe..b12039a 100644 (file)
@@ -168,7 +168,6 @@ public:
     VisiblePositionRange visiblePositionRangeForLine(unsigned) const override;
     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;
     bool hasPopup() const override;
@@ -292,6 +291,10 @@ private:
 
     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&);
 };
 
 } // namespace WebCore
index 31f2561..c200918 100644 (file)
@@ -1599,10 +1599,9 @@ static void appendStringToResult(NSMutableString *result, NSString *string)
 {
     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];
 }
 
 - (CGRect)accessibilityFrame
index 023c23d..23229cb 100644 (file)
@@ -4259,7 +4259,8 @@ IGNORE_WARNINGS_END
     
     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];
     }
     
@@ -4269,7 +4270,8 @@ IGNORE_WARNINGS_END
         if (start.isNull() || end.isNull())
             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];
     }
     
@@ -4504,7 +4506,8 @@ IGNORE_WARNINGS_END
             if (!rangeSet)
                 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];
         }