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
: WebKit
HTML Editing
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To:
:
:
:
: 25676
  Show dependency treegraph
 
Reported: 2009-11-26 11:36 PST by
Modified: 2009-12-18 01:50 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2009-11-26 11:38:25 PST -------
Created an attachment (id=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 From 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 From 2009-12-15 04:09:54 PST -------
Created an attachment (id=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 From 2009-12-16 05:50:39 PST -------
Created an attachment (id=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 From 2009-12-16 05:55:58 PST -------
style-queue ran check-webkit-style on attachment 44965 [details] without any errors.
------- Comment #6 From 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 From 2009-12-16 09:28:12 PST -------
(From update of attachment 44965 [details])
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 From 2009-12-16 09:29:16 PST -------
(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.
------- Comment #9 From 2009-12-16 09:52:09 PST -------
(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.

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 From 2009-12-16 09:53:16 PST -------
(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 44965 [details] [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 From 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 From 2009-12-16 10:46:37 PST -------
Created an attachment (id=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 From 2009-12-16 10:48:54 PST -------
(From update of attachment 44992 [details])
> +        * 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 From 2009-12-16 10:49:23 PST -------
Created an attachment (id=44993) [details]
Proposed fix

Small fix in the paths of the Changelog.
------- Comment #15 From 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 From 2009-12-18 01:38:33 PST -------
(From update of attachment 44993 [details])
Set cq+
------- Comment #17 From 2009-12-18 01:50:35 PST -------
(From update of attachment 44993 [details])
Clearing flags on attachment: 44993

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