Bug 103999 - Rework bug 97927 to not depend on RenderLayer::allowsScrolling
Summary: Rework bug 97927 to not depend on RenderLayer::allowsScrolling
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antonio Gomes
URL:
Keywords:
Depends on: 104368
Blocks: 97927
  Show dependency treegraph
 
Reported: 2012-12-04 07:27 PST by Antonio Gomes
Modified: 2012-12-12 08:34 PST (History)
9 users (show)

See Also:


Attachments
patch (4.83 KB, patch)
2012-12-04 08:11 PST, Antonio Gomes
simon.fraser: review-
Details | Formatted Diff | Diff
patch2 - take simon's feedback into account (11.86 KB, patch)
2012-12-05 08:27 PST, Antonio Gomes
no flags Details | Formatted Diff | Diff
(landed r136947, r=jamesr) patch 2.1 - take Sami's feedback into account (12.04 KB, patch)
2012-12-06 07:16 PST, Antonio Gomes
no flags Details | Formatted Diff | Diff
patch for EWS (12.10 KB, patch)
2012-12-07 05:03 PST, Antonio Gomes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Gomes 2012-12-04 07:27:43 PST
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.
Comment 1 Antonio Gomes 2012-12-04 08:11:46 PST
Created attachment 177484 [details]
patch
Comment 2 Simon Fraser (smfr) 2012-12-04 13:04:49 PST
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?
Comment 3 Antonio Gomes 2012-12-05 08:27:17 PST
Created attachment 177756 [details]
patch2 - take simon's feedback into account
Comment 4 Sami Kyöstilä 2012-12-06 06:46:16 PST
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.
Comment 5 Antonio Gomes 2012-12-06 07:16:36 PST
Created attachment 178009 [details]
(landed r136947, r=jamesr) patch 2.1 - take Sami's feedback into account
Comment 6 Antonio Gomes 2012-12-06 15:53:44 PST
(In reply to comment #5)
> Created an attachment (id=178009) [details]
> patch 2.1 - take Sami's feedback into account

Kindly ping reviewers.
Comment 7 Antonio Gomes 2012-12-07 05:03:30 PST
Created attachment 178200 [details]
patch for EWS
Comment 8 Antonio Gomes 2012-12-07 05:15:37 PST
Comment on attachment 178009 [details]
(landed r136947, r=jamesr) patch 2.1 - take Sami's feedback into account

https://trac.webkit.org/changeset/136947
Comment 9 Csaba Osztrogonác 2012-12-07 06:08:00 PST
(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
Comment 10 Antonio Gomes 2012-12-07 06:14:45 PST
(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...
Comment 11 Antonio Gomes 2012-12-07 06:38:47 PST
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.
Comment 12 Antonio Gomes 2012-12-07 06:39:34 PST
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.
Comment 13 Antonio Gomes 2012-12-07 08:30:36 PST
(In reply to comment #12)
...
> I will likely revert the RenderBox part of the patch, and track is separately. 

Filed bug 104373
Comment 14 Antonio Gomes 2012-12-12 08:34:24 PST
(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