Bug 64143 - REGRESSION (r60268): Page-Up or Page-Down scrolls a scrollable textarea and the next embedding scrollable area at the same time
Summary: REGRESSION (r60268): Page-Up or Page-Down scrolls a scrollable textarea and t...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-07 19:24 PDT by Kentaro Hara
Modified: 2017-07-18 08:30 PDT (History)
13 users (show)

See Also:


Attachments
Patch (56.38 KB, patch)
2011-07-11 05:30 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.33 MB, application/zip)
2011-07-11 06:47 PDT, WebKit Review Bot
no flags Details
Patch (56.11 KB, patch)
2011-07-12 02:23 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (26.88 KB, patch)
2011-07-12 23:21 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (27.75 KB, patch)
2011-07-13 23:37 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Screen shot on Gtk (367.95 KB, image/png)
2011-07-24 23:48 PDT, Kentaro Hara
no flags Details
Patch (27.80 KB, patch)
2011-07-25 00:02 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (27.95 KB, patch)
2011-08-31 09:55 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
patch for review (28.56 KB, patch)
2011-12-08 18:16 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (28.58 KB, patch)
2011-12-08 23:04 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
rebased patch for review (28.56 KB, patch)
2011-12-18 21:49 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
rebased patch for review (28.50 KB, patch)
2012-01-10 16:31 PST, Kentaro Hara
rniwa: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2011-07-07 19:24:44 PDT
Assume the following code:

many many lines here...
<textarea rows="100">abcde</textarea>
many many lines here...

(1) Move a caret somewhere in the textarea.
(2) Press 'PageUp' key.

Expected behavior: The caret should go at the beginning of the textarea and no scrolling should happen.

Actual behavior: The caret goes at the beginning of the textarea and the page is scrolled to the top.

Reported by http://code.google.com/p/chromium/issues/detail?id=60472
Comment 1 Alexey Proskuryakov 2011-07-08 11:54:03 PDT
I'm pretty sure that this is intentional behavior, and it seems correct to me. We don't block PageUp/PageDown when focus is in other form controls (e.g. a text input). It would be very confusing if PgUp/PgDown refused to scroll the page when focus was in an empty textarea, for example.

Note that we have the same behavior with scrolling - first, a scrollable area under the mouse is scrolled, then the next embedding scrollable area, and so on.
Comment 2 Alexey Proskuryakov 2011-07-08 11:55:35 PDT
I don't know why the original Chrome bug report says that Safari 5 is "OK". Maybe I misunderstood the issue - in any case, bug URL is no longer accessible.
Comment 3 Kentaro Hara 2011-07-10 17:14:04 PDT
Sorry, my description was inaccurate. In particular, the position of a caret is not a problem in this case. 

Expected behavior: First, a scrollable area under the mouse is scrolled, then the next embedding scrollable area is scrolled, and so on. Note that the scrollable area and the embedding scrollable area are never scrolled at the same time.

Actual behavior (in the latest Chromium): First, a scrollable textarea and the next embedding scrollable area are scrolled at the same time. Here is screenshots:

before:
http://haraken.info/a/textarea.png

after pressing PageUp:
http://haraken.info/a/textarea_after.png

I am making a patch for this issue.
Comment 4 Alexey Proskuryakov 2011-07-10 23:23:47 PDT
Yes, scrolling two scrollable areas at once certainly sounds wrong.
Comment 5 Kentaro Hara 2011-07-11 05:30:03 PDT
Created attachment 100282 [details]
Patch
Comment 6 Kentaro Hara 2011-07-11 05:33:56 PDT
If this patch is OK, I would like to later update the results of fast/layers/scroll-rect-to-visible.html as a rebaseline.
Comment 7 WebKit Review Bot 2011-07-11 06:47:22 PDT
Comment on attachment 100282 [details]
Patch

Attachment 100282 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9014195

