[Webkit-unassigned] [Bug 50012] Moving cursor down in table cycles at the end of a row

Wed Feb 2 15:53:12 PST 2011

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

Ryosuke Niwa <rniwa at webkit.org> changed:

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

--- Comment #10 from Ryosuke Niwa <rniwa at webkit.org>  2011-02-02 15:53:12 PST ---
(From update of attachment 80981)
View in context: https://bugs.webkit.org/attachment.cgi?id=80981&action=review

Ah great!  I had imagined this would have been much harder to fix.  Good job fixing it.  r- for stylistic issues.

> Source/WebCore/ChangeLog:7
> +

Please explain what caused the bug and how you fixed it. You can almost copy & paste your comment #c9.

> Source/WebCore/editing/visible_units.cpp:589
>      while (n) {
>          if (editable == n->isContentEditable())
>              return n;

It's sad that this while loop is identical to that of the function below yet we don't share code.  Is there anyway we can share code?

> LayoutTests/ChangeLog:7
> +

Please explain what kind of test you're adding, etc...

> LayoutTests/ChangeLog:9
> +        * editing/selection/move-by-line-006-expected.txt: Added.
> +        * editing/selection/move-by-line-006.html: Added.

It would be nice if the test name reflected what the test is about rather than just having a number 006.

> LayoutTests/editing/selection/move-by-line-006.html:1
> +<html>

Missing <!DOCTYPE html>

> LayoutTests/editing/selection/move-by-line-006.html:17
> +    <script src=../editing.js language="JavaScript" type="text/JavaScript" ></script>
> +    <script>
> +        function test()
> +        {
> +            if (window.layoutTestController)
> +                layoutTestController.dumpAsText();
> +
> +            var target = document.getElementById("target");
> +            getSelection().setBaseAndExtent(target.firstChild, 0, target.firstChild, 0);
> +            for (var i=0; i<2; i++)
> +                moveSelectionForwardByLineCommand();
> +
> +            document.getElementById("result").innerText = getSelection().baseOffset == 6 ? "PASS" : "FAIL";
> +        }
> +    </script>

We don't normally indent tags like this.  And I don't think we need to wait until load event.  You can just run all of them when the parser inserts the script element into the document if you moved the script element after table.  If you do that, then you can just use document.write to print PASS/FAIL instead of having a #result paragraph.

Also, no space between i and 2, and no need to specify type and language in the script tag.

> LayoutTests/editing/selection/move-by-line-006.html:21
> +    <p>
> +        Test for <i><a href="http://bugs.webkit.org/show_bug.cgi?id=50012">http://bugs.webkit.org/show_bug.cgi?id=50012</a>

Ditto about indentation.

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