[webkit-reviews] review denied: [Bug 64143] REGRESSION (r60268): Page-Up or Page-Down scrolls a scrollable textarea and the next embedding scrollable area at the same time : [Attachment 105785] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Kentaro Hara <haraken at google.com>'s
request for review:
Bug 64143: REGRESSION (r60268): Page-Up or Page-Down scrolls a scrollable
textarea and the next embedding scrollable area at the same time
https://bugs.webkit.org/show_bug.cgi?id=64143

Attachment 105785: Patch
https://bugs.webkit.org/attachment.cgi?id=105785&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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.