Attachment 368923 Details for Bug 197511 – Patch

[patch] Patch

bug-197511-20190503073142.patch (text/plain), 19.36 KB, created by Andres Gonzalez on 2019-05-03 07:31:43 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Andres Gonzalez

Created: 2019-05-03 07:31:43 PDT

Size: 19.36 KB

patch

obsolete

>Subversion Revision: 244751
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 7ad66b58957b19f4e59df73f0d24ee9de0e4cd9b..ed853e809287db948d5f99056627b29d53bdd22b 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,29 @@
>+2019-05-02  Andres Gonzalez  <andresg_22@apple.com>
>+
>+        Title 17A469: Hitpoint for link which spans two lines in web content is incorrect
>+        https://bugs.webkit.org/show_bug.cgi?id=197511
>+        <rdar://problem/49971483> 17A469: Hitpoint for link which spans two lines in web content is incorrect
>+    
>+        - 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.
>+        - Added LayoutTest.
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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-04-29  Youenn Fablet  <youenn@apple.com>
> 
>         getDisplayMedia should be called on user gesture
>diff --git a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp b/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
>index 0da4f27c9e6f6087a35fcf4bb19ff88461c931e4..de2a0052b8345088f4cc1c7b572382b19f847a4c 100644
>--- a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
>+++ b/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
>@@ -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
>diff --git a/Source/WebCore/accessibility/AccessibilityRenderObject.h b/Source/WebCore/accessibility/AccessibilityRenderObject.h
>index 19ad8feddb3601ca9c4660803134f5b5025c7ff6..b12039abea50a8447a9e91acc68966572e754742 100644
>--- a/Source/WebCore/accessibility/AccessibilityRenderObject.h
>+++ b/Source/WebCore/accessibility/AccessibilityRenderObject.h
>@@ -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
>diff --git a/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm b/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm
>index 31f2561cf0855261a20bf10bf09e769d3d8abbe0..c2009186ce53342a9996412d04ca697f19e27519 100644
>--- a/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm
>+++ b/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm
>@@ -1599,10 +1599,9 @@ - (CGPoint)accessibilityActivationPoint
> {
>     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
>diff --git a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
>index 023c23d208a15646219d4c1aa06ecba0eb431c18..23229cb53009e58d82588f9f340ff98fc677295c 100644
>--- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
>+++ b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
>@@ -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];
>         }
>         
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 65b942599120f484a92334d6a84f4293393562d6..ff0b37c29266e739554049e736450acc2aed434a 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,22 @@
>+2019-05-02  Andres Gonzalez  <andresg_22@apple.com>
>+
>+        Title 17A469: Hitpoint for link which spans two lines in web content is incorrect
>+        https://bugs.webkit.org/show_bug.cgi?id=197511
>+        <rdar://problem/49971483> 17A469: Hitpoint for link which spans two lines in web content is incorrect
>+    
>+        - 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.
>+        - Added LayoutTest.
>+
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * 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-04-29  Youenn Fablet  <youenn@apple.com>
> 
>         getDisplayMedia should be called on user gesture
>diff --git a/LayoutTests/accessibility/ios-simulator/links-activation-expected.txt b/LayoutTests/accessibility/ios-simulator/links-activation-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..a73fcfb244f1c7827c545c425a0a3ed165261685
>--- /dev/null
>+++ b/LayoutTests/accessibility/ios-simulator/links-activation-expected.txt
>@@ -0,0 +1,21 @@
>+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".
>+
>+
>+touchStart event [object TouchEvent] for element A with id multiline-link
>+touchEnd event [object TouchEvent] for element A with id multiline-link
>+touchStart event [object TouchEvent] for element A with id singleline-link
>+touchEnd event [object TouchEvent] for element A with id singleline-link
>+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
>index 0000000000000000000000000000000000000000..8db2726707ee751729284dec126e62c44132741d
>--- /dev/null
>+++ b/LayoutTests/accessibility/ios-simulator/links-activation.html
>@@ -0,0 +1,82 @@
>+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
>+<html>
>+<head>
>+<script src="../../resources/js-test-pre.js"></script>
>+<script>
>+if (window.testRunner)
>+   testRunner.dumpAsText();
>+
>+function touchStart(event) {
>+    debug("touchStart event " + event
>+          + " for element " + event.target.nodeName
>+          + " with id " + event.target.id);
>+    event.stopPropagation();
>+    event.preventDefault();
>+}
>+
>+function touchEnd(event) {
>+    debug("touchEnd event " + event
>+          + " for element " + event.target.nodeName
>+          + " with id " + event.target.id);
>+    event.stopPropagation();
>+    event.preventDefault();
>+}
>+
>+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" ontouchstart="touchStart(event);" ontouchend="touchEnd(event);" 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" ontouchstart="touchStart(event);" ontouchend="touchEnd(event);" 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.");
>+
>+    function outputClickAndCenterPoints(el) {
>+        debug("clickX = " + el.clickPointX);
>+        debug("clickY = " + el.clickPointY);
>+
>+        debug("frameX = " + el.x);
>+        debug("frameY = " + el.y);
>+        debug("frameW = " + el.width);
>+        debug("frameH = " + el.height);
>+
>+        debug("centerX = " + (el.x + el.width / 2));
>+        debug("centerY = " + (el.y + el.height / 2));
>+    };
>+
>+    if (window.accessibilityController) {
>+        window.jsTestIsAsync = true;
>+
>+        var link = accessibilityController.accessibleElementById("multiline-link");
>+        outputClickAndCenterPoints(link);
>+        link.press();
>+
>+        link = accessibilityController.accessibleElementById("singleline-link");
>+        outputClickAndCenterPoints(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
>index 0000000000000000000000000000000000000000..faa4dd2c100469f4f1574d5008f25ed4f022faae
>--- /dev/null
>+++ b/LayoutTests/accessibility/links-activation-expected.txt
>@@ -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
>index 0000000000000000000000000000000000000000..a5f1f6460a461b4e385aabe616a0d46e356d3597
>--- /dev/null
>+++ b/LayoutTests/accessibility/links-activation.html
>@@ -0,0 +1,82 @@
>+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
>+<html>
>+<head>
>+<script src="../resources/js-test-pre.js"></script>
>+<script>
>+if (window.testRunner)
>+   testRunner.dumpAsText();
>+
>+function touchStart(event) {
>+    debug("touchStart event " + event
>+          + " for element " + event.target.nodeName
>+          + " with id " + event.target.id);
>+    event.stopPropagation();
>+    event.preventDefault();
>+}
>+
>+function touchEnd(event) {
>+    debug("touchEnd event " + event
>+          + " for element " + event.target.nodeName
>+          + " with id " + event.target.id);
>+    event.stopPropagation();
>+    event.preventDefault();
>+}
>+
>+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" ontouchstart="touchStart(event);" ontouchend="touchEnd(event);" 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" ontouchstart="touchStart(event);" ontouchend="touchEnd(event);" 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.");
>+
>+    function outputClickAndCenterPoints(el) {
>+        debug("clickX = " + el.clickPointX);
>+        debug("clickY = " + el.clickPointY);
>+
>+        debug("frameX = " + el.x);
>+        debug("frameY = " + el.y);
>+        debug("frameW = " + el.width);
>+        debug("frameH = " + el.height);
>+
>+        debug("centerX = " + (el.x + el.width / 2));
>+        debug("centerY = " + (el.y + el.height / 2));
>+    };
>+
>+    if (window.accessibilityController) {
>+        window.jsTestIsAsync = true;
>+
>+        var link = accessibilityController.accessibleElementById("multiline-link");
>+        outputClickAndCenterPoints(link);
>+        link.press();
>+
>+        link = accessibilityController.accessibleElementById("singleline-link");
>+        outputClickAndCenterPoints(link);
>+        link.press();
>+
>+        setTimeout(
>+            function() {
>+                finishJSTest();
>+            }, 10);
>+    }
>+</script>
>+
>+<script src="../resources/js-test-post.js"></script>
>+</body>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 197511: 368773 | 368785 | 368810 | 368870 | 368873 | 368879 | 368887 | 368889 | 368894 | 368923 | 368928 | 368930 | 368931 | 368934 | 368935 | 368945 | 368954 | 368956 | 368957 | 368960 | 368963 | 368964 | 368966 | 368967 | 368971 | 368973 | 368982 | 368993 | 369006 | 369008 | 369019 | 369020 | 369022 | 369097 | 369114 | 370160 | 370162