New failing tests:
http/tests/misc/slow-loading-mask.html
fast/blockflow/japanese-rl-selection.html
svg/transforms/text-with-mask-with-svg-transform.svg
fast/backgrounds/background-leakage.html
fast/box-shadow/scaled-box-shadow.html
fast/backgrounds/repeat/negative-offset-repeat.html
svg/W3C-SVG-1.1/struct-use-01-t.svg
transforms/2d/hindi-rotated.html
fast/overflow/scrollRevealButton.html
svg/repaint/filter-repaint.svg
fast/blockflow/japanese-lr-selection.html
Comment 8 WebKit Review Bot 2011-07-11 06:47:27 PDT
Created attachment 100291 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 9 Alexey Proskuryakov 2011-07-11 08:27:44 PDT
> Actual behavior (in the latest Chromium): First, a scrollable textarea and the next embedding scrollable area are scrolled at the same time. Here is screenshots:

This seems to only affect Chrome, so fixing the problem by modifying cross platform code needs a very good explanation.

Is this a regression? Can you find out when it started?
Comment 10 Kentaro Hara 2011-07-12 02:23:38 PDT
Created attachment 100455 [details]
Patch
Comment 11 Kentaro Hara 2011-07-12 02:24:55 PDT
Thank you very much for the review.

> This seems to only affect Chrome, so fixing the problem by modifying cross platform code needs a very good explanation.

Yes, this seems to affect only Chrome. This problem does not happen on Mac Safari since it uses its own rendering code for scroll operations (Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm etc...).


> Is this a regression? Can you find out when it started?

Regression. I found that the problem starts from r60268 (http://trac.webkit.org/changeset/60268/), which changed WebCore codes. The attached test (pageup-and-pagedown-in-textarea.html) passes r60267, but does not pass r60268.
Comment 12 Kent Tamura 2011-07-12 03:06:19 PDT
(In reply to comment #11)
> Thank you very much for the review.
> 
> > This seems to only affect Chrome, so fixing the problem by modifying cross platform code needs a very good explanation.
> 
> Yes, this seems to affect only Chrome. This problem does not happen on Mac Safari since it uses its own rendering code for scroll operations (Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm etc...).

What about other ports such as win, gtk, qt?
Comment 13 Kent Tamura 2011-07-12 03:13:43 PDT
Comment on attachment 100455 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100455&action=review

I'm not familiar with RenderLayer and I won't set r+, but I have some comments.

> LayoutTests/ChangeLog:15
> +
> +        * editing/input/pageup-and-pagedown-in-textarea-expected.txt: Added.
> +        * editing/input/pageup-and-pagedown-in-textarea.html: Added.
> +

The file list is out-of-sync.  You updated other files.

> LayoutTests/editing/input/pageup-and-pagedown-in-textarea-expected.txt:33
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.
> +Many many lines here.

These lines are meaningless.  Please remove them before the test completion.

> LayoutTests/editing/input/pageup-and-pagedown-in-textarea.html:76
> +}

Please show something if there is no layoutTestController.
If we can test manually on a browser, show manual test instructions. Otherwise, show an excuse message about the requirement of layoutTestController.

> LayoutTests/platform/chromium-win/fast/layers/scroll-rect-to-visible-expected.txt:-62
>          text run at (0,0) width 11: "A"
> -scrolled to 0,13

You updated a text expectation for chromium-win but not updated an image expectation. So this will break Chromium-win buidbots.
Please add this test to LayoutTests/platform/chromium/test_expectations.txt to avoid the breakage.
Comment 14 Alexey Proskuryakov 2011-07-12 08:40:17 PDT
CC'ing people who worked on r60268.
Comment 15 Adele Peterson 2011-07-12 14:31:49 PDT
Comment on attachment 100455 [details]
Patch

This change is at far too low a level.  Did you test other callers of scrollRectToVisible?  I believe many other callers expect the current behavior, so this affects a lot more functionality than just pageUp/pageDown in text areas.
Comment 16 Kentaro Hara 2011-07-12 23:21:09 PDT
Created attachment 100633 [details]
Patch
Comment 17 Kentaro Hara 2011-07-12 23:21:45 PDT
> > > This seems to only affect Chrome, so fixing the problem by modifying cross platform code needs a very good explanation.
> > 
> > Yes, this seems to affect only Chrome. This problem does not happen on Mac Safari since it uses its own rendering code for scroll operations (Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm etc...).
> 
> What about other ports such as win, gtk, qt?

It seems that win, gtk and qt have their own editor client and handleKeyboardEvent(), although I do not yet confirm it by the real execution.
Comment 18 Kentaro Hara 2011-07-12 23:21:59 PDT
Comment on attachment 100455 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100455&action=review

>> LayoutTests/ChangeLog:15
>> +
> 
> The file list is out-of-sync.  You updated other files.

Sorry, fixed it.

>> LayoutTests/editing/input/pageup-and-pagedown-in-textarea-expected.txt:33
>> +Many many lines here.
> 
> These lines are meaningless.  Please remove them before the test completion.

Done.

>> LayoutTests/editing/input/pageup-and-pagedown-in-textarea.html:76
>> +}
> 
> Please show something if there is no layoutTestController.
> If we can test manually on a browser, show manual test instructions. Otherwise, show an excuse message about the requirement of layoutTestController.

