Allow frames with overflow:hidden scroll in slow scrolling path when scaled up

Allow frames with overflow:hidden scroll in slow scrolling path when scaled up

When a page is scaled up, we should allow the clipped contents to be
panned through the smaller viewport.

Two changes:
1. Let contentsSize be the layout size if originally the frame is not
allowed to be scrolled;
2. Set ScrollbarMode to ScrollbarAuto if it ScrollbarAlwaysOff and
visibleContentScaleFactor > 1.

BUG=181315
TEST=WebFrameTest.OverflowHiddenScrollable

[Xianzhu, 2013-05-03 00:00:41]

https://codereview.chromium.org/14874002/diff/3001/Source/core/page/FrameView...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/14874002/diff/3001/Source/core/page/FrameView...
Source/core/page/FrameView.cpp:608: hMode = ScrollbarAuto;
Sorry. Just noticed that this is wrong which is left from earlier changes.
Should not change this.

[Xianzhu, 2013-05-03 00:26:49]

https://codereview.chromium.org/14874002/diff/3001/Source/core/page/FrameView...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/14874002/diff/3001/Source/core/page/FrameView...
Source/core/page/FrameView.cpp:608: hMode = ScrollbarAuto;
On 2013/05/03 00:00:41, Xianzhu wrote:
> Sorry. Just noticed that this is wrong which is left from earlier changes.
> Should not change this.

Done.

[jamesr, 2013-05-03 00:29:14]

This is for subframes, correct?  When would a non-scrollable subframe become
scrollable due to page scale?  If the iframe's contents fit entirely into its
container at a scale of 1, when scaled in both the iframe's contents and the
iframe container itself should grow by the same amount and thus everything
should still fit.

[Xianzhu, 2013-05-03 00:41:38]

On 2013/05/03 00:29:14, jamesr wrote:
> This is for subframes, correct?  When would a non-scrollable subframe become
> scrollable due to page scale?  If the iframe's contents fit entirely into its
> container at a scale of 1, when scaled in both the iframe's contents and the
> iframe container itself should grow by the same amount and thus everything
> should still fit.

No. In current environment, this is for main frame whose
visibleContentScaleFactor > 1.

(However, in some in-theory only circumstance that we can scale a subframes
independent of the page scale, I think the logic will also apply to subframes.)

[jamesr, 2013-05-03 00:57:10]

If you're just dealing with the main frame, then I think we should probably
determine scrollability ourselves in the compositor based on whether the user's
pinched in or not.  Do we allow pinch zoom on non-scrollable pages today?

I don't think we should add code to deal with the theoretical possibility of
scaling subframes.

[aelias_OOO_until_Jul13, 2013-05-03 01:03:34]

How about just making the main frame always return "true" for isScrollable, and
make overflow:hidden affect the scrollable bounds?

[Xianzhu, 2013-05-03 02:29:32]

On 2013/05/03 00:57:10, jamesr wrote:
> If you're just dealing with the main frame, then I think we should probably
> determine scrollability ourselves in the compositor based on whether the
user's
> pinched in or not.  Do we allow pinch zoom on non-scrollable pages today?

Does the method also work for slow scrolling path?

The code added is intended for main frame only. The theoretical possibility is
not intended.

[Xianzhu, 2013-05-03 02:32:15]

On 2013/05/03 01:03:34, aelias wrote:
> How about just making the main frame always return "true" for isScrollable,
and
> make overflow:hidden affect the scrollable bounds?

I think isScrollable is not the key function, but the ScrollbarModes. Maybe we
can force ScrollbarModes to be Auto for main frames, but that will introduce
again FrameView needs to know if itself is main frame.

[Xianzhu, 2013-05-03 20:52:41]

Added some code review comments to explain the changes.

https://codereview.chromium.org/14874002/diff/7001/Source/core/page/FrameView...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/14874002/diff/7001/Source/core/page/FrameView...
Source/core/page/FrameView.cpp:580: size.setHeight(layoutHeight());
Change 1: let contentsSize() be the layout size if the content is not supposed
to be scrollable when not scaled.

This ensures that the overflowed contents won't be scrolled in. For
ScrollableArea, the layout size is actually the scrollable content size in the
case.

Just verified that this also fixes the scroll range issue for overflow:hidden in
the fast path.

https://codereview.chromium.org/14874002/diff/7001/Source/core/page/FrameView...
Source/core/page/FrameView.cpp:720: }
Change 2: Allow scrolling of originally not-scrollable frames if the frame is
scaled up, by forcing the scrollbar mode to ScrollbarAuto.

https://codereview.chromium.org/14874002/diff/7001/Source/core/page/FrameView...
Source/core/page/FrameView.cpp:2645:
static_cast<ScrollbarModesCalculationStrategy>(RulesFromWebContent |
RulesFromVisibleContentScaleFactor));
This is not the key change, just to let isScrollable() return the actual
scrollable status.

[Xianzhu, 2013-05-07 16:39:46]

Any feedbacks to my latest comments?

[aelias_OOO_until_Jul13, 2013-05-07 19:57:06]

Shrinking contentsSize ends up shrink the CC contents layer size, which is
incorrect.  It would be cut off if the scroll position is set by javascript.

Secondly, this has to do with scrolling, not scrollbars, so it's messy that
"ScrollbarMode" is involved.  I understand that in the current code those
concepts are tangled up together, but that should be cleaned up, not made worse.

[Xianzhu, 2013-05-07 20:14:56]

On 2013/05/07 19:57:06, aelias wrote:
> Shrinking contentsSize ends up shrink the CC contents layer size, which is
> incorrect.  It would be cut off if the scroll position is set by javascript.
>

In contrary, I think this actually fixes the issue that overflowed contents
waste layer space and may be shown incorrectly by the compositor. Truly
overflowed contents in a overflow:hidden container should never be shown.

> Secondly, this has to do with scrolling, not scrollbars, so it's messy that
> "ScrollbarMode" is involved.  I understand that in the current code those
> concepts are tangled up together, but that should be cleaned up, not made
worse.

Agreed. I can rename ScrollbarMode to ScrollMode.

[Xianzhu, 2013-05-07 20:33:49]

About ScrollableArea::contentsSize(), it is actually 'scrollableContentsSize' as
a interface method of ScrollableArea. In this sense, I think it's correct to set
it to layoutSize for overflow:hidden FrameView.

trchen@ will rename contentsSize to a more proper name when cleaning up
scrolling stuffs, so I just leave it as is in this change.

[Xianzhu, 2013-05-07 21:42:36]

Closing this CL because enne@'s two viewport method can better resolve the
issue.