Bug 35314 – smartdelete should only occur after double-click

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ojan Vafai]

Currently it also happens if you do a word-selection via cmd+option+left/right.

[Ojan Vafai]

From webkit-dev:
On Tue, Feb 23, 2010 at 1:49 PM, Darin Adler <[e-mail]> 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?

[Darin Adler]

Yes, eventSender would work, but a test that also works in the browser, not just in DumpRenderTree, would be preferred.

[Ojan Vafai]

Created attachment 50066 [details]
Patch

[Eric Seidel (no email)]

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. :)

[Ojan Vafai]

Created attachment 50147 [details]
Patch

[Ojan Vafai]

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.

[David Levin]

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?

[Alexey Proskuryakov]

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

[Ojan Vafai]

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

[chris fleizach]

(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

[Alexey Proskuryakov]

Using keyboard selection seems to mean that smart delete won't work with VoiceOver any more.

[Ojan Vafai]

(In reply to comment #11)
> Using keyboard selection seems to mean that smart delete won't work with
> VoiceOver any more.

Should it?

[Ojan Vafai]

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

[Ojan Vafai]

Committed r55913: <http://trac.webkit.org/changeset/55913>

[Ojan Vafai]

Created attachment 50616 [details]
Patch

[Eric Seidel (no email)]

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.

[David Levin]

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
> [phone]  Ojan Vafai  <[e-mail]>
> +
> +        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);
      }

[Ojan Vafai]

Created attachment 51040 [details]
Patch

[Ojan Vafai]

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.

[Ojan Vafai]

Committed r56175: <http://trac.webkit.org/changeset/56175>

[David Levin]

Comment on attachment 51040 [details]
Patch

Removed r?