Bug 31920

Summary: REGRESSION (r38898): Using the up arrow in a textarea gets "stuck" at the beginning
Product: WebKit Reporter: Alejandro G. Castro <alex>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, bdakin, commit-queue, ddkilzer, eric, jdiggs, justin.garcia, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 25676    
Attachments:
Description Flags
Patch fixing the problem
none
New proposed patch
none
Proposed patch
darin: review-
Proposed fix patch
darin: review+
Proposed fix none

Alejandro G. Castro
Reported 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.
Attachments
Patch fixing the problem (524 bytes, patch)
2009-11-26 11:38 PST, Alejandro G. Castro 		no flags
DetailsFormatted DiffDiff
New proposed patch (1.30 KB, patch)
2009-12-15 04:09 PST, Alejandro G. Castro 		no flags
DetailsFormatted DiffDiff
Proposed patch (4.25 KB, patch)
2009-12-16 05:50 PST, Alejandro G. Castro 		darin: review-
DetailsFormatted DiffDiff
Proposed fix patch (3.54 KB, patch)
2009-12-16 10:46 PST, Alejandro G. Castro 		darin: review+
DetailsFormatted DiffDiff
Proposed fix (3.52 KB, patch)
2009-12-16 10:49 PST, Alejandro G. Castro 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Alejandro G. Castro
Comment 1 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.
Alejandro G. Castro
Comment 2 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.
Alejandro G. Castro
Comment 3 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.
Alejandro G. Castro
Comment 4 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.
WebKit Review Bot
Comment 5 2009-12-16 05:55:58 PST
style-queue ran check-webkit-style on attachment 44965 [details] without any errors.
Alexey Proskuryakov
Comment 6 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.
Darin Adler
Comment 7 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.
Darin Adler
Comment 8 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.
Alejandro G. Castro
Comment 9 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?
Alejandro G. Castro
Comment 10 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
Alexey Proskuryakov
Comment 11 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.
Alejandro G. Castro
Comment 12 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.
Darin Adler
Comment 13 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
Alejandro G. Castro
Comment 14 2009-12-16 10:49:23 PST
Created attachment 44993 [details] Proposed fix Small fix in the paths of the Changelog.
Alejandro G. Castro
Comment 15 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?
Xan Lopez
Comment 16 2009-12-18 01:38:33 PST
Comment on attachment 44993 [details] Proposed fix Set cq+
WebKit Commit Bot
Comment 17 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>
WebKit Commit Bot
Comment 18 2009-12-18 01:50:41 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.