[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


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #54949|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #17 from David Levin <levin at chromium.org>  2010-05-06 10:23:38 PST ---
(From update of attachment 54949)
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 @@
> +2010-05-03  Zelidrag Hornung  <zelidrag at chromium.org>
> +
> +        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.

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