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
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.
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.
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.
Yes, scrolling two scrollable areas at once certainly sounds wrong.
Created attachment 100282 [details] Patch
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 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
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
> 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?
Created attachment 100455 [details] Patch
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.
(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 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.
CC'ing people who worked on r60268.
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.
Created attachment 100633 [details] Patch
> > > 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 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.
> 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?
> 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)?
> 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.
Created attachment 100773 [details] Patch
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.
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.
Created attachment 101852 [details] Patch
Zelidrag: would you please comment on this patch?
Created attachment 105785 [details] Patch
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.
Alexey told me I am wrong and this is actually something unintentional.
(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.
Might have to retitle this for clarity.
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.
Created attachment 118506 [details] patch for review
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 on attachment 118506 [details] patch for review Attachment 118506 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10781605
Created attachment 118535 [details] Patch
Created attachment 119813 [details] rebased patch for review
rniwa: Would you please take a look?
rniwa or darin adler is your man here.
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.
(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”.
(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.
Created attachment 121936 [details] rebased patch for review
darin, rniwa: ping for review?
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.
Also see https://bugs.webkit.org/show_bug.cgi?id=82875.