Page up / page down moves edited content for the entire height

[Zelidrag Hornung]

Frame page up / page down scroll height calculation seems to be broken in EditorCommand.cpp. Function verticalScrollDistance() assumes that the scrolling distance for these operations is the entire client height instead of the visible portion of it. 

Repro Steps:
1. Load a Google Doc, or any other page hosting an iframe that has vertical scrollbar.
2. Select the frame content, then press Page Down to page down through the doc, one page at a time.

Expect to page one page at a time.  Instead, it the page scrolls almost the entire content height instead.

Check out http://crosbug.com/2562 for further details.

[Ojan Vafai]

I don't see this on Chromium 5.0.391.0 (Developer Build 45722) on Mac. I don't see it in Google Docs and I don't see it on a test page that just has an iframe.

Can you include more detailed steps? What browsers have you tested in? What versions?

[Tony Chang]

A test case attached to the bug would be awesome.

[Zelidrag Hornung]

I've seen this in 5.0.375.17 dev on Windows and it is also detected in the current tip of the trunk which we pulled into ChromeOS.

Please take a look at the example in WK layout test at LayoutTests/editing/input/option-page-up-down.html

Currently, this layout test seems to be rigged to pass around this bug.

[Tony Chang]

You're right, it looks like we're moving the cursor to the wrong place (although we seem to be scrolling the right amount).  I'll investigate.

[Zelidrag Hornung]

Actually, we don't even scroll properly by page there (if the edit box is focused).

I have a fix for this being prepared for the upload.

[Zelidrag Hornung]

Created attachment 54490 [details]
Fix for scroll height calc for page up/down key handling in Editor

Fix for scroll height calc for page up/down key handling in Editor.

[WebKit Review Bot]

Attachment 54490 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 3 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zelidrag Hornung]

Created attachment 54491 [details]
Fix for scroll height calc for page up/down key handling in Editor

removed tabs from ChangeLogs

[Tony Chang]

Thanks for the patch!  A few comments:

- This doesn't link on Mac because WebCore::SelectionController::setSelection is in the list of exported symbols.  I think you need to update WebCore.base.exp.
- The change to setSelection is so you can scroll to iframe so the cursor is the first selected line.  In TextEdit on mac, I don't see that behavior.  In TextEdit, the frame scrolls so that the cursor is in the middle.  This is probably a platform specific behavior.
- The existing layout test doesn't really tell you how the view scrolled.  We probably need to add a layout test to verify that we scrolled the right amount.

[David Levin]

Comment on attachment 54491 [details]
Fix for scroll height calc for page up/down key handling in Editor

This looks like it is heading on a good path. I have some general comments and when they are done either I or someone else should verify the patch in greater detail.

Ideally, the ChangeLog should have per function comments to explain why the change was done in that function.

For example, I don't know why the code in verticalScrollDistance changed and the ChangeLog should explain this.
 
When you find yourself adding a bool and then passing in true/false for it, it is a bad sign. It is especially bad if it is with a bunch of other bools. Please change the bool to an enum that describes the desired behavior (something like alignToTop or alignToEdgeIfNeeded?).

Also, it looks like Tony had a bunch of good comments which should be addressed.

[Zelidrag Hornung]

Created attachment 54833 [details]
fixed mac link and behavior, other comments addressed

- fixes WebCore::SelectionController::setSelection definition in WebCore.base.exp
- made cursor align consistent with other apps on Mac
- replaced cursor align control param with enum (instead of bool) in SelectionController::moveTo/setSelection and related calls
- added better descriptions to ChangeLogs

[Zelidrag Hornung]

Created attachment 54834 [details]
fixed mac link and behavior, other comments addressed  

- fixes WebCore::SelectionController::setSelection definition in
WebCore.base.exp
- made cursor align consistent with other apps on Mac
- replaced cursor align control param with enum (instead of bool) in
SelectionController::moveTo/setSelection and related calls
- added better descriptions to ChangeLogs
- compared to previous patch, it has the correct scroll #if logic for Mac vs. the rest

[Zelidrag Hornung]

Created attachment 54838 [details]
fixed mac link issues and scroll behavior, other comments addressed  

- fixes WebCore::SelectionController::setSelection definition in
WebCore.base.exp
- made cursor align consistent with how other apps work on Mac
- replaced cursor align control param with enum (instead of bool) in
SelectionController::moveTo/setSelection and related calls
- added better descriptions to ChangeLogs
- compared to previous patches, this cl used #if OS(DARWIN) properly

