Initial draft - modify ViewportAnchor to know about both inner and outer viewports.

Source/web/ViewportAnchor.cpp

Index: Source/web/ViewportAnchor.cpp
diff --git a/Source/web/ViewportAnchor.cpp b/Source/web/ViewportAnchor.cpp
index 4a6a3d40164d5d6fb0f61dfd69c1936e049087c7..4d39354cad4ea259145a4b1162abf0a5c83be2a1 100644
--- a/Source/web/ViewportAnchor.cpp
+++ b/Source/web/ViewportAnchor.cpp
@@ -68,31 +68,65 @@ Node* findNonEmptyAnchorNode(const IntPoint& point, const IntRect& viewRect, Eve
     return node;
 }
 
+void moveToEncloseRect(FloatRect * outer, const FloatRect & inner)
+{
+    ASSERT(outer);
+
+    FloatPoint minimumPosition = inner.location() + inner.size() - outer->size();
+
+    // Minimum position should be positive or zero
+    minimumPosition = minimumPosition.expandedTo(FloatPoint());
+
+    FloatPoint maximumPosition = inner.location();
+
+    FloatPoint outerOrigin = outer->location();
+    outerOrigin = outerOrigin.expandedTo(minimumPosition);
+    outerOrigin = outerOrigin.shrunkTo(maximumPosition);
+
+    outer->setLocation(outerOrigin);
+}
+
 } // namespace
 
 ViewportAnchor::ViewportAnchor(EventHandler* eventHandler)
     : m_eventHandler(eventHandler) { }
 
-void ViewportAnchor::setAnchor(const IntRect& viewRect, const FloatSize& anchorInViewCoords)
+void ViewportAnchor::setAnchor(const IntRect& outerViewRect, const IntRect& innerViewRect,
+    const FloatSize& anchorInInnerViewCoords)
 {
-    m_viewRect = viewRect;
+    // Preserve the inner viewport position in document in case we won't find the anchor
+    m_pinchViewportInDocument = innerViewRect.location();
+
     m_anchorNode.clear();
     m_anchorNodeBounds = LayoutRect();
     m_anchorInNodeCoords = FloatSize();
-    m_anchorInViewCoords = anchorInViewCoords;
+    m_anchorInInnerViewCoords = anchorInInnerViewCoords;
+    m_normalizedPinchViewportOffset = FloatSize();
 
-    if (viewRect.isEmpty())
+    if (innerViewRect.isEmpty())
         return;
 
     // Preserve origins at the absolute screen origin
-    if (viewRect.location() == IntPoint::zero())
+    if (innerViewRect.location() == IntPoint::zero())
         return;
 
-    FloatSize anchorOffset = viewRect.size();
-    anchorOffset.scale(anchorInViewCoords.width(), anchorInViewCoords.height());
-    const FloatPoint anchorPoint = FloatPoint(viewRect.location()) + anchorOffset;
+    // Inner rectangle should be within the outer one.
+    ASSERT(outerViewRect.contains(innerViewRect));
+
+    // Outer rectangle is used as a scale, we need positive width and height.
+    ASSERT(!outerViewRect.isEmpty());
+
+    m_normalizedPinchViewportOffset = innerViewRect.location() - outerViewRect.location();
 
-    Node* node = findNonEmptyAnchorNode(flooredIntPoint(anchorPoint), viewRect, m_eventHandler);
+    // Normalize by the size of the outer rect
+    m_normalizedPinchViewportOffset.scale(1.0 / outerViewRect.width(), 1.0 / outerViewRect.height());
+
+
+    FloatSize anchorOffset = innerViewRect.size();
+    anchorOffset.scale(anchorInInnerViewCoords.width(), anchorInInnerViewCoords.height());
+    const FloatPoint anchorPoint = FloatPoint(innerViewRect.location()) + anchorOffset;
+
+    Node* node = findNonEmptyAnchorNode(flooredIntPoint(anchorPoint), innerViewRect, m_eventHandler);
     if (!node)
         return;
 
@@ -102,14 +136,32 @@ void ViewportAnchor::setAnchor(const IntRect& viewRect, const FloatSize& anchorI
     m_anchorInNodeCoords.scale(1.f / m_anchorNodeBounds.width(), 1.f / m_anchorNodeBounds.height());
 }
 
-IntPoint ViewportAnchor::computeOrigin(const IntSize& currentViewSize) const
+void ViewportAnchor::computeOrigins(const IntSize& outerSize, const IntSize& innerSize,
+    IntPoint* mainFrameOffset, FloatPoint* pinchViewportOffset) const
+{
+    FloatSize absPinchViewportOffset = m_normalizedPinchViewportOffset;
+    absPinchViewportOffset.scale(outerSize.width(), outerSize.height());
+
+    FloatPoint innerOrigin = getInnerOrigin(innerSize); // relative to the document
+    FloatPoint outerOrigin = innerOrigin - absPinchViewportOffset;
+
+    // Move outer viewport to enclose inner viewport
+    FloatRect outerRect = FloatRect(outerOrigin, outerSize);
+    moveToEncloseRect(&outerRect, FloatRect(innerOrigin, FloatSize(innerSize)));

[timav, 2014/09/11 21:52:05]

At first I thought this move is necessary, but now the whole idea of this move
seems incorrect to me. Please confirm that we are never going to scroll the
outer viewport horizontally and I will remove it.

[bokan, 2014/09/12 20:53:07]

In general, the outer viewport can be scrolled horizontally and vertically, but
I'm not sure why you think the horizontal case is special; they should be
symmetrical. I do think you may have edge cases where the innerOrigin -
absPinchViewportOffset ends up with the outer viewport not bounding the inner so
I expect you need this.

That said, you're relying on the returned pinchViewportOffset to be clamped by
the caller for this to work. I think a better way to do this would be to replace
moveToEncloseRect with something like
clampPinchViewportOffset(absPinchViewportOffset, innerSize, outerSize) to ensure
the inner viewport offset is valid and do that before setting the outerOrigin.

[timav, 2014/09/12 23:33:35]

David, my idea here was different (of course if I get you right): I did not want
to clamp inner viewport into some boundaries after calculating its proper
position in the document. On the contrary, I wanted to ensure that the outer
viewport covers the inner by moving the outer (effectively modifying the
relative position of the inner within the outer).

The thing is, I never saw it working in practice: I could never made the inner
viewport protrude down through the outer viewport lower edge for vertical
scrolls; in case of extreme horizontal scrolls I did have this situation and the
outer viewport did move to x > 0, but later the ScrollView::setScrollOffset()
returned it back to 0.

I realized that honestly I would need to take into account the outer viewport
constraints by ScrollView::adjustScrollPositionWithinRange(), but then I thought
that there are cases where anchor cannot be satisfied and so inner viewport will
be clamped, then what's wrong on relying the caller to clamp it (as you say)?

This is why I now think that the whole moving idea *in this place in code* is
incorrect. 

[timav, 2014/09/12 23:42:28]

Correction: I actually wanted to say that this would be code complication that
won't give us much (but the code place is right), but of course I'd like to hear
your opinion.

[timav, 2014/09/12 23:59:52]

Correction 2: I did not realize that if I move the outer rectangle I need to
move the inner one in the opposite direction do it will stay in the same place
in doc. This is still to precisely clamping, but now I'd just like to apologize
for the noise.
I'll provide another version and the we'll see whether we need it or not. Sorry.

[bokan, 2014/09/15 15:36:35]

Right, the end result is the same though, we want the inner viewport to be
bounded by the outer since otherwise there'll be a clamp.
In most cases the outer viewport won't be horizontally scrollable, so that's
why you see it return to 0. However, when the page sets an explicit minimum
zoom there may be horizontal scrolling of the outer viewport so I think this
is necessary. I still think it can be caused in both directions due to symmetry.
The issue is that any clamping on the outer viewport has to be matched by moving
the inner viewport in the opposite direction. This is non-obvious and easy to
get wrong so I'd rather we do all that in this class; the offsets returned
from here should be the final positions of the viewports.

In light of this, you'll need an additional rect in the form of the outer
viewport's content rect. I think it may make more sense to pass in the
FrameView itself (in the form of a ScrollableArea ref). IMO the steps should
looks something like:

1) Clamp absPinchViewportOffset so that:
absPinchViewportOffset + innerViewportSize <= outerViewportSize

2) Position outer viewport
outerRect = innerOrigin - absPinchViewportOffset