Done. I described how to run this test manually.

>> LayoutTests/platform/chromium-win/fast/layers/scroll-rect-to-visible-expected.txt:-62
>> -scrolled to 0,13
> 
> You updated a text expectation for chromium-win but not updated an image expectation. So this will break Chromium-win buidbots.
> Please add this test to LayoutTests/platform/chromium/test_expectations.txt to avoid the breakage.

- I reverted chromium-win/fast/layers/scroll-rect-to-visible-expected.txt to the original.
- I added chromium-linux/fast/layers/scroll-rect-to-visible-expected.txt, so that scroll-rect-to-visible.html passes on Linux chromium.
- I added "BUGWK64143 WIN MAC : fast/layers/scroll-rect-to-visible.html = FAIL" to test_expectations.txt. I would like to remove this failure as a rebase-line after this patch is landed.
Comment 19 Kentaro Hara 2011-07-12 23:22:17 PDT
> This change is at far too low a level.  Did you test other callers of scrollRectToVisible?  I believe many other callers expect the current behavior, so this affects a lot more functionality than just pageUp/pageDown in text areas.

Makes sense. I fixed the patch so that it affects only the behaviors of PageUp and PageDown. Would you please take a look at it?
Comment 20 Alexey Proskuryakov 2011-07-13 08:35:44 PDT
> Regression. I found that the problem starts from r60268 (http://trac.webkit.org/changeset/60268/), which changed WebCore codes. The attached test (pageup-and-pagedown-in-textarea.html) passes r60267, but does not pass r60268.

If this problem started with a cross platform patch, then it could indeed be that a proper fix is in cross platform code. But given that no other platform apparently has this problem, it's still far from a proven fact.

The ChangeLog describes what was done, but not why. Is cross platform code buggy? Or is it clearly insufficient in some way (but then why other platforms manage to work)?
Comment 21 Kentaro Hara 2011-07-13 23:03:08 PDT
> If this problem started with a cross platform patch, then it could indeed be that a proper fix is in cross platform code. But given that no other platform apparently has this problem, it's still far from a proven fact.

I think that this is because mac, win, gtk and qt have their own editor client and handleKeyboardEvent(), and thus do not use the WebCore codes. For example, in case of Mac Safari, I confirmed that it uses Source/WebKit/mac/WebView/WebFrameView.mm for rendering scroll operations, instead of the WebCore codes. 


> The ChangeLog describes what was done, but not why. Is cross platform code buggy? Or is it clearly insufficient in some way (but then why other platforms manage to work)?

I hope that Zelidrag will reply more in detail about why, but my guess for what happened around r60268 is as follows:

- Before r60268, PageUp and PageDown in textarea were using ScrollAlignment::alignToEdgeIfNeeded for their alignment strategy.
- With respect to this bug (i.e. PageUp and PageDown scroll a parent layer even if the child layer is scrollable), the implementation of alignToEdgeIfNeeded was correct but the implementation of alignTopAlways was wrong, as of before r60268. 
- r60268 changed the alignment strategy from alignToEdgeIfNeeded to alignTopAlways. 
- This accounts for why my test (pageup-and-pagedown-in-textarea.html) passes at r60267 but does not pass at r60268. 

And, in this patch, I guess that I am now trying to fix the bug of alignTopAlways, which was existing since before r60268.
Comment 22 Kentaro Hara 2011-07-13 23:37:45 PDT
Created attachment 100773 [details]
Patch
Comment 23 Alexey Proskuryakov 2011-07-13 23:45:11 PDT
Mac Safari is indeed special, as it's using platform views (NSViews) for frames. All other platforms (including Mac WebKit2) should work identically. Even though a platform-specific handleKeyboardEvent method is called by each, we should be returning on a cross platform code path in MovePageDown (or MovePageDownAndModifySelection) editor command.

I think that this needs deeper investigation.
Comment 24 Kentaro Hara 2011-07-24 23:48:17 PDT
Created attachment 101851 [details]
Screen shot on Gtk

ap: Thank you very much for the comments. 

> Mac Safari is indeed special, as it's using platform views (NSViews) for frames. All other platforms (including Mac WebKit2) should work identically. Even though a platform-specific handleKeyboardEvent method is called by each, we should be returning on a cross platform code path in MovePageDown (or MovePageDownAndModifySelection) editor command.
> 
> I think that this needs deeper investigation.

I confirmed that the similar problem happens on not only Chromium but also Gtk with the latest WebKit r91650. I attached a screen shot on Gtk. When we press PageUp in Gtk, the page scrolls up and right so that the position of a caret corresponds to the top left corner of the page. I also confirmed that my patch solves the problem. So I think that the change on WebCore codes is reasonable.
Comment 25 Kentaro Hara 2011-07-25 00:02:54 PDT
Created attachment 101852 [details]
Patch
Comment 26 Kentaro Hara 2011-07-26 17:22:14 PDT
Zelidrag: would you please comment on this patch?
Comment 27 Kentaro Hara 2011-08-31 09:55:58 PDT
Created attachment 105785 [details]
Patch
Comment 28 Darin Adler 2011-08-31 10:30:41 PDT
Comment on attachment 105785 [details]
Patch

The current behavior is intentional, so we should discuss the design change in webkit-dev before we put in a patch to change the behavior.
Comment 29 Darin Adler 2011-08-31 10:47:02 PDT
Alexey told me I am wrong and this is actually something unintentional.
Comment 30 Kentaro Hara 2011-08-31 10:55:08 PDT
(In reply to comment #29)
> Alexey told me I am wrong and this is actually something unintentional.

Darin: Thank you very much. I am sorry, my first comment of this bug thread is inaccurate. Please see my second comment of this bug thread for the accurate description of the problem.
Comment 31 Darin Adler 2011-08-31 10:57:01 PDT
Might have to retitle this for clarity.
Comment 32 Ryosuke Niwa 2011-12-08 16:45:24 PST
Comment on attachment 105785 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105785&action=review

r- due to a missing test.

> LayoutTests/editing/input/pageup-and-pagedown-in-textarea.html:72
> +    debug("Press PageDown on a scrollable textarea.");
> +    scrollTo(0, 0);
> +    textarea.focus();
> +    textarea.setSelectionRange(29, 29); // Move a caret to the tail of '09', i.e. it looks like '09|'.
> +    textarea.scrollTop = textarea.scrollHeight / 2;
> +    previousScrollTopOfDocument = document.body.scrollTop;
> +    previousScrollLeftOfDocument = document.body.scrollLeft;
> +    eventSender.keyDown("pageDown");
> +    shouldBe('document.body.scrollTop', 'previousScrollTopOfDocument');
> +    shouldBe('document.body.scrollLeft', 'previousScrollLeftOfDocument');
> +    textarea.blur();

Seems like code duplication. Please make a function and share code.

> LayoutTests/platform/chromium/test_expectations.txt:3204
> +BUGWK64143 WIN MAC : fast/layers/scroll-rect-to-visible.html = FAIL

Where is this test? It's missing from the patch.

> Source/WebCore/editing/FrameSelection.cpp:247
> +            switch (align) {
> +            case AlignCursorOnScrollAlways:
> +                alignment = ScrollAlignment::alignCenterAlways;
> +                break;
> +            case AlignCursorOnScrollIfNeeded:
> +                alignment = ScrollAlignment::alignCenterIfNeeded;
> +                break;
> +            case AlignCursorOnScrollIfScrollable:
> +                alignment = ScrollAlignment::alignCenterIfScrollable;
> +                break;

Wish there was a cleaner way of writing this. Could you at least put this in some helper function?

> Source/WebCore/editing/FrameSelection.cpp:260
> +            switch (align) {
> +            case AlignCursorOnScrollAlways:
> +                alignment = ScrollAlignment::alignTopAlways;
> +                break;
> +            case AlignCursorOnScrollIfNeeded:
> +                alignment = ScrollAlignment::alignToEdgeIfNeeded;
> +                break;
> +            case AlignCursorOnScrollIfScrollable:
> +                alignment = ScrollAlignment::alignTopIfScrollable;
> +                break;
> +            }

Ditto.

> Source/WebCore/editing/FrameSelection.h:113
>      enum CursorAlignOnScroll { AlignCursorOnScrollIfNeeded,
> -                               AlignCursorOnScrollAlways };
> +                               AlignCursorOnScrollAlways,
> +                               AlignCursorOnScrollIfScrollable };

I don't understand the difference between IfNeeded and IfScrollable. We need a better name for these two be distinguishable.
Comment 33 Kentaro Hara 2011-12-08 18:16:24 PST
Created attachment 118506 [details]
patch for review
Comment 34 Kentaro Hara 2011-12-08 18:16:48 PST
Comment on attachment 105785 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105785&action=review

>> LayoutTests/editing/input/pageup-and-pagedown-in-textarea.html:72
>> +    textarea.blur();
> 
> Seems like code duplication. Please make a function and share code.

Done.

>> LayoutTests/platform/chromium/test_expectations.txt:3204
>> +BUGWK64143 WIN MAC : fast/layers/scroll-rect-to-visible.html = FAIL
> 
> Where is this test? It's missing from the patch.

This is an existing test and is not a newly added test. With the change of this patch, fast/layers/scroll-rect-to-visible-expected.txt needs to be rebaselined. I'll do it after this patch is landed.

>> Source/WebCore/editing/FrameSelection.cpp:247
>> +                break;
> 
> Wish there was a cleaner way of writing this. Could you at least put this in some helper function?

Done.

>> Source/WebCore/editing/FrameSelection.cpp:260
>> +            }
> 
> Ditto.

Done.

>> Source/WebCore/editing/FrameSelection.h:113
>> +                               AlignCursorOnScrollIfScrollable };
> 
> I don't understand the difference between IfNeeded and IfScrollable. We need a better name for these two be distinguishable.

