Bug 30234 – Win: Double clicking words in selects adjacent newlines

• Component: HTML Editing
• Status: REOPENED

[Erik Arvidsson]

From Chromium bug, http://crbug.com/12679

Double clicking the last word on a line also selects the following adjacent new lines.

This happens on Windows which has isSelectTrailingWhitespaceEnabled set to true. Mac and Linux returns false for that. This cause us to call appendTrailingWhitespace which incorrectly appends newlines.

[Erik Arvidsson]

Created attachment 40915 [details]
Fixes issue where doubleclicking a word could select following adjacent newlines.

https://bugs.webkit.org/show_bug.cgi?id=30234
---
 12 files changed, 246 insertions(+), 1 deletions(-)

[Darin Adler]

Comment on attachment 40915 [details]
Fixes issue where doubleclicking a word could select following adjacent newlines.

I'm not so happy with this function being named isSpace(). The <ctype.h> function named isspace() returns true for newlines, and having such a similarly-named function with different behavior seems wrong. So if you do want to add a function I think it needs a different name.

It's also worth noting that the characters coming out of the CharacterIterator will not be '\r' characters under any circumstance. I think for this patch it would be better to just add a special case for '\n' in the VisibleSelection code rather than adding the new function.

This otherwise seems fine.

[Erik Arvidsson]

Thanks for the quick review. I'll remove isSpace and just handle '\n' in VisibleSelection as you suggested.

[Erik Arvidsson]

Created attachment 40920 [details]
Remove isSpace and check for newline in appendTrailingWhitespace

https://bugs.webkit.org/show_bug.cgi?id=30234
---
 11 files changed, 242 insertions(+), 1 deletions(-)

[Erik Arvidsson]

Created attachment 40921 [details]
Fix ChangeLogs

https://bugs.webkit.org/show_bug.cgi?id=30234
---
 11 files changed, 238 insertions(+), 1 deletions(-)

[Erik Arvidsson]

Darin, can you take another look?

[Adam Barth]

Comment on attachment 40921 [details]
Fix ChangeLogs

R+ given that you've addressed Darin's comment from Comment #2 and he said the rest looked good.

[WebKit Commit Bot]

Comment on attachment 40921 [details]
Fix ChangeLogs

Clearing flags on attachment: 40921

Committed r49767: <http://trac.webkit.org/changeset/49767>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brian Weinstein]

The test for this inside platform/win is not passing on the bots:

http://build.webkit.org/results/Windows%20Release%20(Tests)/r49789%20(5277)/platform/win/editing/selection/doubleclick-should-not-expand-across-lines-pretty-diff.html

Re-opening the bug and adding this test to the Windows Skipped List.

[Erik Arvidsson]

Sorry about that. It seems like DRT is not platform specific and uses the Mac setting for Windows. I'll need to update DRT to do the right thing on Windows.

[Erik Arvidsson]

I think I misunderstood how DRT was supposed to work. I'm going to move the test out of platform since DRT behaves the same on all platforms. We'll just have to rebaseline our chromium layout tests instead.

[Erik Arvidsson]

Created attachment 41452 [details]
Move layout tests to non platform dirs.

[Eric Seidel (no email)]

This seems like a strange quirk to Apple's window port.  I think other windows ports will probably want windows-style behavior in DRT.

[Daniel Bates]

Just wanted to let you know I landed changes to the Windows skip file before yours. So, you will have to update that bit of your patch before you land.

For your reference the changes landed were part of bug #30266.

(In reply to comment #13)
> Created an attachment (id=41452) [details]
> Move layout tests to non platform dirs.

[Darin Adler]

(In reply to comment #14)
> This seems like a strange quirk to Apple's window port.  I think other windows
> ports will probably want windows-style behavior in DRT.

I don’t think this is an intentional quirk. Could you clarify what the quirk is?

[Erik Arvidsson]

I don't think this is so much a quirk as an issue with Safari and DRT. WebViewClient::isSelectTrailingWhitespaceEnabled is supposed to return true on Windows but that is not the case with Safari Win nor DRT.

[Eric Seidel (no email)]

bweinstein added it to the skipped list in http://trac.webkit.org/changeset/49793

[Eric Seidel (no email)]

I'm confused as to what's the status of this patch or if it's really up for review?

[Erik Arvidsson]

This fixes the test (and removes the entry in the platform/win/Skipped). This is ready to be reviewed.

[Erik Arvidsson]

Created attachment 45026 [details]
Updated the ChangeLog with more description

[Erik Arvidsson]

Created attachment 45027 [details]
Updated the ChangeLog with more description as well as removed some whitespace changes

[Darin Adler]

Comment on attachment 45027 [details]
Updated the ChangeLog with more description as well as removed some whitespace changes

> +        Move the layout tests to non platform dir. Since DRT on Windows uses Mac
> +        settings all platforms produce the same result so there is no need for
> +        platform specific expectations.

I don't understand this. Aren't there platforms other than Windows and Mac?

Don't we want to test both modes? Can DRT be told to switch modes so we can test both modes?

[Maciej Stachowiak]

Comment on attachment 45027 [details]
Updated the ChangeLog with more description as well as removed some whitespace changes

r- to address Darin's comment on Dec 18.

[Ryosuke Niwa]

Have you tried setEditingBehavior?