Bug 50610 - REGRESSION(r55990): Shift-End does not select to the end of the line
Summary: REGRESSION(r55990): Shift-End does not select to the end of the line
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.5
: P1 Normal
Assignee: Benjamin (Ben) Kalman
URL:
Keywords:
Depends on: 49873 50944
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-06 18:39 PST by Mark Tsui
Modified: 2010-12-21 20:26 PST (History)
11 users (show)

See Also:


Attachments
test case (287 bytes, text/html)
2010-12-06 19:30 PST, Mark Tsui
no flags Details
Patch (1.40 KB, patch)
2010-12-12 20:13 PST, Benjamin (Ben) Kalman
no flags Details | Formatted Diff | Diff
Patch (2.63 KB, patch)
2010-12-12 20:19 PST, Benjamin (Ben) Kalman
no flags Details | Formatted Diff | Diff
Patch (4.95 KB, patch)
2010-12-12 21:21 PST, Benjamin (Ben) Kalman
no flags Details | Formatted Diff | Diff
patch with test case (4.95 KB, patch)
2010-12-12 21:28 PST, Benjamin (Ben) Kalman
no flags Details | Formatted Diff | Diff
Demonstration of failing test (813 bytes, text/html)
2010-12-15 16:44 PST, Benjamin (Ben) Kalman
no flags Details
Patch (9.62 KB, patch)
2010-12-20 05:03 PST, Benjamin (Ben) Kalman
no flags Details | Formatted Diff | Diff
Patch (10.34 KB, patch)
2010-12-20 16:36 PST, Benjamin (Ben) Kalman
no flags Details | Formatted Diff | Diff
Patch (11.73 KB, patch)
2010-12-20 20:20 PST, Benjamin (Ben) Kalman
no flags Details | Formatted Diff | Diff
Patch (11.71 KB, patch)
2010-12-21 04:12 PST, Benjamin (Ben) Kalman
no flags Details | Formatted Diff | Diff
Patch (12.12 KB, patch)
2010-12-21 17:18 PST, Benjamin (Ben) Kalman
no flags Details | Formatted Diff | Diff
Patch (11.92 KB, patch)
2010-12-21 17:33 PST, Benjamin (Ben) Kalman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Tsui 2010-12-06 18:39:43 PST
What steps will reproduce the problem?
1. Place cursor at the start of a line of text (but not the first line) inside a contenteditable that has been wrapped.
2. Press shift-end

What is the expected result?
The selection should expand from the start of the line, to the end of the line.

What happens instead?
The selection jumps to the previous line.


Please provide any additional information below. Attach a screenshot if
possible.
I've tested this to be an issue on Linux, and Mac OSX, have not tested windows.
Comment 1 Mark Tsui 2010-12-06 19:30:13 PST
Created attachment 75774 [details]
test case
Comment 2 Srikumar B 2010-12-10 06:37:51 PST
I am able to reproduce the issue on gtk-webkit port also. I will be analyzing the issue
Comment 3 Benjamin (Ben) Kalman 2010-12-12 18:47:32 PST
Caused by https://bugs.webkit.org/show_bug.cgi?id=33413
Comment 4 Benjamin (Ben) Kalman 2010-12-12 20:13:02 PST
Created attachment 76346 [details]
Patch
Comment 5 Benjamin (Ben) Kalman 2010-12-12 20:19:26 PST
Created attachment 76347 [details]
Patch
Comment 6 Benjamin (Ben) Kalman 2010-12-12 20:25:01 PST
I've uploaded two patches; the former extends the little hack from https://bugs.webkit.org/show_bug.cgi?id=33413 to work here too, while the latter (imo the nicer but more invasive approach) changes positionForPlatform to use visibleEnd/visibleStart rather than end/start (which also makes the fix from 33413 unnecessary).

