[webkit-reviews] review denied: [Bug 38213] Page up / page down moves edited content for the entire height : [Attachment 56100] Review comment fixed, handling of non-iframe contenteditable cases

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied David Levin <levin at chromium.org>'s
request for review:
Bug 38213: Page up / page down moves edited content for the entire height
https://bugs.webkit.org/show_bug.cgi?id=38213

Attachment 56100: Review comment fixed, handling of non-iframe contenteditable
cases
https://bugs.webkit.org/attachment.cgi?id=56100&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
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.