Bug 31920 - REGRESSION (r38898): Using the up arrow in a textarea gets "stuck" at the beginning
: REGRESSION (r38898): Using the up arrow in a textarea gets "stuck" at the beg...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML Editing
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks: 25676
  Show dependency treegraph
 
Reported: 2009-11-26 11:36 PST by Alejandro G. Castro
Modified: 2009-12-18 01:50 PST (History)
8 users (show)

See Also:


Attachments
Patch fixing the problem (524 bytes, patch)
2009-11-26 11:38 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
New proposed patch (1.30 KB, patch)
2009-12-15 04:09 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Proposed patch (4.25 KB, patch)
2009-12-16 05:50 PST, Alejandro G. Castro
darin: review-
Details | Formatted Diff | Diff
Proposed fix patch (3.54 KB, patch)
2009-12-16 10:46 PST, Alejandro G. Castro
darin: review+
Details | Formatted Diff | Diff
Proposed fix (3.52 KB, patch)
2009-12-16 10:49 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alejandro G. Castro 2009-11-26 11:36:24 PST
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.
Comment 1 Alejandro G. Castro 2009-11-26 11:38:25 PST
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.
Comment 2 Alejandro G. Castro 2009-12-02 00:12:57 PST
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.
Comment 3 Alejandro G. Castro 2009-12-15 04:09:54 PST
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.
Comment 4 Alejandro G. Castro 2009-12-16 05:50:39 PST
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.
Comment 5 WebKit Review Bot 2009-12-16 05:55:58 PST
style-queue ran check-webkit-style on attachment 44965 [details] without any errors.
Comment 6 Alexey Proskuryakov 2009-12-16 08:50:03 PST
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.
Comment 7 Darin Adler 2009-12-16 09:28:12 PST
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.
Comment 8 Darin Adler 2009-12-16 09:29:16 PST
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.
Comment 9 Alejandro G. Castro 2009-12-16 09:52:09 PST
(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?
Comment 10 Alejandro G. Castro 2009-12-16 09:53:16 PST
(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
Comment 11 Alexey Proskuryakov 2009-12-16 10:07:13 PST
> 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.
Comment 12 Alejandro G. Castro 2009-12-16 10:46:37 PST
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.
Comment 13 Darin Adler 2009-12-16 10:48:54 PST
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
Comment 14 Alejandro G. Castro 2009-12-16 10:49:23 PST
Created attachment 44993 [details]
Proposed fix

Small fix in the paths of the Changelog.
Comment 15 Alejandro G. Castro 2009-12-18 01:36:25 PST
There is another patch that fixes another regression caused by the same patch in the bug 25676, could someone check it?
Comment 16 Xan Lopez 2009-12-18 01:38:33 PST
Comment on attachment 44993 [details]
Proposed fix

Set cq+
Comment 17 WebKit Commit Bot 2009-12-18 01:50:35 PST
Comment on attachment 44993 [details]
Proposed fix

Clearing flags on attachment: 44993

Committed r52303: <http://trac.webkit.org/changeset/52303>
Comment 18 WebKit Commit Bot 2009-12-18 01:50:41 PST
All reviewed patches have been landed.  Closing bug.