[Tony Chang]

(In reply to comment #13)
> Created an attachment (id=54838) [details]

I think the code changes look good.  It would be great if you could add another test to verify the scroll behavior (center align vs top align).  Alternately, maybe you could make the existing test check this.  One way would be to use JS to read the scroll offset (maybe give it a range of pixels to be tolerant of font differences).

[Zelidrag Hornung]

Created attachment 54949 [details]
added additional test for testing visual viewport of scrolled frame

[Tony Chang]

This LGTM, but I am not a reviewer.

[David Levin]

Comment on attachment 54949 [details]
added additional test for testing visual viewport of scrolled frame

Unfortunately, this comment from last time was not addressed:
   Ideally, the ChangeLog should have per function comments to explain why the
   change was done in that function.

   For example, I don't know why the code in verticalScrollDistance changed and
   the ChangeLog should explain this.
(Actually, I think I get it now....but the ChangeLog should still explain it.)


Also if there were per function comments, it would hopefully tell me why a particular place became "align always" when before it was "align if needed".

Note that if you were to say the same thing more than once in a row then "Ditto." suffices the second time.


> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 58689)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,32 @@
> [phone]  Zelidrag Hornung  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Fixed frame page up/down scrolling calculation, made sure that the

It should be a . instead of a ,

You're doing three separate things in this patch.

A well focused patch that only does one thing would be smaller, quicker to review, get committed faster, and most importantly reviewed better because of the focused nature. It also would help folks in the future to not have to understand what each part of this patch is doing (but once again per function comments in the ChangeLog woudl also help with this).

It may take you slightly longer, but there are far more committers than reviewers, so it is nice to optimize for reviewer time.

You don't need to break this one up at this point but it is something to keep in mind. (I'd guess that we would have had two of these committed at this point had that course been taken.)

> +        cursor moves with page up/down as well. Please note that now OS(DARWIN)
> +        implementation will now center the center the cursor on page up/down

Is the cursor behavior tested anywhere? (And where is the mac vs other platforms behavior of the cursor tested?)

Is there a test to verify that the cursor isn't moved (on OS(DARWIN)) when alt isn't pressed?


> +        while other platforms will align the cursor with the top of displayed
> +        frame.
> +        https://bugs.webkit.org/show_bug.cgi?id=38213

> +        * editing/SelectionController.h:
> +        (WebCore::SelectionController::):

This should be a function name. prepare-ChangeLog messed this up but you should fix it ideally.


> Index: WebCore/editing/EditorCommand.cpp

Now that you have removed the one call to renderbox in this file, will it build with the "#include "RenderBox.h"" be removed?


> Index: WebCore/editing/SelectionController.h
> ===================================================================
> --- WebCore/editing/SelectionController.h	(revision 58684)
> +++ WebCore/editing/SelectionController.h	(working copy)
> @@ -45,6 +45,7 @@ class SelectionController : public Nonco
>  public:
>      enum EAlteration { MOVE, EXTEND };
>      enum EDirection { FORWARD, BACKWARD, RIGHT, LEFT };
> +    enum EScrollCursorAlignment { ALIGN_IF_NEEDED, ALIGN_ALWAYS };

The enum is oddly named with the "E" prefix.
The enums themselves (while consistent with the other enums here) are not ConsistentWithTheStyleGuide.
The enum names do not stand on their own:
   ALIGN_IF_NEEDED -- align what if needed? When I see it in the code, it looks like this SelectionController::ALIGN_IF_NEEDED. This makes one think that the selection or selection controller is being aligned with something.
 



>  
>      SelectionController(Frame* = 0, bool isDragCaretController = false);
>  
> @@ -54,14 +55,14 @@ public:
>      Node* shadowTreeRootNode() const { return m_selection.shadowTreeRootNode(); }
>       
>      void moveTo(const Range*, EAffinity, bool userTriggered = false);
> -    void moveTo(const VisiblePosition&, bool userTriggered = false);
> +    void moveTo(const VisiblePosition&, bool userTriggered = false, EScrollCursorAlignment cursorAlignment = ALIGN_IF_NEEDED);

There is no need for the param to be named here ("cursorAlignment") because it adds no new information.

> +    void setSelection(const VisibleSelection&, bool closeTyping = true, bool clearTypingStyle = true, bool userTriggered = false, EScrollCursorAlignment cursorAlignment = ALIGN_IF_NEEDED, TextGranularity = CharacterGranularity);

