Bug 50012 – Moving cursor down in table cycles at the end of a row

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

When you try to move with the cursor through an editable table, the cursor jumps to the beginning at the end of a row. See attached layout test.

Created attachment 74745 [details]
layout test demonstrating the problem

If you try to go with e.g. page down over a part of html that contains a table, webkit will go into an endless loop in SelectionController::modify (in the for loop around line 777)

Adding a few interested parties (according to svn blame on the files I suspect to be affected). Apologies if I'm mistaken

note that the test doesn't work outside of LayoutTests/editing/selection/

However, you can just place the cursor on the beginning of the last line and repeatedly hit cursor down to see the problem

[Ryosuke Niwa]

(In reply to comment #1)
> Created an attachment (id=74745) [details]
> layout test demonstrating the problem
> 
> If you try to go with e.g. page down over a part of html that contains a table, webkit will go into an endless loop in SelectionController::modify (in the for loop around line 777)

I don't understand what you mean by the endless loop.  Do you mean that pressing down key will keep moving cursor between two positions?  Or that WebKit falls into an infinite loop and does not respond?

[Levi Weintraub]

It actually returns to whichever position you start it in on that line. Placing the caret in the middle will jump between the middle and end.

I was just taking a look at this code for 25757 so I'm happy to find the root cause.

(In reply to comment #4)
> (In reply to comment #1)
> > Created an attachment (id=74745) [details] [details]
> > layout test demonstrating the problem
> > 
> > If you try to go with e.g. page down over a part of html that contains a table, webkit will go into an endless loop in SelectionController::modify (in the for loop around line 777)
> 
> I don't understand what you mean by the endless loop.  Do you mean that pressing down key will keep moving cursor between two positions?  Or that WebKit falls into an infinite loop and does not respond?

I discovered the issue while using page-down to scroll through an reply to an html mail in gmail. In that case, WebKit falls into an infinite loop and does not respond.

After adding some debug output, I figured out that the current editing position was inside a table and WebKit got stuck in SelectionController::modify (the for loop in line 845-860.

The layout test demonstrates the underlying issue. When you press down at the end of the table, you should stay at the end. Instead, you jump again to the beginning of the table. That's what's causing the endless loop IMO.

[Ryosuke Niwa]

This is a bug in nextLinePosition in visible_units.cpp.  It's a hard bug to fix though.

[Levi Weintraub]

Created attachment 80981 [details]
Patch

[Levi Weintraub]

(In reply to comment #8)
> Created an attachment (id=80981) [details]
> Patch

The code improperly descended back into the same leaf node because the anchor node the last position used wasn't in a leaf position.

In nextLeafWithSameEditability, when there's no child node at the provided offset, it should traverse to the last descendant before finding the next leaf node, or it can regress like in this example.

Also, I had to update the offset in your layout test to correspond with the actual final offset.

[Ryosuke Niwa]

Comment on attachment 80981 [details]
Patch

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.

[Levi Weintraub]

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

I put the short description after the function name but I can throw in more if you'd like ;)

+        * editing/visible_units.cpp:
+        (WebCore::nextLeafWithSameEditability):
+        Properly avoid descending back into the original leaf node.

I'll clean up the layout test. I simply grabbed it from the bug.

[Ryosuke Niwa]

(In reply to comment #11)
> I put the short description after the function name but I can throw in more if you'd like ;)
> 
> +        * editing/visible_units.cpp:
> +        (WebCore::nextLeafWithSameEditability):
> +        Properly avoid descending back into the original leaf node.

I think we normally start the comment immediately after : and a space as in:

        (WebCore::nextLeafWithSameEditability): Properly avoid descending back into
        the original leaf node.

[Levi Weintraub]

Created attachment 81005 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 81005 [details]
Patch

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.

[Levi Weintraub]

Committed r77521: <http://trac.webkit.org/changeset/77521>