Fix drag-and-drop autoscrolling with --enable-pinch-virtual-viewport

Fix drag-and-drop autoscrolling with --enable-pinch-virtual-viewport

The autoscrolling code uses RenderBox::scrollRectToVisible but because the
virtual-viewport path was using the pinch viewport as the visible rect, it
also included the scrollbar area, meaning the autoscroll region was shifted
outwards.

The fix here intersects the PinchViewport visible rect with the main FrameView's
visible rect to remove the scrollbar from the region. This is done both in
RenderBox::scrollRectToVisible and PinchViewport::scrollIntoView to prevent
scrollIntoView from counter scrolling the FrameView due to it's rect centering
logic.

No new test since existing layout test already exists for this and is reenabled
by this patch.

BUG=443631, 428980
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188953

[bokan, 2015-01-15 23:52:26]

bokan@chromium.org changed reviewers:
+ rbyers@chromium.org

[bokan, 2015-01-15 23:53:01]

Hey Rick, ptal when you have a minute. FYI, this one will need a merge to 41.

https://codereview.chromium.org/853103003/diff/20001/Source/core/frame/PinchV...
File Source/core/frame/PinchViewport.cpp (left):

https://codereview.chromium.org/853103003/diff/20001/Source/core/frame/PinchV...
Source/core/frame/PinchViewport.cpp:160: rect.y() - centeringOffsetY -
visible.y());
visibleRect() is relative to the FrameView, since we now use
visibleRectInDocument, the adjustment here is no longer necessary.

[bokan, 2015-01-16 16:23:50]

Actually, hold off while I investigate, the unit tests were passing locally but
are failing on the bots.

[bokan, 2015-01-16 20:39:24]

Ok, ptal now. The test was failing locally as well, I must have run the wrong
build (built Debug, ran Release). I was wrong about not needing to account for
the pinch viewport in  targetOffset.

The essence of this change now is that the centering logic excludes the
FrameView's scrollbar area now.

[Rick Byers, 2015-01-19 21:21:20]

On 2015/01/16 20:39:24, bokan wrote:
> Ok, ptal now. The test was failing locally as well, I must have run the wrong
> build (built Debug, ran Release). I was wrong about not needing to account for
> the pinch viewport in  targetOffset.
> 
> The essence of this change now is that the centering logic excludes the
> FrameView's scrollbar area now.

Rather than put this logic in two places, perhaps you should add a
'visibleRectExcludingScrollbars' method to PinchViewport and use that in both
places?  I can imagine there might be other scenarios where that's really the
rect we want.  WDYT?

[bokan, 2015-01-22 16:26:22]

On 2015/01/19 21:21:20, Rick Byers wrote:
> On 2015/01/16 20:39:24, bokan wrote:
> > Ok, ptal now. The test was failing locally as well, I must have run the
wrong
> > build (built Debug, ran Release). I was wrong about not needing to account
for
> > the pinch viewport in  targetOffset.
> > 
> > The essence of this change now is that the centering logic excludes the
> > FrameView's scrollbar area now.
> 
> Rather than put this logic in two places, perhaps you should add a
> 'visibleRectExcludingScrollbars' method to PinchViewport and use that in both
> places?  I can imagine there might be other scenarios where that's really the
> rect we want.  WDYT?

On second thought, I've changed my mind on this patch. I'll put up a new one
soon. Basically, I don't think the PinchViewport should be thinking about
whether it's container has scrollbars or not. As far as it's concerned, it's
contained in a box and that's all; it should be the client code's responsibility
to jigger the rects to get it do to the right thing (i.e. RenderBox should
change the rect it passes to PinchViewport so that it scrolls like RenderBox
wants it to).

This might make the code in RenderBox a little clunky, but this fix is a minimal
band-aid for 41. Drag-and-drop autoscroll doesn't work when you're zoomed in
(tracked in crbug.com/449340). I'll clean it up when looking at that.

[bokan, 2015-01-23 01:09:43]

Rick, PTAL. Like I said, it's not ideal but I'll clean it up when enabling it to
work while zoomed in.

[Rick Byers, 2015-01-23 22:07:44]

LGTM
I agree keeping this minimal is the right fix to merge.  I know you'll continue
to clean this all up in trunk and remove your FIXMEs :-).  Thanks!

Rick

[bokan, 2015-01-26 12:11:13]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2015-01-26 12:11:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/853103003/80001

[commit-bot: I haz the power, 2015-01-26 13:27:45]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188953