Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1461473002:
Ensure that the auto scroll state in the AutoscrollController is cleared if the scroll operation cl… (Closed)
DescriptionEnsure that the auto scroll state in the AutoscrollController is cleared if the
scroll operation clears out the m_autoscrollLayoutObject member.
This typically happens when we scroll a div inside an iframe which uses
overflow:auto and during the scroll the cursor leaves the bounds of the div.
If we don't reset the state then the page gets stuck in the autoscroll state
causing it to be unresponsive to clicks etc.
BUG=336373
Committed: https://crrev.com/a12dafbe77137b234cf65cd5031fd7db2d1ba6fc
Cr-Commit-Position: refs/heads/master@{#360997}
Patch Set 1 #Patch Set 2 : Add layouttests #
Total comments: 6
Patch Set 3 : Remove usage of setTimeout from the layout test #
Total comments: 15
Patch Set 4 : Address review comments #Patch Set 5 : Attempt to fix layout test failures by delaying the autoscroll to after the layout happens #Patch Set 6 : Another attempt to fix layout test redness #
Messages
Total messages: 28 (12 generated)
ananta@chromium.org changed reviewers: + rbyers@chromium.org, wkorman@chromium.org
wkorman@chromium.org changed reviewers: + skobes@chromium.org
Thanks for looking at this! Please add a test, it should be possible to create a simplified LayoutTest that crashes/times out without this fix. See other scroll/autoscroll tests for samples.
On 2015/11/18 18:55:55, wkorman wrote: > Thanks for looking at this! Please add a test, it should be possible to create a > simplified LayoutTest that crashes/times out without this fix. See other > scroll/autoscroll tests for samples. Done. PTAL
https://codereview.chromium.org/1461473002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/autoscroll-scrollable-iframe-div.html (right): https://codereview.chromium.org/1461473002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/autoscroll-scrollable-iframe-div.html:25: setTimeout(autoscrollTestPart2, 100); Did you try doing another layoutAndPaintAsyncThen() here, that calls autoscrollTestPart2, instead of setTimeout? https://codereview.chromium.org/1461473002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/autoscroll-scrollable-iframe-div.html:40: setTimeout(testFailed, 100); And try layoutAndPaintAsyncThen() here as well. Or, runAfterLayoutAndPaint, see: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/1461473002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/autoscroll-scrollable-iframe-div.html:45: log("FAILED : the autoscroll has failed !"); This only logs FAILED if testrunner exists, but below we always log PASSED regardless of testrunner. Probably we should log here even without the runner?
https://codereview.chromium.org/1461473002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/autoscroll-scrollable-iframe-div.html (right): https://codereview.chromium.org/1461473002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/autoscroll-scrollable-iframe-div.html:25: setTimeout(autoscrollTestPart2, 100); On 2015/11/19 19:06:44, wkorman wrote: > Did you try doing another layoutAndPaintAsyncThen() here, that calls > autoscrollTestPart2, instead of setTimeout? Done. https://codereview.chromium.org/1461473002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/autoscroll-scrollable-iframe-div.html:40: setTimeout(testFailed, 100); On 2015/11/19 19:06:44, wkorman wrote: > And try layoutAndPaintAsyncThen() here as well. Or, runAfterLayoutAndPaint, see: > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Done. https://codereview.chromium.org/1461473002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/autoscroll-scrollable-iframe-div.html:45: log("FAILED : the autoscroll has failed !"); On 2015/11/19 19:06:44, wkorman wrote: > This only logs FAILED if testrunner exists, but below we always log PASSED > regardless of testrunner. Probably we should log here even without the runner? Done.
https://codereview.chromium.org/1461473002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/autoscroll-scrollable-iframe-div.html (right): https://codereview.chromium.org/1461473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/autoscroll-scrollable-iframe-div.html:66: <iframe id="ScrollIFrame" src="resources/iframe-with-overflow-scrollable-div.html" style="height: 100px; width: 100px"> A meta question -- based on the CL description, the issue occurs due to just a div with overflow, do we actually need the test to use an iframe? If we can do without an iframe the test could be much simpler.
https://codereview.chromium.org/1461473002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/autoscroll-scrollable-iframe-div.html (right): https://codereview.chromium.org/1461473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/autoscroll-scrollable-iframe-div.html:66: <iframe id="ScrollIFrame" src="resources/iframe-with-overflow-scrollable-div.html" style="height: 100px; width: 100px"> On 2015/11/19 22:13:55, wkorman wrote: > A meta question -- based on the CL description, the issue occurs due to just a > div with overflow, do we actually need the test to use an iframe? If we can do > without an iframe the test could be much simpler. I could not reproduce this reliably with just an overflow div. The test cases pointed by users had iframes
lgtm OK, LGTM but let's also see what skobes@ and/or rbyers@ think as I am still somewhat new to this code myself.
FYI commit messages are typically wrapped at ~ 80 cols. https://codereview.chromium.org/1461473002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/autoscroll-scrollable-iframe-div.html (right): https://codereview.chromium.org/1461473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/autoscroll-scrollable-iframe-div.html:4: document.getElementById('console').appendChild(document.createTextNode(msg + '\n')); If you include ../../resources/js-test.js you can just use debug(msg) and get a console div for free. https://codereview.chromium.org/1461473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/autoscroll-scrollable-iframe-div.html:25: function() { more concisely: testRunner.layoutAndPaintAsyncThen(autoscrollTestPart2); https://codereview.chromium.org/1461473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/autoscroll-scrollable-iframe-div.html:44: testFailed(); It's confusing to call testFailed here. Can you have the button click set a var and check it here? e.g. <button ... onclick="buttonWasClicked = true"> function autoscrollTestPart2() { ... simulate click ... shouldBeTrue("buttonWasClicked"); } https://codereview.chromium.org/1461473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/autoscroll-scrollable-iframe-div.html:66: <iframe id="ScrollIFrame" src="resources/iframe-with-overflow-scrollable-div.html" style="height: 100px; width: 100px"> On 2015/11/19 22:32:01, ananta wrote: > On 2015/11/19 22:13:55, wkorman wrote: > > A meta question -- based on the CL description, the issue occurs due to just a > > div with overflow, do we actually need the test to use an iframe? If we can do > > without an iframe the test could be much simpler. > > I could not reproduce this reliably with just an overflow div. The test cases > pointed by users had iframes It would be nice to understand why the iframe is required for the repro. How does the autoscroll get stopped in the non-iframe case? https://codereview.chromium.org/1461473002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/resources/iframe-with-overflow-scrollable-div.html (right): https://codereview.chromium.org/1461473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/resources/iframe-with-overflow-scrollable-div.html:4: document.getElementById("overflow_div").style.overflow = "scroll"; Why is this style changed in onload?
Description was changed from ========== Ensure that the auto scroll state in the AutoscrollController is cleared if the scroll operation clears out the m_autoscrollLayoutObject member. This typically happens when we scroll a div which uses overflow:auto and during the scroll the cursor leaves the bounds of the div. If we don't reset the state then the page gets stuck in the autoscroll state causing it to be unresponsive to clicks etc. BUG=336373 ========== to ========== Ensure that the auto scroll state in the AutoscrollController is cleared if the scroll operation clears out the m_autoscrollLayoutObject member. This typically happens when we scroll a div which uses overflow:auto and during the scroll the cursor leaves the bounds of the div. If we don't reset the state then the page gets stuck in the autoscroll state causing it to be unresponsive to clicks etc. BUG=336373 ==========
Description was changed from ========== Ensure that the auto scroll state in the AutoscrollController is cleared if the scroll operation clears out the m_autoscrollLayoutObject member. This typically happens when we scroll a div which uses overflow:auto and during the scroll the cursor leaves the bounds of the div. If we don't reset the state then the page gets stuck in the autoscroll state causing it to be unresponsive to clicks etc. BUG=336373 ========== to ========== Ensure that the auto scroll state in the AutoscrollController is cleared if the scroll operation clears out the m_autoscrollLayoutObject member. This typically happens when we scroll a div which uses overflow:auto and during the scroll the cursor leaves the bounds of the div. If we don't reset the state then the page gets stuck in the autoscroll state causing it to be unresponsive to clicks etc. BUG=336373 ==========
Description was changed from ========== Ensure that the auto scroll state in the AutoscrollController is cleared if the scroll operation clears out the m_autoscrollLayoutObject member. This typically happens when we scroll a div which uses overflow:auto and during the scroll the cursor leaves the bounds of the div. If we don't reset the state then the page gets stuck in the autoscroll state causing it to be unresponsive to clicks etc. BUG=336373 ========== to ========== Ensure that the auto scroll state in the AutoscrollController is cleared if the scroll operation clears out the m_autoscrollLayoutObject member. This typically happens when we scroll a div inside an iframe which uses overflow:auto and during the scroll the cursor leaves the bounds of the div. If we don't reset the state then the page gets stuck in the autoscroll state causing it to be unresponsive to clicks etc. BUG=336373 ==========
Description was changed from ========== Ensure that the auto scroll state in the AutoscrollController is cleared if the scroll operation clears out the m_autoscrollLayoutObject member. This typically happens when we scroll a div inside an iframe which uses overflow:auto and during the scroll the cursor leaves the bounds of the div. If we don't reset the state then the page gets stuck in the autoscroll state causing it to be unresponsive to clicks etc. BUG=336373 ========== to ========== Ensure that the auto scroll state in the AutoscrollController is cleared if the scroll operation clears out the m_autoscrollLayoutObject member. This typically happens when we scroll a div inside an iframe which uses overflow:auto and during the scroll the cursor leaves the bounds of the div. If we don't reset the state then the page gets stuck in the autoscroll state causing it to be unresponsive to clicks etc. BUG=336373 ==========
Description was changed from ========== Ensure that the auto scroll state in the AutoscrollController is cleared if the scroll operation clears out the m_autoscrollLayoutObject member. This typically happens when we scroll a div inside an iframe which uses overflow:auto and during the scroll the cursor leaves the bounds of the div. If we don't reset the state then the page gets stuck in the autoscroll state causing it to be unresponsive to clicks etc. BUG=336373 ========== to ========== Ensure that the auto scroll state in the AutoscrollController is cleared if the scroll operation clears out the m_autoscrollLayoutObject member. This typically happens when we scroll a div inside an iframe which uses overflow:auto and during the scroll the cursor leaves the bounds of the div. If we don't reset the state then the page gets stuck in the autoscroll state causing it to be unresponsive to clicks etc. BUG=336373 ==========
https://codereview.chromium.org/1461473002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/autoscroll-scrollable-iframe-div.html (right): https://codereview.chromium.org/1461473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/autoscroll-scrollable-iframe-div.html:4: document.getElementById('console').appendChild(document.createTextNode(msg + '\n')); On 2015/11/20 00:10:17, skobes wrote: > If you include ../../resources/js-test.js you can just use debug(msg) and get a > console div for free. Done. https://codereview.chromium.org/1461473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/autoscroll-scrollable-iframe-div.html:4: document.getElementById('console').appendChild(document.createTextNode(msg + '\n')); On 2015/11/20 00:10:17, skobes wrote: > If you include ../../resources/js-test.js you can just use debug(msg) and get a > console div for free. Done. https://codereview.chromium.org/1461473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/autoscroll-scrollable-iframe-div.html:25: function() { On 2015/11/20 00:10:17, skobes wrote: > more concisely: > testRunner.layoutAndPaintAsyncThen(autoscrollTestPart2); Done. https://codereview.chromium.org/1461473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/autoscroll-scrollable-iframe-div.html:25: function() { On 2015/11/20 00:10:17, skobes wrote: > more concisely: > testRunner.layoutAndPaintAsyncThen(autoscrollTestPart2); Done. https://codereview.chromium.org/1461473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/autoscroll-scrollable-iframe-div.html:44: testFailed(); On 2015/11/20 00:10:17, skobes wrote: > It's confusing to call testFailed here. Can you have the button click set a var > and check it here? e.g. > > <button ... onclick="buttonWasClicked = true"> > > function autoscrollTestPart2() { > ... simulate click ... > > shouldBeTrue("buttonWasClicked"); > } Done. https://codereview.chromium.org/1461473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/autoscroll-scrollable-iframe-div.html:44: testFailed(); On 2015/11/20 00:10:17, skobes wrote: > It's confusing to call testFailed here. Can you have the button click set a var > and check it here? e.g. > > <button ... onclick="buttonWasClicked = true"> > > function autoscrollTestPart2() { > ... simulate click ... > > shouldBeTrue("buttonWasClicked"); > } Done. https://codereview.chromium.org/1461473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/autoscroll-scrollable-iframe-div.html:66: <iframe id="ScrollIFrame" src="resources/iframe-with-overflow-scrollable-div.html" style="height: 100px; width: 100px"> On 2015/11/20 00:10:17, skobes wrote: > On 2015/11/19 22:32:01, ananta wrote: > > On 2015/11/19 22:13:55, wkorman wrote: > > > A meta question -- based on the CL description, the issue occurs due to just > a > > > div with overflow, do we actually need the test to use an iframe? If we can > do > > > without an iframe the test could be much simpler. > > > > I could not reproduce this reliably with just an overflow div. The test cases > > pointed by users had iframes > > It would be nice to understand why the iframe is required for the repro. How > does the autoscroll get stopped in the non-iframe case? For the non iframe case the autoscroll is stopped when the mouse release is received. For the iframe case in the AutoscrollController::updateAutoscrollLayoutObject function we hit a null layout object which messes up the state. Dunno why that happens. I have no context on that code. Maybe we can address that as a follow up bug. https://codereview.chromium.org/1461473002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/resources/iframe-with-overflow-scrollable-div.html (right): https://codereview.chromium.org/1461473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/resources/iframe-with-overflow-scrollable-div.html:4: document.getElementById("overflow_div").style.overflow = "scroll"; On 2015/11/20 00:10:17, skobes wrote: > Why is this style changed in onload? Removed.
lgtm
The CQ bit was checked by ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org Link to the patchset: https://codereview.chromium.org/1461473002/#ps60001 (title: "Address review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1461473002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1461473002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) ios_rel_device_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skobes@chromium.org, wkorman@chromium.org Link to the patchset: https://codereview.chromium.org/1461473002/#ps100001 (title: "Another attempt to fix layout test redness")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1461473002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1461473002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a12dafbe77137b234cf65cd5031fd7db2d1ba6fc Cr-Commit-Position: refs/heads/master@{#360997} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