Ditto.

> +    bool modify(EAlteration, int verticalDistance, bool userTriggered = false, EScrollCursorAlignment cursorAlignment = ALIGN_IF_NEEDED);

Ditto.


> Index: LayoutTests/editing/input/scroll-viewport-page-up-down.html

> +function runTest() {
> +    var tolerance = 10;
> +    var frame = frames[0];
> +    var doc = frame.document;
> +    var body = doc.body;
> +    for (var i = 0; i < 10; ++i)
> +        body.innerHTML += "<p>line " + i + "</p>\n";
> +    frame.focus();
> +    frame.getSelection().setPosition(body.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(frame.pageYOffset - offsets[0]) > tolerance) {
> +        throw "Frame viewport should be around " + offsets[0] +
> +            "px , not at " + frame.pageYOffset;
> +    }
> +
> +    eventSender.keyDown("pageDown", modifiers);
> +    var line = frame.getSelection().baseNode.nodeValue;
> +    if (Math.abs(frame.pageYOffset - offsets[1]) > tolerance) {
> +        throw "Frame viewport should be around " + offsets[1] +
> +            "px , not " + frame.pageYOffset;
> +    }
> +
> +    eventSender.keyDown("pageUp", modifiers);
> +    var line = frame.getSelection().baseNode.nodeValue;
> +    if (Math.abs(frame.pageYOffset - offsets[0]) > tolerance) {
> +        throw "Frame viewport should be around " + offsets[0] +
> +            "px , not at " + frame.pageYOffset;
> +    }
> +
> +    document.getElementById("results").innerText = "PASS";
> +}
> +</script>
> +
> +<div>On Mac, option+pagedown should move the frame mouse cursor in the middle of the frame viewport in text areas.
> +On other platforms, pagedown should move the mouse cursor at the top of the viewport while scroll in text areas.

This talks about the cursor but the test doesn't test the cursor at all from what I can see.



> +This test requires DRT to pass.</div>

Instead of saying that it requires the DRT to pass, ideally it would tell someone how to manually repro the test if possible.

[Ojan Vafai]

Comment on attachment 54949 [details]
added additional test for testing visual viewport of scrolled frame

WebCore/editing/SelectionController.cpp:165
 +  #if OS(DARWIN)
This should use settings->editingBehavior() == EditingMacBehavior. That way it's a runtime setting and once https://bugs.webkit.org/show_bug.cgi?id=38603 is completed, we can change the setting on the fly in layout tests.

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

You don't have to, but if you want to make this test a bit more complete, you could also add verification that the selection is in the right place (e.g. it should be centered on mac except at the bounds of the scroll region).You can get the selection bounding box with window.getSelection().getBoundingClientRect().

[David Levin]

