Bug 103999 – Rework bug 97927 to not depend on RenderLayer::allowsScrolling

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Antonio Gomes]

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.

[Antonio Gomes]

Created attachment 177484 [details]
patch

[Simon Fraser (smfr)]

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?

[Antonio Gomes]

Created attachment 177756 [details]
patch2 - take simon's feedback into account

[Sami Kyöstilä]

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.

[Antonio Gomes]

Created attachment 178009 [details]
(landed r136947, r=jamesr) patch 2.1 - take Sami's feedback into account

[Antonio Gomes]

(In reply to comment #5)
> Created an attachment (id=178009) [details]
> patch 2.1 - take Sami's feedback into account

Kindly ping reviewers.

[Antonio Gomes]

Created attachment 178200 [details]
patch for EWS

[Antonio Gomes]

Comment on attachment 178009 [details]
(landed r136947, r=jamesr) patch 2.1 - take Sami's feedback into account

https://trac.webkit.org/changeset/136947

[Csaba Osztrogonác]

(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

[Antonio Gomes]

(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...

[Antonio Gomes]

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.

[Antonio Gomes]

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.

[Antonio Gomes]

(In reply to comment #12)
...
> I will likely revert the RenderBox part of the patch, and track is separately. 

Filed bug 104373

[Antonio Gomes]

(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