|Summary:||Win: Double clicking words in selects adjacent newlines|
|Product:||WebKit||Reporter:||Erik Arvidsson <arv>|
|Component:||HTML Editing||Assignee:||Erik Arvidsson <arv>|
|Severity:||Normal||CC:||bweinstein, commit-queue, darin, eric, rniwa, tonikitoo, tony|
|Version:||528+ (Nightly build)|
Description Erik Arvidsson 2009-10-08 14:21:25 PDT
Comment 1 Erik Arvidsson 2009-10-08 15:23:57 PDT
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 2 Darin Adler 2009-10-08 16:34:55 PDT
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.
Comment 3 Erik Arvidsson 2009-10-08 16:48:25 PDT
Thanks for the quick review. I'll remove isSpace and just handle '\n' in VisibleSelection as you suggested.
Comment 4 Erik Arvidsson 2009-10-08 17:32:35 PDT
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(-)
Comment 5 Erik Arvidsson 2009-10-08 17:41:16 PDT
Created attachment 40921 [details] Fix ChangeLogs https://bugs.webkit.org/show_bug.cgi?id=30234 --- 11 files changed, 238 insertions(+), 1 deletions(-)
Comment 6 Erik Arvidsson 2009-10-13 10:08:07 PDT
Darin, can you take another look?
Comment 7 Adam Barth 2009-10-14 23:31:30 PDT
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 8 WebKit Commit Bot 2009-10-19 00:00:06 PDT
Comment on attachment 40921 [details] Fix ChangeLogs Clearing flags on attachment: 40921 Committed r49767: <http://trac.webkit.org/changeset/49767>
Comment 9 WebKit Commit Bot 2009-10-19 00:00:11 PDT
All reviewed patches have been landed. Closing bug.
Comment 10 Brian Weinstein 2009-10-19 11:15:07 PDT
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.
Comment 11 Erik Arvidsson 2009-10-19 11:45:42 PDT
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.
Comment 12 Erik Arvidsson 2009-10-19 12:43:31 PDT
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.
Comment 13 Erik Arvidsson 2009-10-19 14:30:04 PDT
Created attachment 41452 [details] Move layout tests to non platform dirs.
Comment 14 Eric Seidel (no email) 2009-10-19 14:35:45 PDT
This seems like a strange quirk to Apple's window port. I think other windows ports will probably want windows-style behavior in DRT.
Comment 15 Daniel Bates 2009-10-19 14:46:18 PDT
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.
Comment 16 Darin Adler 2009-10-19 17:14:10 PDT
(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?
Comment 17 Erik Arvidsson 2009-10-19 17:48:34 PDT
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.
Comment 18 Eric Seidel (no email) 2009-12-14 13:06:40 PST
bweinstein added it to the skipped list in http://trac.webkit.org/changeset/49793
Comment 19 Eric Seidel (no email) 2009-12-14 13:06:59 PST
I'm confused as to what's the status of this patch or if it's really up for review?
Comment 20 Erik Arvidsson 2009-12-14 13:10:17 PST
This fixes the test (and removes the entry in the platform/win/Skipped). This is ready to be reviewed.
Comment 21 Erik Arvidsson 2009-12-16 17:11:52 PST
Created attachment 45026 [details] Updated the ChangeLog with more description
Comment 22 Erik Arvidsson 2009-12-16 17:17:17 PST
Created attachment 45027 [details] Updated the ChangeLog with more description as well as removed some whitespace changes
Comment 23 Darin Adler 2009-12-18 10:42:45 PST
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 24 Maciej Stachowiak 2009-12-28 20:02:00 PST
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.
Comment 25 Ryosuke Niwa 2010-10-27 23:12:43 PDT
Have you tried setEditingBehavior?