[Webkit-unassigned] [Bug 38213] Page up / page down moves edited content for the entire height

[bugzilla-daemon at webkit.org]

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


Ojan Vafai <ojan at chromium.org> changed:

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




--- Comment #29 from Ojan Vafai <ojan at chromium.org>  2010-05-18 17:09:43 PST ---
(From update of attachment 56100)
Thanks for adding all the extra testing. Just a few more things before committing this. Thanks for being patient.

> Index: WebCore/editing/SelectionController.cpp
> @@ -161,8 +161,15 @@ void SelectionController::setSelection(c
> +    if (userTriggered) {
> +        ScrollAlignment alignment;
> +        if (m_frame->settings()->editingBehavior() == EditingMacBehavior)

You're fine not null-checking m_frame, but I think settings() still needs a null-check. Specifically, if m_frame's m_page is nulled out, then settings() returns null.

> Index: LayoutTests/editing/input/option-page-up-down-inpage.html
> Index: LayoutTests/editing/input/option-page-up-down.html

Can you combine these two into one test? See my example below for a similar case.

> Index: LayoutTests/editing/input/scroll-viewport-page-up-down.html
> Index: LayoutTests/editing/input/scroll-viewport-page-up-down-div.html
I think you could combine these two into one test without much work. More importantly, I don't see what scroll-viewport-page-up-down-div.html tests that scroll-viewport-page-up-down.html doesn't. I see that there are extra contentEditable divs there, but they're not actually the ones getting scrolled.

Also, these tests shouldn't need a fudge factor. If the issue is text rendering, you should be able to get around it by using the "ahem" font-family. That font is specifically for testing. Every character has the same width/height.

How about something like this? I haven't actually ran this, so I'm sure I got some bits wrong.

<div>Page up/down (option+page up/down on Mac) should move the move cursor and scroll the content
in contenteditable elements. This sample covers scroll position test of iframe element containing
contenteditable body. Even though the cursor will move exactly to the same location on all platforms
(covered by test option-page-up-down.html), please note that Mac will scroll the visible area
by placing the cursor position in the middle. All other platforms will scroll by
keeping the cursor aligned with the top edge of the visible area. 
</div>
<div id="editable" contenteditable="true" style="height:150px;overflow:auto;"></div>
<iframe src="../resources/contenteditable-iframe-src.html"></iframe>
<div id="results">FAIL</div>
<script>
if (window.layoutTestController)
    layoutTestController.dumpAsText();

function runTest(scrollable) {
    var tolerance = 10;
    for (var i = 0; i < 10; ++i)
        scrollable.innerHTML += "<p>line " + i + "</p>\n";
    frame.focus();
    frame.getSelection().setPosition(scrollable.firstChild, 0);

    var onMacPlatform = navigator.userAgent.search(/\bMac OS X\b/) != -1;
    var modifiers = onMacPlatform ? ["altKey"] : [];

    if (onMacPlatform)
        offsets = [ 51, 159 ];
    else 
        offsets = [ 116, 218 ];

    if (!window.eventSender)
        return;

    eventSender.keyDown("pageDown", modifiers);
    if (Math.abs(scrollable.scrollTop - offsets[0]) > tolerance) {
        throw "Frame viewport should be around " + offsets[0] +
            "px , not at " + scrollable.scrollTop;
    }

    eventSender.keyDown("pageDown", modifiers);
    var line = frame.getSelection().baseNode.nodeValue;
    if (Math.abs(scrollable.scrollTop - offsets[1]) > tolerance) {
        throw "Frame viewport should be around " + offsets[1] +
            "px , not " + scrollable.scrollTop;
    }

    eventSender.keyDown("pageUp", modifiers);
    var line = frame.getSelection().baseNode.nodeValue;
    if (Math.abs(scrollable.scrollTop - offsets[0]) > tolerance) {
        throw "Frame viewport should be around " + offsets[0] +
            "px , not at " + scrollable.scrollTop;
    }

    document.getElementById("results").innerText += "PASS<br>";
}

runTest(document.getElementById('editable'));
runTest(frames[0].document.body);
</script>


> +<body onload="runTest()">
> +<iframe src="../resources/contenteditable-iframe-src.html" onload="runTest()"></iframe>

This isn't relevant if you change the test as I suggested above, but for future reference, there is a race condition here. runTest() can get called twice before the test finishes.

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