Delete Node::boundingBox()
authormmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 9 Dec 2014 21:03:54 +0000 (21:03 +0000)
committermmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 9 Dec 2014 21:03:54 +0000 (21:03 +0000)
https://bugs.webkit.org/show_bug.cgi?id=139333

Source/WebCore:

Conceptually, boundingBox() should be on RenderInline. In addition,
Node::boundingBox() is completely broken for inline elements: it
makes a rect from the top left of the first inline child to the
bottom right of the last inline child, disregarding the intermediate
inline children. This breaks with vertical text and with line
breaks.

What makes this problem worse is that some functions actually rely
on this bad behavior. These functions are functions that use the
Node's so-called "bounding box" to scroll to an anchor tag.

This patch goes through all the call sites of Node::boundingBox(),
and segregates them into calls that expect the true bounding box
and calls that need this false bounding box. This patch then moves
this false bounding box into RenderElement, using the name
anchorRect(). Callers what want the correct bounding box have been
updated to use RenderElement::absoluteBoundingBoxRect().

Reviewed by Zalan Bujtas.

No new tests because there should be no behavior change.

* accessibility/AccessibilitySlider.cpp:
(WebCore::AccessibilitySliderThumb::elementRect): Use
RenderObject::absoluteBoundingBoxRect()
* dom/ContainerNode.cpp:
(WebCore::ContainerNode::getUpperLeftCorner): Deleted.
(WebCore::ContainerNode::getLowerRightCorner): Deleted.
(WebCore::ContainerNode::boundingBox): Deleted.
* dom/ContainerNode.h:
* dom/Element.cpp:
(WebCore::Element::scrollIntoView): Use RenderElement::anchorRect().
(WebCore::Element::scrollIntoViewIfNeeded): Ditto.
(WebCore::Element::updateFocusAppearance): Ditto.
* dom/Node.cpp:
(WebCore::Node::boundingBox): Deleted.
* dom/Node.h:
(WebCore::Node::pixelSnappedBoundingBox): Deleted.
* html/ColorInputType.cpp:
(WebCore::ColorInputType::elementRectRelativeToRootView): Use
RenderObject::absoluteBoundingBoxRect()
* html/HTMLInputElement.cpp:
(WebCore::HTMLInputElement::setupDateTimeChooserParameters): Ditto.
* html/ValidationMessage.cpp:
(WebCore::ValidationMessage::buildBubbleTree): Ditto.
* page/FrameView.cpp:
(WebCore::FrameView::scrollElementToRect): Use
RenderElement::anchorRect().
(WebCore::FrameView::scrollToAnchor): Ditto.
* page/SpatialNavigation.cpp:
(WebCore::nodeRectInAbsoluteCoordinates): Use
RenderObject::absoluteBoundingBoxRect()
* rendering/RenderElement.cpp:
(WebCore::RenderElement::getUpperLeftCorner): Moved from ContainerNode.
(WebCore::RenderElement::getLowerRightCorner): Moved from
ContainerNode.
(WebCore::RenderElement::anchorRect): Moved from ContainerNode.
* rendering/RenderObject.h:
* rendering/RenderText.cpp:
(WebCore::RenderText::topOfFirstText): Helper for
RenderElement::anchorRect()
* rendering/RenderText.h:

Source/WebKit/mac:

Reviewed by Zalan Bujtas.

* WebView/WebActionMenuController.mm:
(elementBoundingBoxInWindowCoordinatesFromNode): Use
RenderObject::absoluteBoundingBoxRect().

Source/WebKit2:

Reviewed by Zalan Bujtas.

* Shared/WebHitTestResult.cpp:
(WebKit::WebHitTestResult::Data::elementBoundingBoxInWindowCoordinates):
Use RenderObject::absoluteBoundingBoxRect().
* WebProcess/WebPage/CoordinatedGraphics/WebPageCoordinatedGraphics.cpp:
(WebKit::WebPage::findZoomableAreaForPoint): Use
RenderObject::absoluteBoundingBoxRect().

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

28 files changed:
LayoutTests/fast/spatial-navigation/snav-container-white-space-expected.txt
LayoutTests/fast/spatial-navigation/snav-container-white-space.html
LayoutTests/fast/spatial-navigation/snav-imagemap-overlapped-areas-expected.txt
LayoutTests/fast/spatial-navigation/snav-imagemap-overlapped-areas.html
Source/WebCore/ChangeLog
Source/WebCore/WebCore.exp.in
Source/WebCore/accessibility/AccessibilitySlider.cpp
Source/WebCore/dom/ContainerNode.cpp
Source/WebCore/dom/ContainerNode.h
Source/WebCore/dom/Element.cpp
Source/WebCore/dom/Node.cpp
Source/WebCore/dom/Node.h
Source/WebCore/html/ColorInputType.cpp
Source/WebCore/html/HTMLInputElement.cpp
Source/WebCore/html/ValidationMessage.cpp
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/page/FrameView.cpp
Source/WebCore/page/FrameView.h
Source/WebCore/page/SpatialNavigation.cpp
Source/WebCore/rendering/RenderElement.cpp
Source/WebCore/rendering/RenderElement.h
Source/WebCore/rendering/RenderText.cpp
Source/WebCore/rendering/RenderText.h
Source/WebKit/mac/ChangeLog
Source/WebKit/mac/WebView/WebActionMenuController.mm
Source/WebKit2/ChangeLog
Source/WebKit2/Shared/WebHitTestResult.cpp
Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/WebPageCoordinatedGraphics.cpp

