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

Wed Feb 2 16:56:57 PST 2011

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

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #81005|review?                     |review+
               Flag|                            |

--- Comment #14 from Ryosuke Niwa <rniwa at webkit.org>  2011-02-02 16:56:57 PST ---
(From update of attachment 81005)
View in context: https://bugs.webkit.org/attachment.cgi?id=81005&action=review

I'd say r+ but please address the following comments before landing this patch.

> Source/WebCore/ChangeLog:11
> +        Avoids a caret cycling issue with certain content (e.g. tables) found at the very
> +        end of a document due to a bug in nextLeafWithSameEditability.

Description should appear before tests.

> LayoutTests/editing/selection/move-by-line-cycles-in-table.html:1
> +<html>

Please put <!DOCTYPE html>

> LayoutTests/editing/selection/move-by-line-cycles-in-table.html:3
> +  <script src=../editing.js language="JavaScript" type="text/JavaScript" ></script>

Please remove the indentation, and language/type attributes.  They just clutter the test.

Also, you can move this into body right before another script element.  That way, you can get rid of head.

> LayoutTests/editing/selection/move-by-line-cycles-in-table.html:12
> +<script language="JavaScript" type="text/JavaScript">

Ditto about language and type attributes.

> LayoutTests/editing/selection/move-by-line-cycles-in-table.html:14
> + layoutTestController.dumpAsText();

We should probably give two or four spaces instead of one to be consistent with the rest of tests.

> LayoutTests/editing/selection/move-by-line-cycles-in-table.html:19
> + moveSelectionForwardByLineCommand();

Ditto about the spaces.

> LayoutTests/editing/selection/move-by-line-cycles-in-table.html:21
> +document.write(getSelection().baseOffset == 4 ? "PASS" : "FAIL");

It'll be nice if you printed getSelection().baseOffset in the case of FAIL so that it's easier to diagnose the problem if the test failed on bots.

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