I'll write a couple of tests now.
Comment 7 Benjamin (Ben) Kalman 2010-12-12 21:21:06 PST
Created attachment 76350 [details]
Patch
Comment 8 Benjamin (Ben) Kalman 2010-12-12 21:28:23 PST
Created attachment 76351 [details]
patch with test case
Comment 9 Eric Seidel (no email) 2010-12-13 00:21:30 PST
I believe this is a mac vs. windows thing and will need to be guarded by the editing behavior setting.
Comment 10 Benjamin (Ben) Kalman 2010-12-13 04:03:55 PST
Do you mean beyond what is already guarded?
Comment 11 WebKit Review Bot 2010-12-13 09:28:11 PST
Comment on attachment 76351 [details]
patch with test case

Clearing flags on attachment: 76351

Committed r73923: <http://trac.webkit.org/changeset/73923>
Comment 12 WebKit Review Bot 2010-12-13 09:28:17 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 WebKit Review Bot 2010-12-13 10:09:21 PST
http://trac.webkit.org/changeset/73923 might have broken Qt Linux Release
The following tests are not passing:
editing/selection/extend-selection-home-end.html
editing/selection/extend-to-line-boundary.html
Comment 14 Ojan Vafai 2010-12-13 10:44:19 PST
Rolled out in https://bugs.webkit.org/show_bug.cgi?id=50944. Please investigate the Win/Linux failure. I took a quick look and I *think* it's a real failure and not just a test issue, but I'm not 100% sure.
Comment 15 Ryosuke Niwa 2010-12-13 14:13:55 PST
http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r73923%20(7370)/editing/selection/extend-selection-home-end-pretty-diff.html

--- /home/buildbot/slave/WebKit-BuildSlave/win-release-tests/build/layout-test-results/editing/selection/extend-selection-home-end-expected.txt	2010-12-13 16:42:59.274773600 -0800
+++ /home/buildbot/slave/WebKit-BuildSlave/win-release-tests/build/layout-test-results/editing/selection/extend-selection-home-end-actual.txt	2010-12-13 16:42:59.273773500 -0800
@@ -124,14 +124,14 @@
   Extending backward:  "ABC abc DEF"[(0,11), (0,0)]
 Test 21, LTR:
   Extending forward: "abcdefg abcdefg abcdefg a abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg "[(0,0), (0,8)]
-  Extending backward:  "abcdefg abcdefg abcdefg a abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg "[(0,8)]
+  Extending backward:  "abcdefg abcdefg abcdefg a abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg "[(0,8), (0,0)]
 Test 21, RTL:
   Extending forward: "abcdefg abcdefg abcdefg a abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg "[(0,0), (0,8)]
-  Extending backward:  "abcdefg abcdefg abcdefg a abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg "[(0,8)]
+  Extending backward:  "abcdefg abcdefg abcdefg a abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg "[(0,8), (0,0)]
 Test 22, LTR:
   Extending forward: "abcdefg abcdefg abcdefg a abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg "[(0,0), (0,8)]
-  Extending backward:  "abcdefg abcdefg abcdefg a abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg "[(0,8)]
+  Extending backward:  "abcdefg abcdefg abcdefg a abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg "[(0,8), (0,0)]
 Test 22, RTL:
   Extending forward: "abcdefg abcdefg abcdefg a abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg "[(0,0), (0,8)]
-  Extending backward:  "abcdefg abcdefg abcdefg a abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg "[(0,8)]
+  Extending backward:  "abcdefg abcdefg abcdefg a abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg "[(0,8), (0,0)]
 
This result seems an improvement to me.  We should just rebaseline the test.
Comment 16 Benjamin (Ben) Kalman 2010-12-13 14:24:51 PST
Sorry for not noticing these when I was running the tests myself, there didn't seem to be any changes in behaviour :-(
Comment 17 Benjamin (Ben) Kalman 2010-12-14 00:06:53 PST
I've run the offending tests locally on the two platforms I have, snow leopard and gtk.  Both (a) run the new test fine, and (b) don't change the result of editing/selection/extend-selection-home-end.html.