index 4e29a01ff4575bdfcf5a62b46ac28e3205167958..9032ce8712cfee10e5f7df490f354e364c5a3609 100644 (file)
@@ -1,5 +1,6 @@
 This is an element
  
+
 This is an element
 This is an element
   
index 45932524402d1c72223657e39dfe112bbd6d8ef2..e97f3f8cf4edb598551f3a7b5bbd1e2f54b70720 100644 (file)
@@ -72,7 +72,7 @@
         <img src="resources/green.png" height="42" width="76"  border="0"/>
       </a>
     </div>
-
+    <br/>
     <div>
       <a href="#" id="2">This is an element</a><br>
     </div>
index bb3b35d9562611bc226a9a9f6f7187b96c5b93f9..b26fd4cfea761e1bc50fbc231c23d6eaa9dfe65e 100644 (file)
@@ -1,4 +1,4 @@
+  
 
 
 PASS gFocusedDocument.activeElement.getAttribute("id") is "4"
index 2b110916455590399331031b8ba97d94ec08ef89..adaf7504421853eb8de75efbfb3478b60e0f8d0d 100644 (file)
@@ -57,6 +57,7 @@
   </map>
 
     <a id="start" href="a"><img src="resources/green.png" width=50px height=50px></a>
+    <br>
     <div>
       <img src="resources/green.png" width=200px height=200px usemap="#map">
     </div>
index b61ab1c10cb2c0458d89d5658a881f2d06671bf4..5b266684f9e5a779dafa9ed22010a963ca4d0a2a 100644 (file)
@@ -1,3 +1,71 @@
+2014-12-09  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        Delete Node::boundingBox()
+        https://bugs.webkit.org/show_bug.cgi?id=139333
+
+        Conceptually, boundingBox() should be on RenderInline. In addition,
+        Node::boundingBox() is completely broken for inline elements: it
+        makes a rect from the top left of the first inline child to the
+        bottom right of the last inline child, disregarding the intermediate
+        inline children. This breaks with vertical text and with line
+        breaks.
+
+        What makes this problem worse is that some functions actually rely
+        on this bad behavior. These functions are functions that use the
+        Node's so-called "bounding box" to scroll to an anchor tag.
+
+        This patch goes through all the call sites of Node::boundingBox(),
+        and segregates them into calls that expect the true bounding box
+        and calls that need this false bounding box. This patch then moves
+        this false bounding box into RenderElement, using the name
+        anchorRect(). Callers what want the correct bounding box have been
+        updated to use RenderElement::absoluteBoundingBoxRect().
+
+        Reviewed by Zalan Bujtas.
+
+        No new tests because there should be no behavior change.
+
+        * accessibility/AccessibilitySlider.cpp:
+        (WebCore::AccessibilitySliderThumb::elementRect): Use
+        RenderObject::absoluteBoundingBoxRect()
+        * dom/ContainerNode.cpp:
+        (WebCore::ContainerNode::getUpperLeftCorner): Deleted.
+        (WebCore::ContainerNode::getLowerRightCorner): Deleted.
+        (WebCore::ContainerNode::boundingBox): Deleted.
+        * dom/ContainerNode.h:
+        * dom/Element.cpp:
+        (WebCore::Element::scrollIntoView): Use RenderElement::anchorRect().
+        (WebCore::Element::scrollIntoViewIfNeeded): Ditto.
+        (WebCore::Element::updateFocusAppearance): Ditto.
+        * dom/Node.cpp:
+        (WebCore::Node::boundingBox): Deleted.
+        * dom/Node.h:
+        (WebCore::Node::pixelSnappedBoundingBox): Deleted.
+        * html/ColorInputType.cpp:
+        (WebCore::ColorInputType::elementRectRelativeToRootView): Use
+        RenderObject::absoluteBoundingBoxRect()
+        * html/HTMLInputElement.cpp:
+        (WebCore::HTMLInputElement::setupDateTimeChooserParameters): Ditto.
+        * html/ValidationMessage.cpp:
+        (WebCore::ValidationMessage::buildBubbleTree): Ditto.
+        * page/FrameView.cpp:
+        (WebCore::FrameView::scrollElementToRect): Use
+        RenderElement::anchorRect().
+        (WebCore::FrameView::scrollToAnchor): Ditto.
+        * page/SpatialNavigation.cpp:
+        (WebCore::nodeRectInAbsoluteCoordinates): Use
+        RenderObject::absoluteBoundingBoxRect()
+        * rendering/RenderElement.cpp:
+        (WebCore::RenderElement::getUpperLeftCorner): Moved from ContainerNode.
+        (WebCore::RenderElement::getLowerRightCorner): Moved from
+        ContainerNode.
+        (WebCore::RenderElement::anchorRect): Moved from ContainerNode.
+        * rendering/RenderObject.h:
+        * rendering/RenderText.cpp:
+        (WebCore::RenderText::topOfFirstText): Helper for
+        RenderElement::anchorRect()
+        * rendering/RenderText.h:
+
 2014-12-09  Antti Koivisto  <antti@apple.com>
 
         REGRESSION (r173272): When open PDF from Safari in iBooks, title is replaced to “QuickLookPDF-s72DbgAU-1”
