A div can be pan-scrolled beyond its overflow (Windows only)

A div can be pan-scrolled beyond its overflow (Windows only)

The function RenderLayer::scrollByRecursively() has been
changed so that its parameter |clamp| is passed as an
argument when the function calls itself recursively. This
guarantees correct behavior in cases where nested divs
are pan-scrollable in one direction but not the other.

BUG=232965
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151713

[tdanderson, 2013-05-06 00:41:20]

@yosin, I am trying to write a Windows layout test for http://crbug.com/232965
that will fail on ToT but pass with the changes to RenderLayer seen in patch set
1. However, my layout test is passing on ToT (see my attempts in patch sets 6
and 7). Do you have any suggestions for what I might be doing wrong? I tried to
model the test after panScroll-nested-divs.html.

[yosin_UTC9, 2013-05-07 01:48:43]

It seems timing issue, bot is slower than local desktop.
How about moving mouse pointer toward diagonal right and waiting for horizontal
scroll occurred?
See panScroll-{click,drag}.html

[tdanderson, 2013-05-13 23:06:14]

On 2013/05/07 01:48:43, Yoshifumi Inoue wrote:
> It seems timing issue, bot is slower than local desktop.
> How about moving mouse pointer toward diagonal right and waiting for
horizontal
> scroll occurred?
> See panScroll-{click,drag}.html

@yosin, thanks for the guidance. After some experimentation, I wrote a new test
modeled after panScroll-click.html (see Patch Set 15) but it was [ Timeout Pass
] during the last run though the win trybots. Do you have any advice as to how I
can remove the flakiness?

[yosin_UTC9, 2013-05-14 01:40:05]

https://codereview.chromium.org/14767007/diff/35001/LayoutTests/fast/events/p...
File LayoutTests/fast/events/panScroll-nested-divs-forbidden.html (right):

https://codereview.chromium.org/14767007/diff/35001/LayoutTests/fast/events/p...
LayoutTests/fast/events/panScroll-nested-divs-forbidden.html:47: if
(outerdiv.scrollTop)
nit: these check are redundant. We don't need to verify them here again.