3) Clamp outer viewport so that:
outerRect.location + outerRect.size <= contentRect.size

4) Apply outer viewport clamp amount in opposite direction to
absPinchViewportOffset. You may wish to ASSERT that:

outerRect.location + absPinchViewportOffset == innerOrigin

But feel free to do it as you see fit, I'll leave the details to you :)

[timav, 2014/09/15 20:04:34]

Yes, absolutely. I realized it after I submitted my last lengthy comment.
I passed the view as a const ScrollView * since I use
adjustScrollPositionWithinRange(). The rest seems to be more or less
staightforward, I get the locations of both viewports in the reference frame of
the document, then the pinchViewportOffset is the difference between them.
Please review the next patch.

+
+    *mainFrameOffset = flooredIntPoint(outerRect.location());
+    *pinchViewportOffset = FloatPoint(absPinchViewportOffset);
+}
+
+
+FloatPoint ViewportAnchor::getInnerOrigin(const IntSize& innerSize) const
 {
     if (!m_anchorNode || !m_anchorNode->inDocument())
-        return m_viewRect.location();
+        return m_pinchViewportInDocument;
 
     const LayoutRect currentNodeBounds = m_anchorNode->boundingBox();
     if (m_anchorNodeBounds == currentNodeBounds)
-        return m_viewRect.location();
+        return m_pinchViewportInDocument;
 
     // Compute the new anchor point relative to the node position
     FloatSize anchorOffsetFromNode = currentNodeBounds.size();
@@ -117,9 +169,9 @@ IntPoint ViewportAnchor::computeOrigin(const IntSize& currentViewSize) const
     FloatPoint anchorPoint = currentNodeBounds.location() + anchorOffsetFromNode;
 
     // Compute the new origin point relative to the new anchor point
-    FloatSize anchorOffsetFromOrigin = currentViewSize;
-    anchorOffsetFromOrigin.scale(m_anchorInViewCoords.width(), m_anchorInViewCoords.height());
-    return flooredIntPoint(anchorPoint - anchorOffsetFromOrigin);
+    FloatSize anchorOffsetFromOrigin = innerSize;
+    anchorOffsetFromOrigin.scale(m_anchorInInnerViewCoords.width(), m_anchorInInnerViewCoords.height());
+    return anchorPoint - anchorOffsetFromOrigin;
 }
 
 } // namespace blink