index d90185fc0214d3b6f56a7722e63b5d0816078ebf..c9c0466a34e2a3865548d50a705cce67253f4c18 100644 (file)
@@ -2320,6 +2320,7 @@ __ZN7WebCore19TextIndicatorWindow16setTextIndicatorEN3WTF10PassRefPtrINS_13TextI
 __ZN7WebCore19TextIndicatorWindowC1EP6NSView
 __ZN7WebCore19TextIndicatorWindowD1Ev
 __ZN7WebCore19applicationIsSafariEv
+__ZN7WebCore19rendererBoundingBoxERKNS_4NodeE
 __ZN7WebCore20PlatformEventFactory24createPlatformMouseEventEP7NSEventP6NSView
 __ZN7WebCore20PlatformEventFactory27createPlatformKeyboardEventEP7NSEvent
 __ZN7WebCore20ResourceHandleClient22willCacheResponseAsyncEPNS_14ResourceHandleEP19NSCachedURLResponse
index 91aad3f5623809bf8bfbbab9e0eafb82a1ee7ffe..4d235a9a052f3a2f7656adb620206321745d7673 100644 (file)
@@ -159,7 +159,7 @@ LayoutRect AccessibilitySliderThumb::elementRect() const
     RenderObject* sliderRenderer = m_parent->renderer();
     if (!sliderRenderer || !sliderRenderer->isSlider())
         return LayoutRect();
-    return downcast<HTMLInputElement>(sliderRenderer->node())->sliderThumbElement()->boundingBox();
+    return rendererBoundingBox(*downcast<HTMLInputElement>(sliderRenderer->node())->sliderThumbElement());
 }
 
 bool AccessibilitySliderThumb::computeAccessibilityIsIgnored() const
index 3f63aa9832d3fc64f69b136d6a010c637b953c31..ab4b1a3454e0bde00493826bcef53b6628e2f6ff 100644 (file)
@@ -777,126 +777,6 @@ void ContainerNode::cloneChildNodes(ContainerNode *clone)
     }
 }
 
-bool ContainerNode::getUpperLeftCorner(FloatPoint& point) const
-{
-    if (!renderer())
-        return false;
-    // What is this code really trying to do?
-    RenderObject* o = renderer();
-    if (!o->isInline() || o->isReplaced()) {
-        point = o->localToAbsolute(FloatPoint(), UseTransforms);
-        return true;
-    }
-
-    // find the next text/image child, to get a position
-    while (o) {
-        RenderObject* p = o;
-        if (RenderObject* child = o->firstChildSlow())
-            o = child;
-        else if (o->nextSibling())
-            o = o->nextSibling();
-        else {
-            RenderObject* next = 0;
-            while (!next && o->parent()) {
-                o = o->parent();
-                next = o->nextSibling();
-            }
-            o = next;
-
-            if (!o)
-                break;
-        }
-        ASSERT(o);
-
-        if (!o->isInline() || o->isReplaced()) {
-            point = o->localToAbsolute(FloatPoint(), UseTransforms);
-            return true;
-        }
-
-        if (p->node() && p->node() == this && is<RenderText>(*o) && !downcast<RenderText>(*o).firstTextBox()) {
-            // do nothing - skip unrendered whitespace that is a child or next sibling of the anchor
-        } else if (is<RenderText>(*o) || o->isReplaced()) {
-            point = FloatPoint();
-            if (is<RenderText>(*o) && downcast<RenderText>(*o).firstTextBox())
-                point.move(downcast<RenderText>(*o).linesBoundingBox().x(), downcast<RenderText>(*o).firstTextBox()->root().lineTop());
-            else if (is<RenderBox>(*o))
-                point.moveBy(downcast<RenderBox>(*o).location());
-            point = o->container()->localToAbsolute(point, UseTransforms);
-            return true;
-        }
-    }
-    
-    // If the target doesn't have any children or siblings that could be used to calculate the scroll position, we must be
-    // at the end of the document. Scroll to the bottom. FIXME: who said anything about scrolling?
-    if (!o && document().view()) {
-        point = FloatPoint(0, document().view()->contentsHeight());
-        return true;
-    }
-    return false;
-}
-
-bool ContainerNode::getLowerRightCorner(FloatPoint& point) const
-{
-    if (!renderer())
-        return false;
-
-    RenderObject* o = renderer();
-    if (!o->isInline() || o->isReplaced()) {
-        point = o->localToAbsolute(LayoutPoint(downcast<RenderBox>(*o).size()), UseTransforms);
-        return true;
-    }
-
-    // find the last text/image child, to get a position
-    while (o) {
-        if (RenderObject* child = o->lastChildSlow())
-            o = child;
-        else if (o->previousSibling())
-            o = o->previousSibling();
-        else {
-            RenderObject* prev = 0;
-            while (!prev) {
-                o = o->parent();
-                if (!o)
-                    return false;
-                prev = o->previousSibling();
-            }
-            o = prev;
-        }
-        ASSERT(o);
-        if (is<RenderText>(*o) || o->isReplaced()) {
-            point = FloatPoint();
-            if (is<RenderText>(*o)) {
-                IntRect linesBox = downcast<RenderText>(*o).linesBoundingBox();
-                if (!linesBox.maxX() && !linesBox.maxY())
-                    continue;
-                point.moveBy(linesBox.maxXMaxYCorner());
-            } else
-                point.moveBy(downcast<RenderBox>(*o).frameRect().maxXMaxYCorner());
-            point = o->container()->localToAbsolute(point, UseTransforms);
-            return true;
-        }
-    }
-    return true;
-}
-
-LayoutRect ContainerNode::boundingBox() const
-{
-    FloatPoint upperLeft, lowerRight;
-    bool foundUpperLeft = getUpperLeftCorner(upperLeft);
-    bool foundLowerRight = getLowerRightCorner(lowerRight);
-    
-    // If we've found one corner, but not the other,
-    // then we should just return a point at the corner that we did find.
-    if (foundUpperLeft != foundLowerRight) {
-        if (foundUpperLeft)
-            lowerRight = upperLeft;
-        else
-            upperLeft = lowerRight;
-    } 
-
-    return enclosingLayoutRect(FloatRect(upperLeft, lowerRight.expandedTo(upperLeft) - upperLeft));
-}
-
 unsigned ContainerNode::countChildNodes() const
 {
     unsigned count = 0;
@@ -1030,4 +910,11 @@ RefPtr<RadioNodeList> ContainerNode::radioNodeList(const AtomicString& name)
     return ensureRareData().ensureNodeLists().addCacheWithAtomicName<RadioNodeList>(*this, name);
 }
 
