Bug 64143 – REGRESSION (r60268): Page-Up or Page-Down scrolls a scrollable textarea and the next embedding scrollable area at the same time

• Component: Forms
• Status: NEW

[Kentaro Hara]

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

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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.

[Kentaro Hara]

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.

[Alexey Proskuryakov]

Yes, scrolling two scrollable areas at once certainly sounds wrong.

[Kentaro Hara]

Created attachment 100282 [details]
Patch

[Kentaro Hara]

If this patch is OK, I would like to later update the results of fast/layers/scroll-rect-to-visible.html as a rebaseline.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Alexey Proskuryakov]

> 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?

[Kentaro Hara]

Created attachment 100455 [details]
Patch

[Kentaro Hara]

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.

[Kent Tamura]

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

[Kent Tamura]

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.

[Alexey Proskuryakov]

CC'ing people who worked on r60268.

[Adele Peterson]

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.

[Kentaro Hara]

Created attachment 100633 [details]
Patch

[Kentaro Hara]

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

[Kentaro Hara]

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.

[Kentaro Hara]

> 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?

[Alexey Proskuryakov]

> 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)?

[Kentaro Hara]

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

[Kentaro Hara]

Created attachment 100773 [details]
Patch

[Alexey Proskuryakov]

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.

[Kentaro Hara]

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.

[Kentaro Hara]

Created attachment 101852 [details]
Patch

[Kentaro Hara]

Zelidrag: would you please comment on this patch?

[Kentaro Hara]

Created attachment 105785 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

Alexey told me I am wrong and this is actually something unintentional.

[Kentaro Hara]

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

[Darin Adler]

Might have to retitle this for clarity.

[Ryosuke Niwa]

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.

[Kentaro Hara]

Created attachment 118506 [details]
patch for review

[Kentaro Hara]

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.

[WebKit Review Bot]

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

[Kentaro Hara]

Created attachment 118535 [details]
Patch

[Kentaro Hara]

Created attachment 119813 [details]
rebased patch for review

[Kentaro Hara]

rniwa: Would you please take a look?

[Eric Seidel (no email)]

rniwa or darin adler is your man here.

[Darin Adler]

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.

[Darin Adler]

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

[Kentaro Hara]

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

[Kentaro Hara]

Created attachment 121936 [details]
rebased patch for review

[Kentaro Hara]

darin, rniwa: ping for review?

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Also see https://bugs.webkit.org/show_bug.cgi?id=82875.