Instead of adding ::allowScrolling calls to check scrollability, check if scrolling in allowed in the direction where overflow happens. That allow methods like RenderLayer::updateScrollbarsAfterLayout and RenderLayer::updateScrollbarsAfterStyleChange to be scrollbars-agnostic. Reason behind it: scrollbars can be disabled completely (not only "hidden") in WebCore, and its drawing delegated to the client application.
Created attachment 177484 [details] patch
Comment on attachment 177484 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=177484&action=review It would be nice to only have the "scrollsOverflowX() && scrollWidth() != clientWidth()" logic in one place; here it's in both RenderLayer and RenderBox, and the various width implementations are subtly different. > Source/WebCore/rendering/RenderBox.cpp:621 > + bool hasOverflowAndAllowsScrollingInThatDirection = (scrollsOverflowX() && scrollWidth() != clientWidth()) Awkward variable name. How about hasScrollableOverflow?
Created attachment 177756 [details] patch2 - take simon's feedback into account
Comment on attachment 177756 [details] patch2 - take simon's feedback into account View in context: https://bugs.webkit.org/attachment.cgi?id=177756&action=review Thanks Antonio, I think this makes the logic clearer. > Source/WebCore/ChangeLog:17 > + The naming pattern for the newly added methods were choosen to keep the consitency s/choosen/chosen/, s/consitency/consistency/. > Source/WebCore/ChangeLog:20 > + No new tests, as it is a refactor. Also it is already covered by ScrollingCoordinatorChromiumTest.clippedBodyTest. Not exactly a refactor since it makes canBeScrolledAndHasScrollableArea() more strict, although I can't think of any bad effects from doing so.
Created attachment 178009 [details] (landed r136947, r=jamesr) patch 2.1 - take Sami's feedback into account
(In reply to comment #5) > Created an attachment (id=178009) [details] > patch 2.1 - take Sami's feedback into account Kindly ping reviewers.
Created attachment 178200 [details] patch for EWS
Comment on attachment 178009 [details] (landed r136947, r=jamesr) patch 2.1 - take Sami's feedback into account https://trac.webkit.org/changeset/136947
(In reply to comment #8) > (From update of attachment 178009 [details]) > https://trac.webkit.org/changeset/136947 It broke 2 tests on all platform: - fast/events/autoscroll-in-textfield.html - fast/events/autoscroll-should-not-stop-on-keypress.html --- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/events/autoscroll-in-textfield-expected.txt +++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/events/autoscroll-in-textfield-actual.txt @@ -1,3 +1,3 @@ https://bugs.webkit.org/show_bug.cgi?id=20201 To do the test manually you have to try triggering the autoscroll by starting the dragging from within the text field and moving to the right. If the autoscroll occurs the test has PASSED. -PASSED +FAILED the textfield should have been scrolled --- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/events/autoscroll-should-not-stop-on-keypress-expected.txt +++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/events/autoscroll-should-not-stop-on-keypress-actual.txt @@ -1,5 +1,5 @@ -PASS frame.contentDocument.body.scrollTop + frame.clientHeight is frame.contentDocument.body.scrollHeight +FAIL frame.contentDocument.body.scrollTop + frame.clientHeight should be 2022. Was 400. PASS successfullyParsed is true TEST COMPLETE
(In reply to comment #9) > (In reply to comment #8) > > (From update of attachment 178009 [details] [details]) > > https://trac.webkit.org/changeset/136947 > > It broke 2 tests on all platform: > - fast/events/autoscroll-in-textfield.html > - fast/events/autoscroll-should-not-stop-on-keypress.html Saw it. Checking...
Unfortunately, EWS for some reason can not apply the patch and run the tests in advance. I will likely revert the RenderBox part of the patch, and track is separately. Testing locally first. to See if it fixes the tests.
Unfortunately, EWS for some reason can not apply the patch and run the tests in advance (abarth, would you know why?). I will likely revert the RenderBox part of the patch, and track is separately. Testing locally first to see if it fixes the tests.
(In reply to comment #12) ... > I will likely revert the RenderBox part of the patch, and track is separately. Filed bug 104373
(In reply to comment #11) > Unfortunately, EWS for some reason can not apply the patch and run the tests in advance. > > I will likely revert the RenderBox part of the patch, and track is separately. Testing locally first. to See if it fixes the tests. Fixed by https://trac.webkit.org/changeset/136954