+LayoutRect rendererAnchorRect(const ContainerNode& node)
+{
+    if (RenderElement* renderer = node.renderer())
+        return renderer->anchorRect();
+    return LayoutRect();
+}
+
 } // namespace WebCore
index 07d74bcd407739fe720e02ae0c653905de751073..944c33d805ff43b92a31659651060a84eb3d1773 100644 (file)
@@ -112,8 +112,6 @@ public:
 
     void cloneChildNodes(ContainerNode* clone);
 
-    virtual LayoutRect boundingBox() const override;
-
     enum ChildChangeType { ElementInserted, ElementRemoved, TextInserted, TextRemoved, TextChanged, AllChildrenRemoved, NonContentsChildChanged };
     enum ChildChangeSource { ChildChangeSourceParser, ChildChangeSourceAPI };
     struct ChildChange {
@@ -157,9 +155,6 @@ private:
     void removeBetween(Node* previousChild, Node* nextChild, Node& oldChild);
     void insertBeforeCommon(Node& nextChild, Node& oldChild);
 
-    bool getUpperLeftCorner(FloatPoint&) const;
-    bool getLowerRightCorner(FloatPoint&) const;
-
     void notifyChildInserted(Node& child, ChildChangeSource);
     void notifyChildRemoved(Node& child, Node* previousSibling, Node* nextSibling, ChildChangeSource);
 
@@ -309,6 +304,8 @@ private:
     ChildNodesLazySnapshot* m_nextSnapshot;
 };
 
+LayoutRect rendererAnchorRect(const ContainerNode&);
+
 } // namespace WebCore
 
 SPECIALIZE_TYPE_TRAITS_BEGIN(WebCore::ContainerNode)
index 252fbf0851588c44e08f0f2bbdb0753877e3965e..8bad222f641345fa9e75a156cb12d83eed836505 100644 (file)
@@ -572,7 +572,7 @@ void Element::scrollIntoView(bool alignToTop)
     if (!renderer())
         return;
 
-    LayoutRect bounds = boundingBox();
+    LayoutRect bounds = renderer()->anchorRect();
     // Align to the top / bottom and to the closest edge.
     if (alignToTop)
         renderer()->scrollRectToVisible(bounds, ScrollAlignment::alignToEdgeIfNeeded, ScrollAlignment::alignTopAlways);
@@ -587,7 +587,7 @@ void Element::scrollIntoViewIfNeeded(bool centerIfNeeded)
     if (!renderer())
         return;
 
-    LayoutRect bounds = boundingBox();
+    LayoutRect bounds = renderer()->anchorRect();
     if (centerIfNeeded)
         renderer()->scrollRectToVisible(bounds, ScrollAlignment::alignCenterIfNeeded, ScrollAlignment::alignCenterIfNeeded);
     else
@@ -1978,7 +1978,7 @@ void Element::updateFocusAppearance(bool /*restorePreviousSelection*/)
             frame->selection().revealSelection();
         }
     } else if (renderer() && !renderer()->isWidget())
-        renderer()->scrollRectToVisible(boundingBox());
+        renderer()->scrollRectToVisible(renderer()->anchorRect());
 }
 
 void Element::blur()
index c02bb125def3fe51a8fe7960c28bab7815f0d2bc..3e199c0803d9f6fa1af3fcb063935b95e8f36161 100644 (file)
@@ -637,13 +637,6 @@ RenderBoxModelObject* Node::renderBoxModelObject() const
     RenderObject* renderer = this->renderer();
     return is<RenderBoxModelObject>(renderer) ? downcast<RenderBoxModelObject>(renderer) : nullptr;
 }
