Bug 107998

Summary: Drag and drop events should be forwarded to HTML embedded with object tag.
Product: WebKit Reporter: Daniel Cheng <dcheng>
Component: UI EventsAssignee: Daniel Cheng <dcheng>
Status: ASSIGNED    
Severity: Normal CC: alex, ap, dglazkov, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch ap: review-

Daniel Cheng
Reported 2013-01-25 18:56:35 PST
Reported in http://crbug.com/171472. I've written a local patch that addresses this issue and will upload it once I have a layout test.
Attachments
Patch (6.24 KB, patch)
2013-01-26 22:10 PST, Daniel Cheng
no flags
Patch (6.66 KB, patch)
2013-01-28 15:50 PST, Daniel Cheng
ap: review-
Daniel Cheng
Comment 1 2013-01-26 22:10:56 PST
WebKit Review Bot
Comment 2 2013-01-26 22:57:47 PST
Comment on attachment 184900 [details] Patch Attachment 184900 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16154105 New failing tests: http/tests/misc/bubble-drag-events.html
Alexey Proskuryakov
Comment 3 2013-01-26 23:16:13 PST
Comment on attachment 184900 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184900&action=review r- because there is no rationale posted in Bugzilla, and because this introduces a security bug. > Source/WebCore/page/EventHandler.cpp:1912 > + if (!target->hasTagName(frameTag) && !target->hasTagName(iframeTag) && !target->hasTagName(objectTag)) > return false; > > frame = static_cast<HTMLFrameElementBase*>(target)->contentFrame(); This introduces a security bug due to a bad cast. > Source/WebCore/page/EventHandler.cpp:1914 > + return (frame); This is not normal WebKit style.
Daniel Cheng
Comment 4 2013-01-27 10:13:03 PST
(In reply to comment #3) > (From update of attachment 184900 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184900&action=review > > r- because there is no rationale posted in Bugzilla, The rationale is that if you embed a HTML page with <object>, it behaves like an <iframe>. However, while an <iframe> receives dragging events, a HTML page embedded with <object> will not. > and because this introduces a security bug. > > > Source/WebCore/page/EventHandler.cpp:1912 > > + if (!target->hasTagName(frameTag) && !target->hasTagName(iframeTag) && !target->hasTagName(objectTag)) > > return false; > > > > frame = static_cast<HTMLFrameElementBase*>(target)->contentFrame(); > > This introduces a security bug due to a bad cast. > Ack, thanks for catching this. > > Source/WebCore/page/EventHandler.cpp:1914 > > + return (frame); > > This is not normal WebKit style. I wanted to write frame != 0 to make the bool conversion explicit, but the webkit linter didn't like that. Would you consider !! acceptable instead?
Alexey Proskuryakov
Comment 5 2013-01-27 19:05:56 PST
> The rationale is that if you embed a HTML page with <object>, it behaves like an <iframe>. However, while an <iframe> receives dragging events, a HTML page embedded with <object> will not. So you are actually saying that an HTML page does _not_ behave the same in <object> as it does in <iframe>, and you'd like to change that. Why? Does HTML5 say so, do other browsers agree, are there any known pages that would behave better with this fix? > I wanted to write frame != 0 to make the bool conversion explicit, but the webkit linter didn't like that. Would you consider !! acceptable instead? No. WebKit style is "return frame".
Daniel Cheng
Comment 6 2013-01-28 11:52:26 PST
(In reply to comment #5) > > The rationale is that if you embed a HTML page with <object>, it behaves like an <iframe>. However, while an <iframe> receives dragging events, a HTML page embedded with <object> will not. > > So you are actually saying that an HTML page does _not_ behave the same in <object> as it does in <iframe>, and you'd like to change that. Why? Does HTML5 say so, do other browsers agree, are there any known pages that would behave better with this fix? IE and Firefox are both fowarding DnD events to nested browsing contexts loaded via <object> tags correctly. I don't see anything in particular in the spec about forwarding events to nested browsing contexts in general; however, given that other keyboard/mouse interactions behave as one might expect in a nested <object> browsing context, it seems consistent to make DnD follow the same model. > > > I wanted to write frame != 0 to make the bool conversion explicit, but the webkit linter didn't like that. Would you consider !! acceptable instead? > > No. WebKit style is "return frame".
Adam Barth
Comment 7 2013-01-28 15:29:38 PST
Comment on attachment 184900 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184900&action=review >>> Source/WebCore/page/EventHandler.cpp:1912 >>> frame = static_cast<HTMLFrameElementBase*>(target)->contentFrame(); >> >> This introduces a security bug due to a bad cast. > > Ack, thanks for catching this. Perhaps we should cast to HTMLFrameOwnerElement instead.
Daniel Cheng
Comment 8 2013-01-28 15:50:20 PST
Alexey Proskuryakov
Comment 9 2013-01-28 19:04:27 PST
Comment on attachment 185087 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185087&action=review Compatibility with IE and Firefox seems to be a decent reason for this. Did you consider contacting WhatWG to make this explicit in spec? It's not OK to return true from a function named "targetIsFrame" when target is not frame. Since this is a theoretical issue that is not known to affect sites, this is very low priority, and I will not try to come up with suggestions. Please make a patch that does not degrade WebKit code quality by making functions things that are different from what they say. > Source/WebCore/page/EventHandler.cpp:1918 > + // For <object>, we only consider it a valid frame to forward DnD events to if frame is > + // non-null. Unfortunately, frame will also be null if <object> has an invalid source; in that "targetIsFrame" is a generic function name, so it's not right to explain its behavior by drag and drop requirements. If it's tailored to drag and drop only, its name should say so. Also, "DnD" is not the kind of abbreviation that is welcome in WebKit code base.
Alexander McCabe
Comment 10 2013-05-22 21:54:24 PDT
(In reply to comment #9) > (From update of attachment 185087 [details]) > Since this is a theoretical issue that is not known to affect sites Do I understand correctly that this comment refers to the bug overall? If so, this is incorrect. The Moodle LMS uses the object tag to display all SCORM content, so much DnD activity is broken in this content. Here's some reports from users that are being affected by this bug. http://www.questionwriter.com/qwforum/read1-5100.html http://www.questionwriter.com/qwforum/read1-4837.html
Note You need to log in before you can comment on or make changes to this bug.