Bug 38175 – REGRESSION (r48698): Selection drag-and-drop doesn't work for input/textarea

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Daniel Cheng]

In http://crbug.com/new, attempt to drag some text from a TEXTAREA to an empty text input. It will fail, and will raise an assert in debug builds:
ASSERT(oldLength >= selectionLength);

I think this is because WebCore is calculating oldLength from the wrong element; it looks like it's calculating the length of the text field being dragged to rather than the text field being dragged from. However, I'm not familiar enough with this code to say for sure.

[Kent Tamura]

Created attachment 54411 [details]
Patch

[Darin Adler]

Comment on attachment 54411 [details]
Patch

In what way is this "incorrect arithmetic"? Can we re-title the bug to be clearer?

I don't understand the comment "No need to calculate the selection length in a case of selection drag-and-drop."
- The comment doesn't say why focused() is a reliable way to detect the fact that selected text is being dragged within the element.
- The comment doesn't say why setting selectionLength to 0 is a good way to bypass the selection check entirely.
The main purpose of comments is to explain why the code exists. The code can speak for itself on *what* it does.

I am not convinced that setting selectionLength to 0 is the best way to handle this.

[Kent Tamura]

Created attachment 54498 [details]
Patch (rev.2)

This seems to be a regression from Safari 4.0.5. Any idea what caused it? I am asking because whatever it was may have had other side effects that your patch will not address.

[Kent Tamura]

Thank you for reviewing

(In reply to comment #2)
> I don't understand the comment "No need to calculate the selection length in a
> case of selection drag-and-drop."
> - The comment doesn't say why focused() is a reliable way to detect the fact
> that selected text is being dragged within the element.
> - The comment doesn't say why setting selectionLength to 0 is a good way to
> bypass the selection check entirely.
> The main purpose of comments is to explain why the code exists. The code can
> speak for itself on *what* it does.
> 
> I am not convinced that setting selectionLength to 0 is the best way to handle
> this.

I updated the comments. I think the new comments explain them.

And I removed the existing comment "// selection() may be a pre-edit text.".  I added it when I fixed an issue for IME, but the comment is not helpful for now though it was helpful during the IME change.

Caused by <http://trac.webkit.org/changeset/48698>.

<rdar://problem/7915903>

[Kent Tamura]

(In reply to comment #6)
> Caused by <http://trac.webkit.org/changeset/48698>.

Or r48011.

The assertion has been there for a long time. The code have never considered
text drag-and-drop, but it worked accidentally.  It seems my change exposed the
problem.

[Kent Tamura]

Landed as r58502.

[WebKit Review Bot]

http://trac.webkit.org/changeset/58502 might have broken GTK Linux 32-bit Release

[Kent Tamura]

(In reply to comment #10)
> http://trac.webkit.org/changeset/58502 might have broken GTK Linux 32-bit
> Release

I'll add the test to the skipped list for GTK and Qt.