-
-LayoutRect Node::boundingBox() const
-{
-    if (renderer())
-        return renderer()->absoluteBoundingBoxRect();
-    return LayoutRect();
-}
     
 LayoutRect Node::renderRect(bool* isReplaced)
 {    
@@ -2266,6 +2259,13 @@ bool Node::inRenderedDocument() const
     return inDocument() && document().hasLivingRenderTree();
 }
 
+IntRect rendererBoundingBox(const Node& node)
+{
+    if (RenderObject* renderer = node.renderer())
+        return renderer->absoluteBoundingBoxRect();
+    return IntRect();
+}
+
 } // namespace WebCore
 
 #ifndef NDEBUG
index e8f529b0c04549e66d192db81136f85ff8594e24..ac1ecf4e12e3a3087d5150649f38d3811d216fa3 100644 (file)
@@ -377,8 +377,6 @@ public:
         return false;
     }
 
-    virtual LayoutRect boundingBox() const;
-    IntRect pixelSnappedBoundingBox() const { return snappedIntRect(boundingBox()); }
     WEBCORE_EXPORT LayoutRect renderRect(bool* isReplaced);
     IntRect pixelSnappedRenderRect(bool* isReplaced) { return snappedIntRect(renderRect(isReplaced)); }
 
@@ -739,6 +737,8 @@ inline ContainerNode* Node::parentNodeGuaranteedHostFree() const
     return parentNode();
 }
 
+WEBCORE_EXPORT IntRect rendererBoundingBox(const Node&);
+
 } // namespace WebCore
 
 #ifndef NDEBUG
index d70e004d49a8e273036a8eb25661aff069c9b118..e6cf05208289cd062fd17808f7880e13cfad363f 100644 (file)
@@ -204,7 +204,7 @@ HTMLElement* ColorInputType::shadowColorSwatch() const
 
 IntRect ColorInputType::elementRectRelativeToRootView() const
 {
-    return element().document().view()->contentsToRootView(element().pixelSnappedBoundingBox());
+    return element().document().view()->contentsToRootView(rendererBoundingBox(element()));
 }
 
 Color ColorInputType::currentColor()
index 972ac5b8ff369cb04243636d15f74eff5ea75c12..c2feae7789af8e9f8db99f0fbcc2c5954f0c6157 100644 (file)
@@ -1877,7 +1877,7 @@ bool HTMLInputElement::setupDateTimeChooserParameters(DateTimeChooserParameters&
         parameters.stepBase = 0;
     }
 
-    parameters.anchorRectInRootView = document().view()->contentsToRootView(pixelSnappedBoundingBox());
+    parameters.anchorRectInRootView = document().view()->contentsToRootView(rendererBoundingBox(*this));
     parameters.currentValue = value();
     parameters.isAnchorElementRTL = computedStyle()->direction() == RTL;
 #if ENABLE(DATALIST_ELEMENT)
index 8fbc0e893eb9642fcedf304c95695fcfdf50f6ec..cb4e7118978029c0eb84e4402c12f35b445c44c9 100644 (file)
@@ -178,7 +178,7 @@ void ValidationMessage::buildBubbleTree()
     m_bubble->setInlineStyleProperty(CSSPropertyPosition, CSSValueAbsolute);
     shadowRoot.appendChild(m_bubble.get(), ASSERT_NO_EXCEPTION);
     document.updateLayout();
-    adjustBubblePosition(m_element->boundingBox(), m_bubble.get());
+    adjustBubblePosition(rendererBoundingBox(*m_element), m_bubble.get());
 
     RefPtr<HTMLDivElement> clipper = HTMLDivElement::create(document);
     clipper->setPseudo(AtomicString("-webkit-validation-bubble-arrow-clipper", AtomicString::ConstructFromLiteral));
index 4dbb763c9749fffc1b6292df78c3c34f5636169c..d5b4cf23dcbb451074c0ab7bd0325c0a7101b22b 100644 (file)
@@ -1148,7 +1148,7 @@ void FrameLoader::completed()
         parent->loader().checkCompleted();
 
     if (m_frame.view())
-        m_frame.view()->maintainScrollPositionAtAnchor(0);
+        m_frame.view()->maintainScrollPositionAtAnchor(nullptr);
     m_activityAssertion = nullptr;
 }
 
index e0215016a21fd14d9553dbb4a74df95a6fc7a0b9..f07cc4e7c59782dff9a5a07852a0018f720aa037 100644 (file)
@@ -1984,7 +1984,10 @@ bool FrameView::scrollToAnchor(const String& name)
     if (!anchorElement && !(name.isEmpty() || equalIgnoringCase(name, "top")))
         return false;
 
-    maintainScrollPositionAtAnchor(anchorElement ? static_cast<Node*>(anchorElement) : frame().document());
+    ContainerNode* scrollPositionAnchor = anchorElement;
+    if (!scrollPositionAnchor)
+        scrollPositionAnchor = frame().document();
+    maintainScrollPositionAtAnchor(scrollPositionAnchor);
     
     // If the anchor accepts keyboard focus, move focus there to aid users relying on keyboard navigation.
     if (anchorElement && anchorElement->isFocusable())
