[webkit-reviews] review granted: [Bug 35314] smartdelete should only occur after double-click : [Attachment 50147] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted Ojan Vafai <ojan at chromium.org>'s
request for review:
Bug 35314: smartdelete should only occur after double-click
https://bugs.webkit.org/show_bug.cgi?id=35314

Attachment 50147: Patch
https://bugs.webkit.org/attachment.cgi?id=50147&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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-contr
ol.js
b/LayoutTests/editing/selection/script-tests/delete-word-granularity-text-contr
ol.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?