(In reply to comment #18)
> (From update of attachment 54949 [details])
> You don't have to, but if you want to make this test a bit more complete, you
> could also add verification that the selection is in the right place (e.g. it
> should be centered on mac except at the bounds of the scroll region).

Actually, this appears to be part of the changes being done, so it should be tested as I noted above (and it should be trivial to test).

[Zelidrag Hornung]

Created attachment 55433 [details]
Addressed the latest set of comments

Please note that test scroll-viewport-page-up-down.html covers platform differences in scroll view between mac and other platforms while option-page-up-down.html actually ensures that the cursor it on the same line on page up/down event regardless of the platform. 

* (re)added somehow dropped per-function comments into ChangeLog
* renames SelectionController::CursorAlignOnScroll enum and its values to give more details about what they do
* removed #include "RenderBox.h" from WebCore/editing/EditorCommand.cpp
* renames cursorAlignment param
* changed #if OS(DARWIN) to settings->editingBehavior() == EditingMacBehavior in SelectionController.cpp

[Ojan Vafai]

Comment on attachment 55433 [details]
Addressed the latest set of comments

WebCore/editing/SelectionController.cpp:168
 +          if (m_frame->settings()->editingBehavior() == EditingMacBehavior)
You should use the settings pointer you got in line 165. Also, you need to nullcheck settings here.

WebCore/editing/EditorCommand.cpp:262
 +      int height = frame->view()->visibleHeight();
This doesn't look right to me. We're scrolling a renderer here that has overflow (i.e. the scrollbars are on that box, not the frame). Can you maybe add a case to your test (or another test) where you test scrolling in a div with overflow:auto?

WebCore/editing/EditorCommand.cpp:260
 +      if (!(style->overflowY() == OSCROLL || style->overflowY() == OAUTO || focusedNode->isContentEditable()))
This is only tangentially related to your code, so feel free to ignore, but this if-statement doesn't look right to me. If we're in a contentEditable element that's overflow:visible, I think we probably want to early return here.

[David Levin]

Comment on attachment 55433 [details]
Addressed the latest set of comments

I think you did some changes that didn't make it in this patch. Sorry if I sound like I'm repeating myself in these comments. I may have missed something so let me know if somehow I thought you should do something and you already did.

Thanks!

> Index: WebCore/ChangeLog
> [phone]  Zelidrag Hornung  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Fixed frame page up/down scrolling calculation. Made sure that the
> +        cursor moves with page up/down as well. Please note that now OS(DARWIN)

OS(DARWIN) doesn't match the code anymore.

> +        implementation will now center the center the cursor on page up/down
> +        while other platforms will align the cursor with the top of displayed
> +        frame.
> +        https://bugs.webkit.org/show_bug.cgi?id=38213
> +
> +        Test: editing/input/option-page-up-down.html (fixed)
> +        Test: editing/input/scroll-viewport-page-up-down.html (added)
> +
> +        * WebCore.base.exp:
> +        * editing/EditorCommand.cpp:
> +        (WebCore::verticalScrollDistance): Fixed page scroll calculation. Now scroll height is calculated only from the visible portion not the entire frame height.
> +        (WebCore::executeMovePageDown): Now it can tell SelectionController to move the cursor with the page scroll up/down events.

> Index: WebCore/editing/SelectionController.cpp

> +    if (userTriggered) {
> +        Settings* settings = m_frame ? m_frame->settings() : 0;

settings is unused.

> +        ScrollAlignment alignment;
> +        if (m_frame->settings()->editingBehavior() == EditingMacBehavior)
> +            alignment = (align == ALIGN_CURSOR_ON_SCROLL_ALWAYS) ? ScrollAlignment::alignCenterAlways : ScrollAlignment::alignCenterIfNeeded;
> +        else
> +            alignment = (align == ALIGN_CURSOR_ON_SCROLL_ALWAYS) ? ScrollAlignment::alignTopAlways : ScrollAlignment::alignToEdgeIfNeeded;
> +
> +        m_frame->revealSelection(alignment, true);

If m_frame may be 0, then this will crash. (You check for m_frame being 0 above.) I don't see how m_frame can be 0, since this is checked for on line 121 of "SelectionController::setSelection" and the code returns there. Also this code always has used m_frame without doing a 0 check before.


> Index: WebCore/editing/SelectionController.h
> +    enum CursorAlignOnScroll { ALIGN_CURSOR_ON_SCROLL_IF_NEEDED,
> +                               ALIGN_CURSOR_ON_SCROLL_ALWAYS };

(I tried to mention this before but I must not have been clear... sorry.)
 Enum members should user InterCaps with an initial capital letter. In other words, it should look like this:

    enum CursorAlignOnScroll { AlignCursorOnScrollAlwaysIfNeeded, AlignCursorOnScrollAlways };

(You may wrap the line if you wish but you don't have to.)
> +    void moveTo(const VisiblePosition&, bool userTriggered = false, CursorAlignOnScroll align = ALIGN_CURSOR_ON_SCROLL_IF_NEEDED);

(I mentioned this before but I must not have made it clear what I meant.) 
Please don't include the param name "align" since it add no information. In other words, the line should look like this:
   void moveTo(const VisiblePosition&, bool userTriggered = false, CursorAlignOnScroll = ALIGN_CURSOR_ON_SCROLL_IF_NEEDED);

> +    void setSelection(const VisibleSelection&, bool closeTyping = true, bool clearTypingStyle = true, bool userTriggered = false, CursorAlignOnScroll align = ALIGN_CURSOR_ON_SCROLL_IF_NEEDED, TextGranularity = CharacterGranularity);

Ditto.

> +    bool modify(EAlteration, int verticalDistance, bool userTriggered = false, CursorAlignOnScroll align = ALIGN_CURSOR_ON_SCROLL_IF_NEEDED);

Ditto.

> Index: LayoutTests/ChangeLog
> [phone]  Zelidrag Hornung  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Fixed page up/down scrolling expectations for bug:
> +        https://bugs.webkit.org/show_bug.cgi?id=38213
> +        Added a new test that verifies how visible portion of frame changes
> +        while scrolling the frame. The test will check for different value on
> +        OS(DARWIN) than other platforms since edit frame scroll there will

OS(DARWIN) doesn't match the code anymore.

> +        center the cursor on page up/down while it will align it with the top
> +        on other platforms.
> +        
> +        * editing/input/option-page-up-down.html: Fixed rigged test that was masking scrolling and cursor move errors. Added tests to make sure the cursor does not move on Mac if [option] is not pressed.

Thx! But I don't see that change in this patch?


> Index: LayoutTests/editing/input/scroll-viewport-page-up-down.html
> +function runTest() {
> +    var tolerance = 10;
> +    var frame = frames[0];
> +    var doc = frame.document;
> +    var body = doc.body;
> +    for (var i = 0; i < 10; ++i)
> +        body.innerHTML += "<p>line " + i + "</p>\n";
> +    frame.focus();
> +    frame.getSelection().setPosition(body.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(frame.pageYOffset - offsets[0]) > tolerance) {
> +        throw "Frame viewport should be around " + offsets[0] +
> +            "px , not at " + frame.pageYOffset;
> +    }
> +
> +    eventSender.keyDown("pageDown", modifiers);
> +    var line = frame.getSelection().baseNode.nodeValue;
> +    if (Math.abs(frame.pageYOffset - offsets[1]) > tolerance) {
> +        throw "Frame viewport should be around " + offsets[1] +
> +            "px , not " + frame.pageYOffset;
> +    }
> +
> +    eventSender.keyDown("pageUp", modifiers);
> +    var line = frame.getSelection().baseNode.nodeValue;
> +    if (Math.abs(frame.pageYOffset - offsets[0]) > tolerance) {
> +        throw "Frame viewport should be around " + offsets[0] +
> +            "px , not at " + frame.pageYOffset;
> +    }
> +
> +    document.getElementById("results").innerText = "PASS";
> +}
> +</script>


I don't see how this "ensures that the cursor [is] on the same line
on page up/down event regardless of the platform. "

>
> +<div>On Mac, option+pagedown should move the frame mouse cursor in the middle of the frame viewport in text areas.
> +On other platforms, pagedown should move the mouse cursor at the top of the viewport while scroll in text areas.

As noted previously, I don't understand why this talks about the cursor moving when it doesn't seem to verify that.

> +This test requires DRT to pass.</div>

(As noted previously) Instead of saying that it requires the DRT to pass, ideally it would tell
someone how to manually repro the test if possible.

[Tony Chang]

(In reply to comment #21)
> (From update of attachment 55433 [details])
> WebCore/editing/EditorCommand.cpp:260
>  +      if (!(style->overflowY() == OSCROLL || style->overflowY() == OAUTO || focusedNode->isContentEditable()))
> This is only tangentially related to your code, so feel free to ignore, but this if-statement doesn't look right to me. If we're in a contentEditable element that's overflow:visible, I think we probably want to early return here.

This is correct.  I added it to fix the bug where we weren't moving the cursor in contentEditable.  The code reads: if the node is not contentEditable, then don't move the cursor (i.e., move the cursor 0 lines).

[Tony Chang]

(In reply to comment #23)
> (In reply to comment #21)
> > (From update of attachment 55433 [details] [details])
> > WebCore/editing/EditorCommand.cpp:260
> >  +      if (!(style->overflowY() == OSCROLL || style->overflowY() == OAUTO || focusedNode->isContentEditable()))
> > This is only tangentially related to your code, so feel free to ignore, but this if-statement doesn't look right to me. If we're in a contentEditable element that's overflow:visible, I think we probably want to early return here.
> 
> This is correct.  I added it to fix the bug where we weren't moving the cursor in contentEditable.  The code reads: if the node is not contentEditable, then don't move the cursor (i.e., move the cursor 0 lines).

I'm sorry, my last comment was confusing.  You're asking about contentEditable+overflow:visible.  In that case, we should not return early because we do want to move the cursor (I think).

[Zelidrag Hornung]

Created attachment 56100 [details]
Review comment fixed, handling of non-iframe contenteditable cases

- more tests added to cover non-iframe contenteditable scroll/cursor handling
- addressed other review comments

[David Levin]

Comment on attachment 56100 [details]
Review comment fixed, handling of non-iframe contenteditable cases

Thanks for your efforts and patiently addressing all of the feedback!

[Ojan Vafai]

Comment on attachment 56100 [details]
Review comment fixed, handling of non-iframe contenteditable cases

I have a few things I'd like to see fixed. Putting my thoughts together now. I'll r+/r- appropriately with my comments.

[David Levin]

Comment on attachment 56100 [details]
Review comment fixed, handling of non-iframe contenteditable cases

Switching back to r? for Ojan comments.

[Ojan Vafai]

Comment on attachment 56100 [details]
Review comment fixed, handling of non-iframe contenteditable cases

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.

[Zelidrag Hornung]

Created attachment 56740 [details]
latest comments addressed, merged cursor/scroll tests, ahem fonts...

* added check for m_frame->settings()
* merged iframe and div tests into the same files
* ahem-ized fonts used across these tests

[WebKit Commit Bot]

Comment on attachment 56740 [details]
latest comments addressed, merged cursor/scroll tests, ahem fonts...

Rejecting patch 56740 from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  Due to bug 30084 the commit-queue will require a restart after your change.  Please contact [e-mail] to request a commit-queue restart.  After restart the commit-queue will correctly respect your reviewer rights.

[Zelidrag Hornung]

Comment on attachment 56740 [details]
latest comments addressed, merged cursor/scroll tests, ahem fonts...

wrong flags

[Ojan Vafai]

Comment on attachment 56740 [details]
latest comments addressed, merged cursor/scroll tests, ahem fonts...

r=me with the change below. Thanks for being patient with all this.

WebCore/editing/SelectionController.cpp:174
 +          if (macStyleScroll)
If settings is null, that means we're in a state where it doesn't matter what editing behavior we use (e.g. a page transition). The null check is just to avoid crashing. So you can get rid of the macStyleScroll variable and just do something like this:
if (m_frame->settings() && m_frame->settings()->editingBehavior() == EditingMacBehavior)

[Zelidrag Hornung]

Created attachment 56750 [details]
Changes m_frame->settings() check

changed m_frame->settings() check per latest comment.

[WebKit Commit Bot]

Comment on attachment 56750 [details]
Changes m_frame->settings() check

Rejecting patch 56750 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Last 500 characters of output:
tree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found
Testing 18294 test cases.
editing/input/option-page-up-down.html -> failed

Exiting early after 1 failures. 3778 tests run.
53.45s total testing time

3777 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/2362077

[Tony Chang]

I ran the tests on my mac.  The two new tests both failed because they need an extra blank line in the end of the results.

However, platform/mac/editing/selection/25228.html also fails, but as mentioned in the test, this is probably expected.  The test expects us to scroll such that the cursor is barely exposed, but this change causes the cursor to be centered.  This seems to match TextEdit behavior.

[Tony Chang]

Created attachment 56847 [details]
Patch

[Tony Chang]

I've uploaded a diff with the updated test expectations.  While the new results for platform/mac/editing/selection/25228.html are correct, but it makes the test no longer test what it's supposed to test.  I tried to come up with a new test that tests r42583, but I'm having a hard time.

[Ojan Vafai]

Comment on attachment 56847 [details]
Patch

I find the horizontal centering to be a bit weird, but TextEdit does it and it's better than the old behavior of aligning to the left. 

I would r+ except for platform/mac/editing/selection/25228.html. I feel like we should either fix it so it's actually testing what it claims to or delete the test. The only thing I can think of for testing this would be to do the following:
1. Use the "ahem" font-family so that character widths are fixed. I think they're all 15px.
2. Make the div wide (e.g. 1000px)
3. Put the cursor in the middle of the first line.
4. Move the cursor backward one character.
5. Assert that there is a specific scrollLeft value on the body.

If http://trac.webkit.org/changeset/42583 ever regressed, the scrollLeft would change. It's harder than the old test to verify correctness visually, but at least should keep testing that code path and we'll know if it ever changes.

[Tony Chang]

Created attachment 57081 [details]
Patch

[Tony Chang]

I've updated the test such that rolling out r42583 will cause platform/mac/editing/selection/25228.html to fail.

[Ojan Vafai]

Comment on attachment 57081 [details]
Patch

Thanks for following up on this.

[Tony Chang]

Committed r60268: <http://trac.webkit.org/changeset/60268>

[WebKit Review Bot]

http://trac.webkit.org/changeset/60268 might have broken GTK Linux 32-bit Release