@@ -1993,7 +1996,7 @@ bool FrameView::scrollToAnchor(const String& name)
     return true;
 }
 
-void FrameView::maintainScrollPositionAtAnchor(Node* anchorNode)
+void FrameView::maintainScrollPositionAtAnchor(ContainerNode* anchorNode)
 {
     m_maintainScrollPositionAnchor = anchorNode;
     if (!m_maintainScrollPositionAnchor)
@@ -2014,7 +2017,7 @@ void FrameView::scrollElementToRect(Element* element, const IntRect& rect)
 {
     frame().document()->updateLayoutIgnorePendingStylesheets();
 
-    LayoutRect bounds = element->boundingBox();
+    LayoutRect bounds = rendererAnchorRect(*element);
     int centeringOffsetX = (rect.width() - bounds.width()) / 2;
     int centeringOffsetY = (rect.height() - bounds.height()) / 2;
     setScrollPosition(IntPoint(bounds.x() - centeringOffsetX - rect.x(), bounds.y() - centeringOffsetY - rect.y()));
@@ -2775,7 +2778,7 @@ bool FrameView::shouldUpdate() const
 
 void FrameView::scrollToAnchor()
 {
-    RefPtr<Node> anchorNode = m_maintainScrollPositionAnchor;
+    RefPtr<ContainerNode> anchorNode = m_maintainScrollPositionAnchor;
     if (!anchorNode)
         return;
 
@@ -2784,7 +2787,7 @@ void FrameView::scrollToAnchor()
 
     LayoutRect rect;
     if (anchorNode != frame().document())
-        rect = anchorNode->boundingBox();
+        rect = rendererAnchorRect(*anchorNode.get());
 
     // Scroll nested layers and frames to reveal the anchor.
     // Align to the top and to the closest side (this matches other browsers).
index 97ab39dbe52d5eb580597c0dd5a600dfefccc752..a76170b1024cbe9f6d8a131d70a43402f64fdc08 100644 (file)
@@ -27,6 +27,7 @@
 
 #include "AdjustViewSizeOrNot.h"
 #include "Color.h"
+#include "ContainerNode.h"
 #include "LayoutMilestones.h"
 #include "LayoutRect.h"
 #include "Pagination.h"
@@ -388,7 +389,7 @@ public:
 
     bool scrollToFragment(const URL&);
     bool scrollToAnchor(const String&);
-    void maintainScrollPositionAtAnchor(Node*);
+    void maintainScrollPositionAtAnchor(ContainerNode*);
     WEBCORE_EXPORT void scrollElementToRect(Element*, const IntRect&);
 
     // Methods to convert points and rects between the coordinate space of the renderer, and this view.
@@ -715,7 +716,7 @@ private:
     bool m_isVisuallyNonEmpty;
     bool m_firstVisuallyNonEmptyLayoutCallbackPending;
 
-    RefPtr<Node> m_maintainScrollPositionAnchor;
+    RefPtr<ContainerNode> m_maintainScrollPositionAnchor;
 
     // Renderer to hold our custom scroll corner.
     RenderPtr<RenderScrollbarPart> m_scrollCorner;
index 65507f88241c1a11dff36068e1fcac46c04bd19b..fd0f8fe6fa98ea5c67f58e4194bbbae99cf41fac 100644 (file)
@@ -501,6 +501,7 @@ bool canScrollInDirection(const Frame* frame, FocusDirection direction)
     }
 }
 
+// FIXME: This is completely broken. This should be deleted and callers should be calling ScrollView::contentsToWindow() instead.
 static LayoutRect rectToAbsoluteCoordinates(Frame* initialFrame, const LayoutRect& initialRect)
 {
     LayoutRect rect = initialRect;
@@ -521,7 +522,8 @@ LayoutRect nodeRectInAbsoluteCoordinates(Node* node, bool ignoreBorder)
 
     if (is<Document>(*node))
         return frameRectInAbsoluteCoordinates(downcast<Document>(*node).frame());
-    LayoutRect rect = rectToAbsoluteCoordinates(node->document().frame(), node->boundingBox());
+
+    LayoutRect rect = rectToAbsoluteCoordinates(node->document().frame(), rendererBoundingBox(*node));
 
     // For authors that use border instead of outline in their CSS, we compensate by ignoring the border when calculating
     // the rect of the focused element.
index 6f52ea729b2398c9ecebe8cadc6a3d07f50486cf..25df697b926513e4f429278dee4899cfbc0a567d 100644 (file)
@@ -1511,4 +1511,127 @@ Color RenderElement::selectionBackgroundColor() const
     return theme().inactiveSelectionBackgroundColor();
 }
 
+bool RenderElement::getLeadingCorner(FloatPoint& point) const
+{
+    if (!isInline() || isReplaced()) {
+        point = localToAbsolute(FloatPoint(), UseTransforms);
+        return true;
+    }
+
+    // find the next text/image child, to get a position
+    const RenderObject* o = this;
+    while (o) {
+        const RenderObject* p = o;
+        if (RenderObject* child = o->firstChildSlow())
+            o = child;
+        else if (o->nextSibling())
+            o = o->nextSibling();
+        else {
+            RenderObject* next = 0;
+            while (!next && o->parent()) {
+                o = o->parent();
+                next = o->nextSibling();
+            }
+            o = next;
+
+            if (!o)
+                break;
+        }
+        ASSERT(o);
+
+        if (!o->isInline() || o->isReplaced()) {
+            point = o->localToAbsolute(FloatPoint(), UseTransforms);
+            return true;
+        }
+
+        if (p->node() && p->node() == element() && is<RenderText>(*o) && !downcast<RenderText>(*o).firstTextBox()) {
+            // do nothing - skip unrendered whitespace that is a child or next sibling of the anchor
+        } else if (is<RenderText>(*o) || o->isReplaced()) {
+            point = FloatPoint();
+            if (is<RenderText>(*o) && downcast<RenderText>(*o).firstTextBox())
+                point.move(downcast<RenderText>(*o).linesBoundingBox().x(), downcast<RenderText>(*o).topOfFirstText());
+            else if (is<RenderBox>(*o))
+                point.moveBy(downcast<RenderBox>(*o).location());
+            point = o->container()->localToAbsolute(point, UseTransforms);
+            return true;
+        }
+    }
+    
+    // If the target doesn't have any children or siblings that could be used to calculate the scroll position, we must be
+    // at the end of the document. Scroll to the bottom. FIXME: who said anything about scrolling?
+    if (!o && document().view()) {
+        point = FloatPoint(0, document().view()->contentsHeight());
+        return true;
+    }
+    return false;
+}
+
+bool RenderElement::getTrailingCorner(FloatPoint& point) const
+{
+    if (!isInline() || isReplaced()) {
+        point = localToAbsolute(LayoutPoint(downcast<RenderBox>(*this).size()), UseTransforms);
+        return true;
+    }
+
+    // find the last text/image child, to get a position
+    const RenderObject* o = this;
+    while (o) {
+        if (RenderObject* child = o->lastChildSlow())
+            o = child;
+        else if (o->previousSibling())
+            o = o->previousSibling();
+        else {
+            RenderObject* prev = 0;
+            while (!prev) {
+                o = o->parent();
+                if (!o)
+                    return false;
+                prev = o->previousSibling();
+            }
+            o = prev;
+        }
+        ASSERT(o);
+        if (is<RenderText>(*o) || o->isReplaced()) {
+            point = FloatPoint();
+            if (is<RenderText>(*o)) {
+                IntRect linesBox = downcast<RenderText>(*o).linesBoundingBox();
+                if (!linesBox.maxX() && !linesBox.maxY())
+                    continue;
+                point.moveBy(linesBox.maxXMaxYCorner());
+            } else
+                point.moveBy(downcast<RenderBox>(*o).frameRect().maxXMaxYCorner());
+            point = o->container()->localToAbsolute(point, UseTransforms);
+            return true;
+        }
+    }
+    return true;
+}
+
+LayoutRect RenderElement::anchorRect() const
+{
+    FloatPoint leading, trailing;
+    bool foundLeading = getLeadingCorner(leading);
+    bool foundTrailing = getTrailingCorner(trailing);
+    
+    // If we've found one corner, but not the other,
+    // then we should just return a point at the corner that we did find.
+    if (foundLeading != foundTrailing) {
+        if (foundLeading)
+            foundTrailing = foundLeading;
+        else
+            foundLeading = foundTrailing;
+    }
+
+    FloatPoint upperLeft = leading;
+    FloatPoint lowerRight = trailing;
+
+    // Vertical writing modes might mean the leading point is not in the top left
+    if (!isInline() || isReplaced()) {
+        upperLeft = FloatPoint(std::min(leading.x(), trailing.x()), std::min(leading.y(), trailing.y()));
+        lowerRight = FloatPoint(std::max(leading.x(), trailing.x()), std::max(leading.y(), trailing.y()));
+    } // Otherwise, it's not obvious what to do.
+
+    return enclosingLayoutRect(FloatRect(upperLeft, lowerRight.expandedTo(upperLeft) - upperLeft));
+}
+
 }
