Bug 43778 - Dropping should fire textInput event
Summary: Dropping should fire textInput event
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on: 43893
Blocks: 42957 48791
  Show dependency treegraph
 
Reported: 2010-08-10 02:20 PDT by Hajime Morrita
Modified: 2010-11-01 15:39 PDT (History)
5 users (show)

See Also:


Attachments
Patch (17.73 KB, patch)
2010-08-10 22:17 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (21.96 KB, patch)
2010-08-13 00:36 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (22.17 KB, patch)
2010-08-15 19:26 PDT, Hajime Morrita
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2010-08-10 02:20:55 PDT
Drag-and-drop should result document change, so it should fire textInput event.
Comment 1 Hajime Morrita 2010-08-10 22:17:50 PDT
Created attachment 64070 [details]
Patch
Comment 2 Tony Chang 2010-08-11 10:37:06 PDT
Comment on attachment 64070 [details]
Patch

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +        This chagne:

Nit: typo "change"

> +PassRefPtr<TextEvent> TextEvent::createForDrop(PassRefPtr<AbstractView> view, const String& data)
> +{
> +    return adoptRef(new TextEvent(view, data, 0, false, false, false, true));
> +}

There are too many bools here.  Can you first write a patch to convert these to enums to make it more readable?

> diff --git a/WebCore/page/DragController.cpp b/WebCore/page/DragController.cpp
> @@ -376,6 +386,9 @@ bool DragController::concludeEditDrag(DragData* dragData)
>      Frame* innerFrame = element->ownerDocument()->frame();
>      ASSERT(innerFrame);
>  
> +    if (!dispatchTextInputEventFor(innerFrame, dragData))
> +        return true;

I think it's possible for element to be null here (e.g., if the event handler removed the node from the DOM).  Can you add a test case for this?  We should do the same for the paste event.
Comment 3 Tony Chang 2010-08-11 10:40:58 PDT
> @@ -376,6 +386,9 @@ bool DragController::concludeEditDrag(DragData* dragData)
>      Frame* innerFrame = element->ownerDocument()->frame();
>      ASSERT(innerFrame);
>  
> +    if (!dispatchTextInputEventFor(innerFrame, dragData))
> +        return true;

Oh, Ojan mentions that it might be possible for innerFrame to be null too.  E.g., if you are dropping in an iframe, the event handler might go up to the parent frame and remove the iframe node.  I think this is ok because innerFrame is ref counted, but we should have a test to verify.
Comment 4 Hajime Morrita 2010-08-11 20:23:31 PDT
Hi Tony, thank you for reviewing!

(In reply to comment #2)
> 
> There are too many bools here.  Can you first write a patch to convert these to enums to make it more readable?
I submitted a patch on Bug 43893. Could you review that?
Other fixes will come after that refactoring.
Comment 5 Hajime Morrita 2010-08-13 00:36:28 PDT
Created attachment 64309 [details]
Patch
Comment 6 Hajime Morrita 2010-08-13 00:39:54 PDT
Hi, I updated the patch.

(In reply to comment #2)
> (From update of attachment 64070 [details])
> > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> > +        This chagne:
> 
> Nit: typo "change"
Done.

> 
> > diff --git a/WebCore/page/DragController.cpp b/WebCore/page/DragController.cpp
> > @@ -376,6 +386,9 @@ bool DragController::concludeEditDrag(DragData* dragData)
> >      Frame* innerFrame = element->ownerDocument()->frame();
> >      ASSERT(innerFrame);
> >  
> > +    if (!dispatchTextInputEventFor(innerFrame, dragData))
> > +        return true;
> 
> I think it's possible for element to be null here (e.g., if the event handler removed the node from the DOM).  Can you add a test case for this?  We should do the same for the paste event.
I couldn't figure out make |element| null before dispatchTextInputEventFor().
Added a test that remove an element on the event handler instead.

> Oh, Ojan mentions that it might be possible for innerFrame to be null too.  E.g., if you are dropping in an iframe, the event handler might go up to the parent frame and remove the iframe node.  I think this is ok because innerFrame is ref counted, but we should have a test to verify.
An added test also does against iframe.
Comment 7 Tony Chang 2010-08-14 12:40:32 PDT
Comment on attachment 64309 [details]
Patch

> diff --git a/WebCore/page/DragController.cpp b/WebCore/page/DragController.cpp
> @@ -374,7 +384,9 @@ bool DragController::concludeEditDrag(DragData* dragData)
> -    ASSERT(innerFrame);
> +
> +    if (innerFrame && !dispatchTextInputEventFor(innerFrame, dragData))
> +        return true;

I don't understand this.  How can innerFrame now be null?  If it can be null, it looks like you need to add an innerFrame null check further down.  E.g., if (dragIsMove(innerFrame... needs to check innerFrame.

Other than that, this patch looks good.
Comment 8 Hajime Morrita 2010-08-15 19:26:48 PDT
Created attachment 64462 [details]
Patch
Comment 9 Hajime Morrita 2010-08-15 19:28:28 PDT
Hi Tony, thank you for reviewing again!

> > -    ASSERT(innerFrame);
> > +
> > +    if (innerFrame && !dispatchTextInputEventFor(innerFrame, dragData))
> > +        return true;
> 
> I don't understand this.  How can innerFrame now be null?  If it can be null, it looks like you need to add an innerFrame null check further down.  E.g., if (dragIsMove(innerFrame... needs to check innerFrame.
Agreed. The code in the function looks inconsistent about null checking innerFrame
so I fixed it.
Comment 10 Tony Chang 2010-08-15 23:32:34 PDT
Comment on attachment 64462 [details]
Patch

> diff --git a/WebCore/page/DragController.cpp b/WebCore/page/DragController.cpp
>          if (!color.isValid())
>              return false;
> -        if (!innerFrame)
> -            return false;

I think it's correct to remove this since it's confusing with the ASSERT above.  I wonder if there's some risk that it was needed to avoid a crash (ASSERT doesn't trigger in release builds), but without a test case, I think it's fine to remove.
Comment 11 Hajime Morrita 2010-08-16 00:30:21 PDT
Committed r65395: <http://trac.webkit.org/changeset/65395>
Comment 12 WebKit Review Bot 2010-08-16 01:04:23 PDT
http://trac.webkit.org/changeset/65395 might have broken Qt Linux Release