So it's strange that it fails on qt and windows.  I would have thought these changes are platform independent.  I guess I'll need to figure out how to build and run/test this stuff on windows, in order to debug it?  It doesn't really help that the extend-selection-home-end test is rather unhelpful in describing what happened.
Comment 18 Benjamin (Ben) Kalman 2010-12-15 16:44:41 PST
Created attachment 76710 [details]
Demonstration of failing test
Comment 19 Benjamin (Ben) Kalman 2010-12-15 16:51:18 PST
(In reply to comment #18)
> Created an attachment (id=76710) [details]
> Demonstration of failing test

(In reply to comment #17)
> I've run the offending tests locally on the two platforms I have, snow leopard and gtk.  Both (a) run the new test fine, and (b) don't change the result of editing/selection/extend-selection-home-end.html.
> 
> So it's strange that it fails on qt and windows.  I would have thought these changes are platform independent.  I guess I'll need to figure out how to build and run/test this stuff on windows, in order to debug it?  It doesn't really help that the extend-selection-home-end test is rather unhelpful in describing what happened.

I've now looked into this, and attached a plain-html version of the "failing" test.  As Ryosuke said, the new behaviour is indeed correct; to see this:
 1. open the test, click at the start of either div
 2. press shift+end (on windows/linux) or (because end behaves weirdly on mac), on mac, type in the console
    - set(at())
    - el("forward")
 3. the first line should be highlighted
 4. press shift+home (on windows/linux) or on mac
    - el("backward")
Unexpectedly, the selection doesn't change.  /With/ this patch, the selection changes to be back at the start of the line.

The exception is actually the mac, the behaviour for which doesn't change as it does on window/linux.  I presume this has something to do with the explicit differences on how selections are made on mac, i.e.

if (settings && settings->editingBehaviorType() == EditingMacBehavior)
    pos = isGetStart ? m_selection.start() : m_selection.end();
    return isGetStart ? m_selection.visibleStart() : m_selection.visibleEnd();
} else {
    pos = m_selection.isBaseFirst() ? m_selection.end() : m_selection.start();
    return m_selection.isBaseFirst() ? m_selection.visibleEnd() : m_selection.visibleStart();
}

In conclusion: I'll upload a new version of this patch which includes the new baseline for the test that broke last time.
Comment 20 Ryosuke Niwa 2010-12-15 16:56:40 PST
FWIW, there is https://bugs.webkit.org/show_bug.cgi?id=49873.  We could fix that bug first so that we'll have shower rebaselines to deal with.
Comment 21 Ryosuke Niwa 2010-12-15 16:57:11 PST
s/shower/fewer/
Comment 22 Benjamin (Ben) Kalman 2010-12-15 17:07:29 PST
(In reply to comment #20)
> FWIW, there is https://bugs.webkit.org/show_bug.cgi?id=49873.  We could fix that bug first so that we'll have shower rebaselines to deal with.

Ok cool I'll do that first, if you assign the bug to me.
Comment 23 Benjamin (Ben) Kalman 2010-12-20 05:03:25 PST
Created attachment 76987 [details]
Patch
Comment 24 Ryosuke Niwa 2010-12-20 10:17:09 PST
Comment on attachment 76987 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76987&action=review

Code change looks good but we definitely need a detailed description on what you're changing and why.

> LayoutTests/ChangeLog:7
> +

Please explain your change.

> WebCore/ChangeLog:7
> +

Please explain what you're testing and/or your code change.

> WebCore/editing/SelectionController.cpp:315
>      else {
>          // Linux and Windows always extend selections from the extent endpoint.
>          // FIXME: VisibleSelection should be fixed to ensure as an invariant that
>          // base/extent always point to the same nodes as start/end, but which points
>          // to which depends on the value of isBaseFirst. Then this can be changed
>          // to just return m_sel.extent().
> -        pos = m_selection.isBaseFirst() ? m_selection.end() : m_selection.start();
> +        return m_selection.isBaseFirst() ? m_selection.visibleEnd() : m_selection.visibleStart();
>      }

Nit: we don't put an else statement when the proceeding if statement ends with return.

> WebCore/editing/SelectionController.cpp:386
> -        pos = endForPlatform();
> -        pos.setAffinity(UPSTREAM);
> -        pos = logicalEndOfLine(pos);
> +        pos = logicalEndOfLine(endForPlatform());

These statements are introduced in http://trac.webkit.org/changeset/55990 but your change seems to be an improvement.
Comment 25 Enrica Casucci 2010-12-20 16:24:34 PST
Comment on attachment 76987 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76987&action=review

>> WebCore/editing/SelectionController.cpp:315
>>      }
> 
> Nit: we don't put an else statement when the proceeding if statement ends with return.

I agree with Ryosuke. We definitely don't want to see the else in a case like this.
Comment 26 Enrica Casucci 2010-12-20 16:25:09 PST
The change looks good to me.
Comment 27 Benjamin (Ben) Kalman 2010-12-20 16:36:02 PST
Created attachment 77052 [details]
Patch
Comment 28 Ryosuke Niwa 2010-12-20 16:44:34 PST
Comment on attachment 77052 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77052&action=review

The code change looks good to me. You should fix LayoutTests/ChangeLog and the test so that it passes on all platforms.

> LayoutTests/ChangeLog:9
> +        Add regression test that shift-end selects to the end of the line, and rebaseline a test
> +        which had incorrect expected results.

You should explain what was incorrect and *why* new expected results are correct.

> LayoutTests/editing/selection/extend-to-line-boundary.html:6
> +  #textContainer {
> +    width: 200px;
> +  }