https://codereview.chromium.org/14767007/diff/35001/LayoutTests/fast/events/p...
LayoutTests/fast/events/panScroll-nested-divs-forbidden.html:76: if (retryCount
> 10) {
We don't need to iterate 10 times for fast machine it is near 500ms but on slow
machine it may be more.

Once, we detect left scrolling we can finish testing with checking no vertical
scroll.

function checkScrolled() {
  ++retryCount;
  if (outerdiv.scrollLeft || retryCount > 10) {
    finsihTest();
    return;
  }
  window.setTimeout(checkScrolled, 50);
}

function finishTest() {
  shouldNotBe(outdiv.scrollLeft, 0);
  shouldBeZero(outerDiv.scrollTop);
  testRunner.notifyDone();
}

https://codereview.chromium.org/14767007/diff/35001/LayoutTests/fast/events/p...
LayoutTests/fast/events/panScroll-nested-divs-forbidden.html:94:
window.jsTestIsAsync = false;
should be window.jsTestIsAsync = true and
don't call testRunner.waitUntilDone() which is called in js-test-post.js.

This is a kind of idiom.

[tdanderson, 2013-05-16 17:34:52]

https://codereview.chromium.org/14767007/diff/35001/LayoutTests/fast/events/p...
File LayoutTests/fast/events/panScroll-nested-divs-forbidden.html (right):

https://codereview.chromium.org/14767007/diff/35001/LayoutTests/fast/events/p...
LayoutTests/fast/events/panScroll-nested-divs-forbidden.html:47: if
(outerdiv.scrollTop)
On 2013/05/14 01:40:05, Yoshifumi Inoue wrote:
> nit: these check are redundant. We don't need to verify them here again.

OK, removed.

https://codereview.chromium.org/14767007/diff/35001/LayoutTests/fast/events/p...
LayoutTests/fast/events/panScroll-nested-divs-forbidden.html:76: if (retryCount
> 10) {
On 2013/05/14 01:40:05, Yoshifumi Inoue wrote:
> We don't need to iterate 10 times for fast machine it is near 500ms but on
slow
> machine it may be more.
> 
> Once, we detect left scrolling we can finish testing with checking no vertical
> scroll.
> 
> function checkScrolled() {
>   ++retryCount;
>   if (outerdiv.scrollLeft || retryCount > 10) {
>     finsihTest();
>     return;
>   }
>   window.setTimeout(checkScrolled, 50);
> }
> 
> function finishTest() {
>   shouldNotBe(outdiv.scrollLeft, 0);
>   shouldBeZero(outerDiv.scrollTop);
>   testRunner.notifyDone();
> }

Done. Thanks a lot for the clarification.

https://codereview.chromium.org/14767007/diff/35001/LayoutTests/fast/events/p...
LayoutTests/fast/events/panScroll-nested-divs-forbidden.html:94:
window.jsTestIsAsync = false;
On 2013/05/14 01:40:05, Yoshifumi Inoue wrote:
> should be window.jsTestIsAsync = true and
> don't call testRunner.waitUntilDone() which is called in js-test-post.js.
> 
> This is a kind of idiom.

Done.

[yosin_UTC9, 2013-05-17 02:00:22]

https://codereview.chromium.org/14767007/diff/46001/LayoutTests/fast/events/p...
File LayoutTests/fast/events/panScroll-nested-divs-forbidden.html (right):

https://codereview.chromium.org/14767007/diff/46001/LayoutTests/fast/events/p...
LayoutTests/fast/events/panScroll-nested-divs-forbidden.html:44:
shouldNotBe('outerdiv.scrollLeft', '0');
This 'outerdiv' refers window.outerdiv from div[@id='outerdiv'], because
shouldNotBe() uses eval().

So, please move var outerdiv = $('outerdiv'); to top-level for avoiding
confusion.

[tdanderson, 2013-05-17 17:30:20]

https://codereview.chromium.org/14767007/diff/46001/LayoutTests/fast/events/p...
File LayoutTests/fast/events/panScroll-nested-divs-forbidden.html (right):

https://codereview.chromium.org/14767007/diff/46001/LayoutTests/fast/events/p...
LayoutTests/fast/events/panScroll-nested-divs-forbidden.html:44:
shouldNotBe('outerdiv.scrollLeft', '0');
On 2013/05/17 02:00:22, Yoshifumi Inoue wrote:
> This 'outerdiv' refers window.outerdiv from div[@id='outerdiv'], because
> shouldNotBe() uses eval().
> 
> So, please move var outerdiv = $('outerdiv'); to top-level for avoiding
> confusion.

Done.

[yosin_UTC9, 2013-05-20 03:22:36]

> So, please move var outerdiv = $('outerdiv'); to top-level for avoiding
confusion.

Sorry, my comment is miss leading.
Initialization of |outerdiv| should be in testIt(), because "outerdiv" is
defined after script.

var outerdiv;
function testIt() {
  outerdiv = $('outerdiv');
  ...
}

[tdanderson, 2013-05-28 22:22:46]

After getting access to a Windows machine, I have determined that only a
one-line change is required to fix this bug.

@jamesr, can you please have a look?

@yosin, even with your suggestions in place, this layout test does not fail on
ToT as I would expect. I have tried many different combinations of starting
point of click, length and duration of pan scroll, etc., but outerdiv.scrollTop
is always 0 during the test and once the test finishes. Do you have any other
ideas about why this might be happening?

[jamesr, 2013-05-28 22:26:11]

Code change lgtm. We should try to figure out what's up with the test.

https://codereview.chromium.org/14767007/diff/66001/LayoutTests/fast/events/p...
File LayoutTests/fast/events/panScroll-nested-divs-forbidden.html (right):

https://codereview.chromium.org/14767007/diff/66001/LayoutTests/fast/events/p...
LayoutTests/fast/events/panScroll-nested-divs-forbidden.html:41:
window.setTimeout(checkScrolled, 50);
what is this delay for?

https://codereview.chromium.org/14767007/diff/66001/LayoutTests/fast/events/p...
LayoutTests/fast/events/panScroll-nested-divs-forbidden.html:61:
window.setTimeout(testIt, 0);
what is this timeout for?

[yosin_UTC9, 2013-05-29 08:15:52]

On 2013/05/28 22:22:46, tdanderson wrote:
> @yosin, even with your suggestions in place, this layout test does not fail on
> ToT as I would expect. I have tried many different combinations of starting
> point of click, length and duration of pan scroll, etc., but
outerdiv.scrollTop
> is always 0 during the test and once the test finishes. Do you have any other
> ideas about why this might be happening?

Please change to "width: 120%" to "width: 121%" for innerdiv, or other value to
produce fraction when converting LayoutUnit to pixel.
Default width of content window doesn't produce so for "width: 120%".
Note: when we use "width: 1200px" for innerdiv, there is no vertical scrolling.

Vertical scrolling occurred when innerdiv is chosen for auto scrolling renderer
and it should be wrong.

https://codereview.chromium.org/14767007/diff/66001/LayoutTests/fast/events/p...
File LayoutTests/fast/events/panScroll-nested-divs-forbidden.html (right):

https://codereview.chromium.org/14767007/diff/66001/LayoutTests/fast/events/p...
LayoutTests/fast/events/panScroll-nested-divs-forbidden.html:41:
window.setTimeout(checkScrolled, 50);
On 2013/05/28 22:26:11, jamesr wrote:
> what is this delay for?
I think 50ms comes from Autscroll timer interval.

https://codereview.chromium.org/14767007/diff/66001/LayoutTests/fast/events/p...
LayoutTests/fast/events/panScroll-nested-divs-forbidden.html:61:
window.setTimeout(testIt, 0);
On 2013/05/28 22:26:11, jamesr wrote:
> what is this timeout for?
It is better to use 'load' event rather than timeout to make sure outdiv is
available to testIt().

[tdanderson, 2013-05-29 17:57:31]

@yosin, changing the width to 121% worked. Thank you for the suggestion. I
verified locally that the layout test fails on ToT and passes with this patch.

@jamesr, does the fixed layout test lgty?

https://codereview.chromium.org/14767007/diff/66001/LayoutTests/fast/events/p...
File LayoutTests/fast/events/panScroll-nested-divs-forbidden.html (right):

https://codereview.chromium.org/14767007/diff/66001/LayoutTests/fast/events/p...
LayoutTests/fast/events/panScroll-nested-divs-forbidden.html:61:
window.setTimeout(testIt, 0);
On 2013/05/29 08:15:52, Yoshifumi Inoue wrote:
> On 2013/05/28 22:26:11, jamesr wrote:
> > what is this timeout for?
> It is better to use 'load' event rather than timeout to make sure outdiv is
> available to testIt(). 

Done.

[jamesr, 2013-05-29 22:46:55]

Mostly better.  I'm still concerned with the setTimeout() - what exactly are you
trying to achieve ordering relative to?

https://codereview.chromium.org/14767007/diff/73001/LayoutTests/fast/events/p...
File LayoutTests/fast/events/panScroll-nested-divs-forbidden.html (right):

https://codereview.chromium.org/14767007/diff/73001/LayoutTests/fast/events/p...
LayoutTests/fast/events/panScroll-nested-divs-forbidden.html:41:
window.setTimeout(checkScrolled, 50);
this looks flaky to me.  why 50ms? what happens if the timeout fires much later
than you expect?

[yosin_UTC9, 2013-05-30 01:45:54]

https://codereview.chromium.org/14767007/diff/73001/LayoutTests/fast/events/p...
File LayoutTests/fast/events/panScroll-nested-divs-forbidden.html (right):

https://codereview.chromium.org/14767007/diff/73001/LayoutTests/fast/events/p...
LayoutTests/fast/events/panScroll-nested-divs-forbidden.html:41:
window.setTimeout(checkScrolled, 50);
On 2013/05/29 22:46:56, jamesr wrote:
> this looks flaky to me.  why 50ms? what happens if the timeout fires much
later
> than you expect?

50ms is auto scrolling interval. We may catch second auto scroll event.
We assume we can get an auto scroll event within 500ms in worst case.

[yosin_UTC9, 2013-05-30 04:29:55]

https://codereview.chromium.org/14767007/diff/73001/LayoutTests/fast/events/p...
File LayoutTests/fast/events/panScroll-nested-divs-forbidden.html (right):

https://codereview.chromium.org/14767007/diff/73001/LayoutTests/fast/events/p...
LayoutTests/fast/events/panScroll-nested-divs-forbidden.html:41:
window.setTimeout(checkScrolled, 50);
On 2013/05/29 22:46:56, jamesr wrote:
> this looks flaky to me.  why 50ms? what happens if the timeout fires much
later
> than you expect?

Finally, I understand what you expect.
We can use 'scroll' event handler to check without setTimeout(), e.g.

    outerdiv.addEventListener('scroll', function() {
        shouldNotBe('outerdiv.scrollLeft', '0');
        shouldBeZero('outerdiv.scrollTop');
        testRunner.notifyDone();
    });

I'll update other tests which use this pattern.
Thanks!

[jamesr, 2013-05-30 04:37:11]

Ah yeah, I think that's the ticket.

[yosin_UTC9, 2013-05-30 09:30:54]

On 2013/05/30 04:37:11, jamesr wrote:
> Ah yeah, I think that's the ticket.

I wrote https://codereview.chromium.org/15653007/

So, panScroll-nested-divs-forbidden.html can be

<script>
function start() {
    outerdiv = $('outerdiv');

    testPanScroll({
        'clickOrDrag': 'drag',
        'endX': 225,
        'endY': 75,
        'finishTest': function() {
            shouldNotBe('outerdiv.scrollLeft', '0');
            shouldBeZero('outerdiv.scrollTop');
        },
        'scrollable': outerdiv,
        'startX': 150,
        'startY': 150,
    });
}
</script>
</head>
<body onload="start()">
...
</body>

[tdanderson, 2013-06-03 23:35:59]

On 2013/05/30 09:30:54, Yoshifumi Inoue wrote:
> On 2013/05/30 04:37:11, jamesr wrote:
> > Ah yeah, I think that's the ticket.
> 
> I wrote https://codereview.chromium.org/15653007/
> 
> So, panScroll-nested-divs-forbidden.html can be
> 
> <script>
> function start() {
>     outerdiv = $('outerdiv');
> 
>     testPanScroll({
>         'clickOrDrag': 'drag',
>         'endX': 225,
>         'endY': 75,
>         'finishTest': function() {
>             shouldNotBe('outerdiv.scrollLeft', '0');
>             shouldBeZero('outerdiv.scrollTop');
>         },
>         'scrollable': outerdiv,
>         'startX': 150,
>         'startY': 150,
>     });
> }
> </script>
> </head>
> <body onload="start()">
> ...
> </body>

James, does the test change lgty?

[jamesr, 2013-06-03 23:41:46]

yup, lgtm

[commit-bot: I haz the power, 2013-06-03 23:44:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdanderson@chromium.org/14767007/87001

[commit-bot: I haz the power, 2013-06-04 01:27:01]

Change committed as 151713