Summary: | We should not bubble up events if we drag something to an iframe that has an invalid source | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jian Li <jianli> | ||||||||
Component: | WebCore JavaScript | Assignee: | Jian Li <jianli> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | ||||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Jian Li
2009-10-16 17:29:38 PDT
Created attachment 41343 [details]
Proposed Patch
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?
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.
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. Created attachment 42498 [details]
Proposed Patch
All fixed.
Comment on attachment 42498 [details]
Proposed Patch
r=me
Committed as http://trac.webkit.org/changeset/50566. |