Bug 43778 – Dropping should fire textInput event

• Component: DOM
• Status: RESOLVED FIXED

[Hajime Morrita]

Drag-and-drop should result document change, so it should fire textInput event.

[Hajime Morrita]

Created attachment 64070 [details]
Patch

[Tony Chang]

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.

[Tony Chang]

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

[Hajime Morrita]

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.

[Hajime Morrita]

Created attachment 64309 [details]
Patch

[Hajime Morrita]

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.

[Tony Chang]

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.

[Hajime Morrita]

Created attachment 64462 [details]
Patch

[Hajime Morrita]

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.

[Tony Chang]

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.

[Hajime Morrita]

Committed r65395: <http://trac.webkit.org/changeset/65395>

[WebKit Review Bot]

http://trac.webkit.org/changeset/65395 might have broken Qt Linux Release