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.
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(-)
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.
Thanks for the quick review. I'll remove isSpace and just handle '\n' in VisibleSelection as you suggested.
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(-)
Created attachment 40921 [details] Fix ChangeLogs https://bugs.webkit.org/show_bug.cgi?id=30234 --- 11 files changed, 238 insertions(+), 1 deletions(-)
Darin, can you take another look?
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.
Comment on attachment 40921 [details] Fix ChangeLogs Clearing flags on attachment: 40921 Committed r49767: <http://trac.webkit.org/changeset/49767>
All reviewed patches have been landed. Closing bug.
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.
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.
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.
Created attachment 41452 [details] Move layout tests to non platform dirs.
This seems like a strange quirk to Apple's window port. I think other windows ports will probably want windows-style behavior in DRT.
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.
(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?
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.
bweinstein added it to the skipped list in http://trac.webkit.org/changeset/49793
I'm confused as to what's the status of this patch or if it's really up for review?
This fixes the test (and removes the entry in the platform/win/Skipped). This is ready to be reviewed.
Created attachment 45026 [details] Updated the ChangeLog with more description
Created attachment 45027 [details] Updated the ChangeLog with more description as well as removed some whitespace changes
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?
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.
Have you tried setEditingBehavior?