Bug 31920 – REGRESSION (r38898): Using the up arrow in a textarea gets "stuck" at the beginning

• Component: HTML Editing
• Status: RESOLVED FIXED

[Alejandro G. Castro]

The patch uploaded to fix the bug 13736 breaks the up arrow action. In order to reproduce the problem you can:

1. Open the test LayoutTests/editing/selection/wrapped-line-caret-2.html
2. Move the cursor to a position in the second line after the character 3, in the middle of the xs
3. Use up arrow to move to the first line

The expected result: the up arrow should move the cursor to the end of the first line, after the space.

The actual result: the cursor moves to the beginning of the second line and if you press up again the cursor does not move.

The regression is caused after the patch committed to solve that bug if we comment the call to the searchAheadForBetterMatch we can move to the end of the line with the up arrow.

I've checked it can be solved modifying the affinity.

[Alejandro G. Castro]

Created attachment 43928 [details]
Patch fixing the problem

I have attached a patch that fixes the problem setting the affinity in that case to UPSTREAM. Not sure if it is a good option because the patch breaks some tests, but just because of the log message, the problem is the dump text, it prints a message about the affinity and makes some tests to fail because the expected did not include that message.

In case the patch is correct we would have to include the modifications of the expected values of the tests.

[Alejandro G. Castro]

I forgot to mention that this is also causing a regression in the caret browsing, bug 25676, which btw is not fixed with the modification I've uploaded and fixes this problem, I would try to check the rationale of the loops of the original patch that is causing the problems.

[Alejandro G. Castro]

Created attachment 44862 [details]
New proposed patch

This patch fixes both regressions and does not break any of the editing tests. I'm going to create two patches with test to show the problems because we need reference about the change in that logic. I also have to create a new test function to set the caret browsing from tests.

[Alejandro G. Castro]

Created attachment 44965 [details]
Proposed patch

This patch solves this regression, I've added a test to check it. I'll upload the patch fixing the other regression to the bug 25676.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44965 [details] without any errors.

[Alexey Proskuryakov]

When adding a pixel test, please include pixel results in the patch. But the included test can (and should) be text-only:

if (window.layoutTestController)
    layoutTestController.dumpAsText();

> Not sure if it is a good option because the patch breaks some
> tests, but just because of the log message

Please include changed test results in the patch - it's hard to tell if it's an OK change without seeing it.

[Darin Adler]

Comment on attachment 44965 [details]
Proposed patch

r=me

I don't se how this test case tests the bug. Does the test fail without the patch? While I think the test works as a manual test, I don't think it's effective as an automated test.

[Darin Adler]

Comment on attachment 44965 [details]
Proposed patch

Changing to review- because of the incomplete test in the patch.

This patch includes test results only for the gtk platform and does not include pixel test results.

[Alejandro G. Castro]

(In reply to comment #8)
> (From update of attachment 44965 [details])
> Changing to review- because of the incomplete test in the patch.
> 
> This patch includes test results only for the gtk platform and does not include
> pixel test results.

The test fails without the patch because the caret position does not reach the one specified in the expected result, if you check it manually it means the caret stays in the first position of the first line.

The test uses exactly the same technique as the editing/selection/wrapped-line-caret-2.html. Maybe I can try to avoid it using getSelection().baseOffset and getting the position of the cursor, that way I could use dumpAsText, would that be ok?

[Alejandro G. Castro]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 44965 [details] [details])
> > Changing to review- because of the incomplete test in the patch.
> > 
> > This patch includes test results only for the gtk platform and does not include
> > pixel test results.
> 
> The test fails without the patch because the caret position does not reach the
> one specified in the expected result, if you check it manually it means the
> caret stays in the first position of the first line.
                                            ^^^ I meant, second line

[Alexey Proskuryakov]

> Maybe I can try to avoid it using
> getSelection().baseOffset and getting the position of the cursor, that way I
> could use dumpAsText, would that be ok?

I was going to say yes, but looking at bug 13736  comment 19, I'm no longer sure. If you can make a text-only test case that fails before your fix and passes with it, it would definitely be much better than a pixel test.

[Alejandro G. Castro]

Created attachment 44992 [details]
Proposed fix patch

This one uses dumpAsText and checks the position of the caret. I fails before the patch and passes after it.

[Darin Adler]

Comment on attachment 44992 [details]
Proposed fix patch

> +        * platform/gtk/editing/selection/wrapped-line-caret-3-expected.txt:

This is the wrong path for the added file now.

r=me -- the minor mistake in ChangeLog is no big deal

[Alejandro G. Castro]

Created attachment 44993 [details]
Proposed fix

Small fix in the paths of the Changelog.

[Alejandro G. Castro]

There is another patch that fixes another regression caused by the same patch in the bug 25676, could someone check it?

[Xan Lopez]

Comment on attachment 44993 [details]
Proposed fix

Set cq+

[WebKit Commit Bot]

Comment on attachment 44993 [details]
Proposed fix

Clearing flags on attachment: 44993

Committed r52303: <http://trac.webkit.org/changeset/52303>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.