Bug 107998 – Drag and drop events should be forwarded to HTML embedded with object tag.

• Component: UI Events
• Status: ASSIGNED

[Daniel Cheng]

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.

[Daniel Cheng]

Created attachment 184900 [details]
Patch

[WebKit Review Bot]

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 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]

(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]

> 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]

(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 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]

Created attachment 185087 [details]
Patch

[Alexey Proskuryakov]

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]

(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