I renamed ScrollAlignment::alignTopIfScrollable to ScrollAlignment::alignTopOfScrollableElement to clarify that it does not scroll a parent layer if any of its child layer is scrollable.
Comment 35 WebKit Review Bot 2011-12-08 22:35:41 PST
Comment on attachment 118506 [details]
patch for review

Attachment 118506 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10781605
Comment 36 Kentaro Hara 2011-12-08 23:04:11 PST
Created attachment 118535 [details]
Patch
Comment 37 Kentaro Hara 2011-12-18 21:49:58 PST
Created attachment 119813 [details]
rebased patch for review
Comment 38 Kentaro Hara 2011-12-18 21:50:38 PST
rniwa: Would you please take a look?
Comment 39 Eric Seidel (no email) 2011-12-21 11:40:06 PST
rniwa or darin adler is your man here.
Comment 40 Darin Adler 2011-12-22 08:56:23 PST
Comment on attachment 119813 [details]
rebased patch for review

I downloaded the test case and tested it manually. It works fine on Mac Safari 5.1.2 without any code changes. Are you sure this is a cross-platform bug and not just a Chromium port mistake? I am not sure all this churn in WebCore is needed.
Comment 41 Darin Adler 2011-12-22 08:57:22 PST
(In reply to comment #40)
> I downloaded the test case and tested it manually. It works fine on Mac Safari 5.1.2 without any code changes. Are you sure this is a cross-platform bug and not just a Chromium port mistake? I am not sure all this churn in WebCore is needed.

I now see the earlier comments that say “Mac Safari is OK”.
Comment 42 Kentaro Hara 2011-12-22 15:43:49 PST
(In reply to comment #41)
> (In reply to comment #40)
> > I downloaded the test case and tested it manually. It works fine on Mac Safari 5.1.2 without any code changes. Are you sure this is a cross-platform bug and not just a Chromium port mistake? I am not sure all this churn in WebCore is needed.
> 
> I now see the earlier comments that say “Mac Safari is OK”.

Yeah, Safari is using Source/WebKit/mac/WebView/WebFrameView.mm for rendering scroll operations, instead of the WebCore code. I've confirmed that this issue also happens on Gtk.
Comment 43 Kentaro Hara 2012-01-10 16:31:09 PST
Created attachment 121936 [details]
rebased patch for review
Comment 44 Kentaro Hara 2012-01-10 16:31:52 PST
darin, rniwa: ping for review?
Comment 45 Ryosuke Niwa 2012-04-22 22:26:00 PDT
Comment on attachment 121936 [details]
rebased patch for review

View in context: https://bugs.webkit.org/attachment.cgi?id=121936&action=review

> Source/WebCore/ChangeLog:11
> +        strategies, AlignCursorOnScrollOfScrollableElement, which does not scroll a parent layer

I don't think "of" is a good preposition here. How about "inside"? i.e. AlignCursorOnScrollInsideScrollableElement
Or maybe AlignCursorOnScrollInInnerMostScrollableElement since you're only scrolling in the inner most one if you have multiple scrollable elements?

> Source/WebCore/ChangeLog:13
> +

Could you also explain how you're implement this new behavior using alignCenterIfChildIsNotScrollable?

> Source/WebCore/editing/FrameSelection.cpp:445
> +ScrollAlignment FrameSelection::getScrollAlignment(CursorAlignOnScroll align, bool shouldCenterAlignWhenSelectionIsRevealed)

We use "get" prefix iff it has an out argument.
Also, this function can be static local.

> Source/WebCore/editing/FrameSelection.cpp:456
> +    } else {

No else since the previous "if" block all ends with "return".

> Source/WebCore/rendering/RenderLayer.cpp:1586
> +    if (childScrollable && (ScrollAlignment::getVisibleBehavior(alignX) == alignCenterIfChildIsNotScrollable || ScrollAlignment::getVisibleBehavior(alignX) == alignTopIfChildIsNotScrollable))

Can we add a helper inline function?

> LayoutTests/platform/chromium-linux/fast/layers/scroll-rect-to-visible-expected.txt:2
> +layer at (0,0) size 785x664
> +  RenderView at (0,0) size 785x600

Where is scroll-rect-to-visible.html added? r- because you seem to be missing the test for this expectation.
Comment 46 Ryosuke Niwa 2012-04-22 22:38:51 PDT
Also see https://bugs.webkit.org/show_bug.cgi?id=82875.