We should not bubble up events if we drag something to an iframe that has an invalid source

[Jian Li]

We should not bubble up events if we drag something to an iframe that has an invalid source. FF/IE does not do this and we should mimic the behavior.

[Jian Li]

Created attachment 41343 [details]
Proposed Patch

[Eric Seidel (no email)]

Comment on attachment 41343 [details]
Proposed Patch

Do we have a test which covers the case of a valid source bubbling correctly?

This looks like copy-paste code which should be replaced by a static function, no?

[Jian Li]

Created attachment 41531 [details]
Proposed Patch

Added the test script to cover the case of valid source bubbling. Also introduce a helper function to replace the similar code.

[Dmitry Titov]

Comment on attachment 41531 [details]
Proposed Patch

Almost there.

> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> +        Tests that we don't bubble up the events if we drag something to an
> +        iframe that has an invalid source.

Now that you have a test that verifies both bubbling with valid and invalid targets, this description could change to reflect this.

> +        * http/tests/misc/drag-over-iframe-invalid-source-does-not-bubble-expected.txt: Added.
> +        * http/tests/misc/drag-over-iframe-invalid-source-does-not-bubble.html: Added.

I guess the test name could now also be different. Perhaps "bubble-drag-events-from-iframe.html"?

> diff --git a/LayoutTests/http/tests/misc/drag-over-iframe-invalid-source-does-not-bubble.html b/LayoutTests/http/tests/misc/drag-over-iframe-invalid-source-does-not-bubble.html
> +function log(msg)
> +{
> +    document.getElementById("log").appendChild(document.createTextNode(msg + "\n"));
> +}

Mix of styles - opening '{' of a functions follows different style through the file. Need to stick to one style.

> +    testDragEventBubbling("dragTargetParent", "dragTarget");
> +
> +    log("Tests that we bubble up the events if we drag something to a valid source");
> +    testDragEventBubbling("dragTargetParent2", "dragTarget2");

Perhaps better names for valid/invalid targets could be validDragTarget/validDragTargetParent and invalidDragTarget/invalidDragTartgetParent?

> +
> +<div id="dragTargetParent">
> +<iframe id="dragTarget" src="file:"></iframe>
> +</div>
> +<div contentEditable id="dragTargetParent2">
> +<span id="dragTarget2">Drag here.</span>
> +</div>

In one case it is iframe and in another a span. Should it be both iframes, one with valid url and another with invalid one?
This way, only one variable would change.

> diff --git a/WebCore/page/EventHandler.cpp b/WebCore/page/EventHandler.cpp
> +bool EventHandler::handleDragAndDropForTarget(DragAndDropHandleType type, Node* target, const AtomicString& eventType, const PlatformMouseEvent& event, Clipboard* clipboard)
> +                default:
> +                    ASSERT(false);
> +                    break;

The 'default' can be removed. Now it can not be hit and if someone adds more values to the enum, gcc will issue a warning in compile time, but if there is 'default' it won't.
r- because it seems the valid target in the test should also be iframe.

[Jian Li]

Created attachment 42498 [details]
Proposed Patch

All fixed.

[Dmitry Titov]

Comment on attachment 42498 [details]
Proposed Patch

r=me

[Jian Li]

Committed as http://trac.webkit.org/changeset/50566.