Bug 50012 - Moving cursor down in table cycles at the end of a row
Summary: Moving cursor down in table cycles at the end of a row
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: HasReduction
Depends on:
Blocks:
 
Reported: 2010-11-24 04:05 PST by jochen
Modified: 2011-02-03 11:33 PST (History)
7 users (show)

See Also:


Attachments
layout test demonstrating the problem (1.02 KB, text/html)
2010-11-24 04:14 PST, jochen
no flags Details
Patch (3.74 KB, patch)
2011-02-02 15:35 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (3.92 KB, patch)
2011-02-02 16:51 PST, Levi Weintraub
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jochen 2010-11-24 04:05:43 PST
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.
Comment 1 jochen 2010-11-24 04:14:52 PST
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)
Comment 2 jochen 2010-11-24 04:23:49 PST
Adding a few interested parties (according to svn blame on the files I suspect to be affected). Apologies if I'm mistaken
Comment 3 jochen 2010-11-24 04:30:17 PST
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
Comment 4 Ryosuke Niwa 2011-02-01 13:54:37 PST
(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?
Comment 5 Levi Weintraub 2011-02-01 14:02:36 PST
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.
Comment 6 jochen 2011-02-01 14:04:39 PST
(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.
Comment 7 Ryosuke Niwa 2011-02-01 14:15:43 PST
This is a bug in nextLinePosition in visible_units.cpp.  It's a hard bug to fix though.
Comment 8 Levi Weintraub 2011-02-02 15:35:23 PST
Created attachment 80981 [details]
Patch
Comment 9 Levi Weintraub 2011-02-02 15:41:28 PST
(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.
Comment 10 Ryosuke Niwa 2011-02-02 15:53:12 PST
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.
Comment 11 Levi Weintraub 2011-02-02 15:57:29 PST
> 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.
Comment 12 Ryosuke Niwa 2011-02-02 16:04:58 PST
(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.
Comment 13 Levi Weintraub 2011-02-02 16:51:48 PST
Created attachment 81005 [details]
Patch
Comment 14 Ryosuke Niwa 2011-02-02 16:56:57 PST
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.
Comment 15 Levi Weintraub 2011-02-03 11:33:28 PST
Committed r77521: <http://trac.webkit.org/changeset/77521>