index 399c4efa0a9c1e4eea9feb937cc33fb84188be32..664ac3b7b181aad07a96e0a6d69e603e844f03ab 100644 (file)
@@ -150,6 +150,10 @@ public:
     bool hasClipPath() const { return style().clipPath(); }
     bool hasHiddenBackface() const { return style().backfaceVisibility() == BackfaceVisibilityHidden; }
 
+    // anchorRect() is conceptually similar to absoluteBoundingBoxRect(), but is intended for scrolling to an anchor.
+    // It works differently than absoluteBoundingBoxRect() for inline renderers.
+    LayoutRect anchorRect() const;
+
     bool hasFilter() const { return style().hasFilter(); }
     bool hasBackdropFilter() const
     {
@@ -261,6 +265,9 @@ private:
 
     virtual void newImageAnimationFrameAvailable(CachedImage&) final override;
 
+    bool getLeadingCorner(FloatPoint& output) const;
+    bool getTrailingCorner(FloatPoint& output) const;
+
     unsigned m_baseTypeFlags : 6;
     unsigned m_ancestorLineBoxDirty : 1;
     unsigned m_hasInitializedStyle : 1;
index 1267d9627dec8b3476e42feac9c63b824279fe6b..404847b2fdff70e8d42cbc9fcad2af00e617d065 100644 (file)
@@ -1006,6 +1006,11 @@ UChar RenderText::previousCharacter() const
     return prev;
 }
 
