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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=64143


Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #105785|review?                     |review-
               Flag|                            |




--- Comment #32 from Ryosuke Niwa <rniwa at webkit.org>  2011-12-08 16:45:24 PST ---
(From update of attachment 105785)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.