This test is going to fail on all other platforms except the one you used to make this test because font metrics are different on each platform.  You should consider using Ahem font.
Comment 29 Benjamin (Ben) Kalman 2010-12-20 20:20:09 PST
Created attachment 77079 [details]
Patch
Comment 30 Ryosuke Niwa 2010-12-20 22:57:43 PST
Comment on attachment 77079 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77079&action=review

LGTM assuming that you address the following comments in some way.

> LayoutTests/editing/selection/extend-to-line-boundary.html:2
> +

No need for a blank line.

> LayoutTests/editing/selection/extend-to-line-boundary.html:21
> +      div.setAttribute("contenteditable", "true");

You can do div.contentEditable = true.

> LayoutTests/editing/selection/extend-to-line-boundary.html:28
> +      getSelection().setBaseAndExtent(element.childNodes[0], 0);

You should be calling setPosition if you're not passing the endContainer and endOffset.

> LayoutTests/editing/selection/extend-to-line-boundary.html:39
> +  ltrText = "the quick brown fox jumps";
> +  ltrTextContainer = createEditableMultilineDiv(ltrText, 3);
> +  document.body.appendChild(ltrTextContainer);
> +  selectSecondLine(ltrTextContainer);
> +  Markup.dump(ltrTextContainer, "LTR selection");

I would have checked offset and printed PASS/FAIL so that other people can easily know when it's regressed.
Comment 31 Benjamin (Ben) Kalman 2010-12-21 04:12:06 PST
Created attachment 77100 [details]
Patch
Comment 32 Benjamin (Ben) Kalman 2010-12-21 04:12:49 PST
Comment on attachment 77079 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77079&action=review

>> LayoutTests/editing/selection/extend-to-line-boundary.html:39
>> +  Markup.dump(ltrTextContainer, "LTR selection");
> 
> I would have checked offset and printed PASS/FAIL so that other people can easily know when it's regressed.

