Currently it also happens if you do a word-selection via cmd+option+left/right.
From webkit-dev: On Tue, Feb 23, 2010 at 1:49 PM, Darin Adler <darin@apple.com> wrote: > On Feb 22, 2010, at 6:47 PM, Ojan Vafai wrote: > Option-shift-right does not give you smart deletion behavior in Mac OS X NSTextView. Selecting words with double click does. The smart-delete-001.html test is testing a particular case of smart deletion and we do need a test for that fix. But it incorrectly depends on smart deletion mode being set when extending a selection by word. We probably need to turn this into two tests: > > 1) Test that extending by word does *not* give you smart deletion. > > 2) Test that double clicking at the beginning of a line to do smart deletion correctly deletes both the word and a space. > > It might be tricky to write (2) because I don’t know of a way to make a selection that will result in smart deletion with DOM APIs. Maybe execCommand("SelectWord")? For 1, the current test should suffice. For 2, using eventSender to double-click should work, right?
Yes, eventSender would work, but a test that also works in the browser, not just in DumpRenderTree, would be preferred.
Created attachment 50066 [details] Patch
Would be cool if we could first fix the tests to not depend on keyboard-based selections and then go back and fix the bug. :)
Created attachment 50147 [details] Patch
Comment on attachment 50147 [details] Patch As requested, here's just the test changes on a clean checkout. I'll repost a patch with the WebCore changes once this is committed.
Comment on attachment 50147 [details] Patch I have a few comments but they are all about pretty small stuff, so feel free to fix these on landing (unless you want to me to look it over again). This current patch isn't really about the bug that it is attached to. It is doing some different (which is a good step to fix the bug). I wish there was a separate bug just because I find it confusing to read the ChangeLog which says "smartdelete should only occur after double-click" but this patch doesn't fix that. > diff --git a/LayoutTests/editing/deleting/non-smart-delete.html b/LayoutTests/editing/deleting/non-smart-delete.html > +<br> > +The first word should be deleted. The space following it should remain. It should like this this (currently this is broken and the space is deleted): "It should like this this" I think you meant "It should look like this" If something is broken, it would be nice to reference a bug number. (I think it is this bug.) > diff --git a/LayoutTests/editing/deleting/smart-delete-003.html b/LayoutTests/editing/deleting/smart-delete-003.html > if (window.layoutTestController) > layoutTestController.dumpEditingCallbacks(); > </script> > +<script src=../editing.js language="JavaScript" type="text/JavaScript" ></script> > <p>This tests deleting a selection created with a word granularity. To run it manually, double click on 'bar' and hit delete. You should see 'foo bar'.</p> Shouldn't you see 'foo baz'? > diff --git a/LayoutTests/editing/deleting/smart-delete-004.html b/LayoutTests/editing/deleting/smart-delete-004.html > <p>This tests deleting a selection created with a word granularity. To run it manually, double click on 'bar' and hit forward delete. You should see 'foo bar'.</p> 'foo baz' here as well. > diff --git a/LayoutTests/editing/pasteboard/drag-drop-modifies-page.html b/LayoutTests/editing/pasteboard/drag-drop-modifies-page.html > +<p>This tests non-smartmove drag/drop. The space should be deleted on move, > +but not reinserted on drop, resulting in "worldhello". Currently there's a bug > +where the space is reinserted on drop.</p> Bug # reference would be nice. > diff --git a/LayoutTests/editing/pasteboard/smart-drag-drop.html b/LayoutTests/editing/pasteboard/smart-drag-drop.html > +function editingTest() { > + > + if (!window.eventSender) > + return; > + doubleClickAtSelectionStart(); > + > + // Drag 'hello' > + var e = document.getElementById("dragme"); > + x = e.offsetLeft + 10; Why 10 instead of something based on offsetWidth? > + // and drop it off to the right somewhere Would be nice to add a "." at the end. > +<p>Tests that drag/drop after double-click does a smart drag. Specifically the end result should have a space: "world hello".</p> This should include instructions for how to do the test manually as well. > diff --git a/LayoutTests/editing/selection/script-tests/delete-word-granularity-text-control.js b/LayoutTests/editing/selection/script-tests/delete-word-granularity-text-control.js > +description("Test that setSelectedRange resets the seleciton granularity to CharacterGranularity."); Add info about how to do it manually. > +var successfullyParsed = true; > \ No newline at end of file Please add the newline. > diff --git a/LayoutTests/editing/style/style-boundary-005.html b/LayoutTests/editing/style/style-boundary-005.html > @@ -51,7 +51,8 @@ Pasting at style boundary does not crash or produce empty style span(s). > <div class="expected-results"> > Expected Results: > <br> > -Should see this content in the red box below: <br><div>one two three <b>four</b> one</div> > +Should see this content in the red box below (currently there's a bug where > +there is an extra space after the bold element: <br><div>one two three <b>four</b>one</div> Please close your parenthetical remark which begins "(currently" Also bug reference?
+ Fixing to smart-delete only on mouse-based selections will be a followup patch. This makes me wonder if there are other ways to make selection, besides mouse and keyboard. E.g., might this affect VoiceOver?
(In reply to comment #8) > + Fixing to smart-delete only on mouse-based selections will be a > followup patch. > > This makes me wonder if there are other ways to make selection, besides mouse > and keyboard. E.g., might this affect VoiceOver? Hmmm. Hadn't heard of VoiceOver before. I also don't have Snow Leopard to test it. How does VoiceOver work with WebKit? Does it go through selection.modify? Does selecting a word via VoiceOver lead to smart insert/delete in TextEdit? Sorry, I'd test this all myself if I had easy access to a Snow Leopard machine.
(In reply to comment #9) > (In reply to comment #8) > > + Fixing to smart-delete only on mouse-based selections will be a > > followup patch. > > > > This makes me wonder if there are other ways to make selection, besides mouse > > and keyboard. E.g., might this affect VoiceOver? > > Hmmm. Hadn't heard of VoiceOver before. I also don't have Snow Leopard to test > it. How does VoiceOver work with WebKit? Does it go through selection.modify? > Does selecting a word via VoiceOver lead to smart insert/delete in TextEdit? > > Sorry, I'd test this all myself if I had easy access to a Snow Leopard machine. VoiceOver uses keyboard selection mostly. It doesn't go through a separate API to select, so you should be ok
Using keyboard selection seems to mean that smart delete won't work with VoiceOver any more.
(In reply to comment #11) > Using keyboard selection seems to mean that smart delete won't work with > VoiceOver any more. Should it?
> > diff --git a/LayoutTests/editing/selection/script-tests/delete-word-granularity-text-control.js b/LayoutTests/editing/selection/script-tests/delete-word-granularity-text-control.js > > +description("Test that setSelectedRange resets the seleciton granularity to CharacterGranularity."); > > Add info about how to do it manually. Can't really be done manually. It's testing that modifying the selection from JS resets the selection granularity. Otherwise, addressed all comments and will commit shortly.
Committed r55913: <http://trac.webkit.org/changeset/55913>
Created attachment 50616 [details] Patch
Comment on attachment 50147 [details] Patch Cleared David Levin's review+ from obsolete attachment 50147 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment on attachment 50616 [details] Patch Just a few minor things, which you can fix when landing it. > diff --git a/WebCore/editing/VisibleSelection.h b/WebCore/editing/VisibleSelection.h > + void validate(TextGranularity granularity = CharacterGranularity); Pls remove param name "granularity" as it adds no information here. > + void setStartAndEndFromBaseAndExtentRespectingGranularity(TextGranularity granularity); Pls remove param name "granularity" as it adds no information here. > diff --git a/WebCore/page/DragController.cpp b/WebCore/page/DragController.cpp > + // NSTextView behavior is to always smartdelete on moving a selection, Rather than lower casing smartdelete, smartinsert, selectiongranularity, wordgranularity (which I find hard to read), please consider either UpperCasing both words or separating them or formatting as is done in the code smartDelete. Personally, I prefer smart delete, smart insert, selection granularity, word granularity. > + // but only to smartinsert if the selectiongranularity is wordgranularity. > diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog > +2010-03-12 Ojan Vafai <ojan@chromium.org> > + > + Reviewed by NOBODY (OOPS!). > + > + Need a short description and bug URL (OOPS!) oops indeed :) > + > + * src/WebFrameImpl.cpp: > + (WebKit::WebFrameImpl::selectWordAroundPosition): > + > diff --git a/WebKit/chromium/src/WebFrameImpl.cpp b/WebKit/chromium/src/WebFrameImpl.cpp > if (frame->shouldChangeSelection(selection)) As you mentioned, this should be: if (frame->shouldChangeSelection(selection)) { TextGranularity granularity = CharacterGranularity; if (selection.isRange()) granularity = WordGranularity; frame->selection()->setSelection(selection, granularity); }
Created attachment 51040 [details] Patch
Comment on attachment 51040 [details] Patch This doesn't actually need review. This is the patch with Dave's comments implemented it. Just putting it for review to get it into the EWS. Once it passes EWS, I'll commit.
Committed r56175: <http://trac.webkit.org/changeset/56175>
Comment on attachment 51040 [details] Patch Removed r?