+LayoutUnit RenderText::topOfFirstText() const
+{
+    return firstTextBox()->root().lineTop();
+}
+
 void applyTextTransform(const RenderStyle& style, String& text, UChar previousCharacter)
 {
     switch (style.textTransform()) {
index 4a5075ba26d45397f8b0374cc9610ac0c091d9e6..ffef93d065a9f4ad0d65e289bc054781817a972b 100644 (file)
@@ -164,6 +164,8 @@ public:
 
     StringView stringView(int start = 0, int stop = -1) const;
 
+    LayoutUnit topOfFirstText() const;
+
 protected:
     virtual void computePreferredLogicalWidths(float leadWidth);
     virtual void willBeDestroyed() override;
index 37e03e0415d147fa1f89ffa957cb23b120a3d4a1..87d17618ee1311e1df65dbbcaa409f76ef60500c 100644 (file)
@@ -1,3 +1,14 @@
+2014-12-09  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        Delete Node::boundingBox()
+        https://bugs.webkit.org/show_bug.cgi?id=139333
+
+        Reviewed by Zalan Bujtas.
+
+        * WebView/WebActionMenuController.mm:
+        (elementBoundingBoxInWindowCoordinatesFromNode): Use
+        RenderObject::absoluteBoundingBoxRect().
+
 2014-12-08  Alexey Proskuryakov  <ap@apple.com>
 
         REGRESSION (r158036): WebView cannot handle HTTP Basic Authentication challenge
index ade30c17a9fdddf5a18cf03f19f0861400ba9a8e..b85aa71b77382d4d1865cc21f5ef577c2465c931 100644 (file)
@@ -262,7 +262,7 @@ static IntRect elementBoundingBoxInWindowCoordinatesFromNode(Node* node)
     if (!view)
         return IntRect();
 
-    return view->contentsToWindow(node->pixelSnappedBoundingBox());
+    return view->contentsToWindow(rendererBoundingBox(*node));
 }
 
 - (NSArray *)_defaultMenuItemsForLink
index 36e3affdd6f053e60f385341bedd84a7fcfeecef..5de5de88e095cef277cb02b87803dd7b6db342ed 100644 (file)
@@ -1,3 +1,17 @@
+2014-12-09  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        Delete Node::boundingBox()
+        https://bugs.webkit.org/show_bug.cgi?id=139333
+
+        Reviewed by Zalan Bujtas.
+
+        * Shared/WebHitTestResult.cpp:
+        (WebKit::WebHitTestResult::Data::elementBoundingBoxInWindowCoordinates):
+        Use RenderObject::absoluteBoundingBoxRect().
+        * WebProcess/WebPage/CoordinatedGraphics/WebPageCoordinatedGraphics.cpp:
+        (WebKit::WebPage::findZoomableAreaForPoint): Use
+        RenderObject::absoluteBoundingBoxRect().
+
 2014-12-09  Chris Dumez  <cdumez@apple.com>
 
         [WK2] Crash when answering notification permission request after navigating
index 965c299aa4730827975c70153d4231284a6d57f1..4521f2d8bcf7f9b5931c2decb5321b4b9c75c65b 100644 (file)
@@ -25,6 +25,7 @@
 #include <WebCore/Frame.h>
 #include <WebCore/FrameView.h>
 #include <WebCore/HitTestResult.h>
+#include <WebCore/RenderObject.h>
 #include <WebCore/URL.h>
 #include <WebCore/Node.h>
 #include <wtf/text/WTFString.h>
@@ -117,7 +118,7 @@ IntRect WebHitTestResult::Data::elementBoundingBoxInWindowCoordinates(const HitT
     if (!view)
         return IntRect();
 
-    return view->contentsToWindow(node->pixelSnappedBoundingBox());
+    return view->contentsToWindow(rendererBoundingBox(*node));
 }
 
 } // WebKit
index d8bfe8871361f472b747fd70b076fede0d270f9e..2bd37fe9a65f58b64306947830aef83b45b5d961 100644 (file)
@@ -37,6 +37,7 @@
 #include <WebCore/EventHandler.h>
 #include <WebCore/Frame.h>
 #include <WebCore/FrameView.h>
+#include <WebCore/RenderObject.h>
 #include <WebCore/ScrollView.h>
 
 using namespace WebCore;
@@ -54,7 +55,7 @@ void WebPage::findZoomableAreaForPoint(const IntPoint& point, const IntSize& are
     if (!node)
         return;
 
-    IntRect zoomableArea = node->pixelSnappedBoundingBox();
+    IntRect zoomableArea = rendererBoundingBox(*node);
 
     while (true) {
         bool found = !node->isTextNode() && !node->isShadowRoot();
@@ -70,7 +71,7 @@ void WebPage::findZoomableAreaForPoint(const IntPoint& point, const IntSize& are
             break;
 
         node = node->parentNode();
-        zoomableArea.unite(node->pixelSnappedBoundingBox());
+        zoomableArea.unite(rendererBoundingBox(*node));
     }
 
     if (node->document().frame() && node->document().frame()->view()) {