Which particular way of writing tests would you use to do that?  It's not easy to print stuff with Markup, but it's nice seeing what's actually selected (e.g. given a test failure seeing where the cursor actually is rather than just a FAIL or "" or something).  The description on the test should be ok to see regressions?
Comment 33 Ryosuke Niwa 2010-12-21 08:55:08 PST
(In reply to comment #32)
> (From update of attachment 77079 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77079&action=review
> > I would have checked offset and printed PASS/FAIL so that other people can easily know when it's regressed.
> 
> Which particular way of writing tests would you use to do that?  It's not easy to print stuff with Markup, but it's nice seeing what's actually selected (e.g. given a test failure seeing where the cursor actually is rather than just a FAIL or "" or something).  The description on the test should be ok to see regressions?

You could just call dumpAsText manually and print text.  I think dump-as-markup.js is useful only if you're interested in seeing how DOM changed.  In the case where we're only interested in seeing the selection, and we know exactly which container/offset pairs the first range has, I prefer comparing the result in script.

My problem with your test is that it doesn't tells us what is expected and what is not in the output.  Say your test output changed and had a diff:
-| "the quick brown fox jumps <#selection-anchor>the quick brown fox jumps <#selection-focus>the quick brown fox jumps"
+| "the quick brown fox jumps<#selection-anchor> the quick brown fox jumps<#selection-focus> the quick brown fox jumps"

How do we know if this is a regression or just needing a rebaseline?
Comment 34 Benjamin (Ben) Kalman 2010-12-21 17:18:32 PST
Created attachment 77169 [details]
Patch
Comment 35 Benjamin (Ben) Kalman 2010-12-21 17:20:09 PST
(In reply to comment #33)
> (In reply to comment #32)
> > (From update of attachment 77079 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=77079&action=review
> > > I would have checked offset and printed PASS/FAIL so that other people can easily know when it's regressed.
> > 
> > Which particular way of writing tests would you use to do that?  It's not easy to print stuff with Markup, but it's nice seeing what's actually selected (e.g. given a test failure seeing where the cursor actually is rather than just a FAIL or "" or something).  The description on the test should be ok to see regressions?
> 
> You could just call dumpAsText manually and print text.  I think dump-as-markup.js is useful only if you're interested in seeing how DOM changed.  In the case where we're only interested in seeing the selection, and we know exactly which container/offset pairs the first range has, I prefer comparing the result in script.
> 
> My problem with your test is that it doesn't tells us what is expected and what is not in the output.  Say your test output changed and had a diff:
> -| "the quick brown fox jumps <#selection-anchor>the quick brown fox jumps <#selection-focus>the quick brown fox jumps"
> +| "the quick brown fox jumps<#selection-anchor> the quick brown fox jumps<#selection-focus> the quick brown fox jumps"
> 
> How do we know if this is a regression or just needing a rebaseline?

Thanks, I've modified the test to use dumpAsText and print PASS/FAIL.
Comment 36 Ryosuke Niwa 2010-12-21 17:29:33 PST
Comment on attachment 77169 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77169&action=review

Thanks! LGTM. Please wait for a formal review :)

> LayoutTests/editing/selection/extend-to-line-boundary.html:8
> +<script>
> +    function log(s) {
> +        document.getElementById("console").innerHTML += s + "\n";
> +    }
> +

Nit: I don't think you need to indent.
Comment 37 Benjamin (Ben) Kalman 2010-12-21 17:31:18 PST
Comment on attachment 77169 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77169&action=review

>> LayoutTests/editing/selection/extend-to-line-boundary.html:8
>> +
> 
> Nit: I don't think you need to indent.

heh, ok.
Comment 38 Benjamin (Ben) Kalman 2010-12-21 17:33:39 PST
Created attachment 77171 [details]
Patch
Comment 39 Ryosuke Niwa 2010-12-21 17:34:57 PST
(In reply to comment #38)
> Created an attachment (id=77171) [details]
> Patch

My LGTM withstands.
Comment 40 WebKit Commit Bot 2010-12-21 19:43:33 PST
The commit-queue encountered the following flaky tests while processing attachment 77171 [details]:

inspector/extensions-audits-api.html bug 51442 (author: caseq@chromium.org)
The commit-queue is continuing to process your patch.
Comment 41 WebKit Commit Bot 2010-12-21 19:45:06 PST
Comment on attachment 77171 [details]
Patch

Clearing flags on attachment: 77171

Committed r74452: <http://trac.webkit.org/changeset/74452>
Comment 42 WebKit Commit Bot 2010-12-21 19:45:16 PST
All reviewed patches have been landed.  Closing bug.
Comment 43 WebKit Review Bot 2010-12-21 20:26:12 PST
http://trac.webkit.org/changeset/74452 might have broken Qt Linux Release
The following tests are not passing:
editing/selection/extend-to-line-boundary.html