Bug 50610 – REGRESSION(r55990): Shift-End does not select to the end of the line

• Component: HTML Editing
• Status: RESOLVED FIXED

[Mark Tsui]

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.

[Mark Tsui]

Created attachment 75774 [details]
test case

[Srikumar B]

I am able to reproduce the issue on gtk-webkit port also. I will be analyzing the issue

[Benjamin (Ben) Kalman]

Caused by https://bugs.webkit.org/show_bug.cgi?id=33413

[Benjamin (Ben) Kalman]

Created attachment 76346 [details]
Patch

[Benjamin (Ben) Kalman]

Created attachment 76347 [details]
Patch

[Benjamin (Ben) Kalman]

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.

[Benjamin (Ben) Kalman]

Created attachment 76350 [details]
Patch

[Benjamin (Ben) Kalman]

Created attachment 76351 [details]
patch with test case

[Eric Seidel (no email)]

I believe this is a mac vs. windows thing and will need to be guarded by the editing behavior setting.

[Benjamin (Ben) Kalman]

Do you mean beyond what is already guarded?

[WebKit Review Bot]

Comment on attachment 76351 [details]
patch with test case

Clearing flags on attachment: 76351

Committed r73923: <http://trac.webkit.org/changeset/73923>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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

[Ojan Vafai]

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.

[Ryosuke Niwa]

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.

[Benjamin (Ben) Kalman]

Sorry for not noticing these when I was running the tests myself, there didn't seem to be any changes in behaviour :-(

[Benjamin (Ben) Kalman]

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.

[Benjamin (Ben) Kalman]

Created attachment 76710 [details]
Demonstration of failing test

[Benjamin (Ben) Kalman]

(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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

s/shower/fewer/

[Benjamin (Ben) Kalman]

(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.

[Benjamin (Ben) Kalman]

Created attachment 76987 [details]
Patch

[Ryosuke Niwa]

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.

[Enrica Casucci]

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.

[Enrica Casucci]

The change looks good to me.

[Benjamin (Ben) Kalman]

Created attachment 77052 [details]
Patch

[Ryosuke Niwa]

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.

[Benjamin (Ben) Kalman]

Created attachment 77079 [details]
Patch

[Ryosuke Niwa]

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.

[Benjamin (Ben) Kalman]

Created attachment 77100 [details]
Patch

[Benjamin (Ben) Kalman]

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?

[Ryosuke Niwa]

(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?

[Benjamin (Ben) Kalman]

Created attachment 77169 [details]
Patch

[Benjamin (Ben) Kalman]

(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.

[Ryosuke Niwa]

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.

[Benjamin (Ben) Kalman]

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.

[Benjamin (Ben) Kalman]

Created attachment 77171 [details]
Patch

[Ryosuke Niwa]

(In reply to comment #38)
> Created an attachment (id=77171) [details]
> Patch

My LGTM withstands.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 77171 [details]:

inspector/extensions-audits-api.html bug 51442 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 77171 [details]
Patch

Clearing flags on attachment: 77171

Committed r74452: <http://trac.webkit.org/changeset/74452>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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