Bug 136837 - Videos with controls enabled never receive 'dragstart' events.
Summary: Videos with controls enabled never receive 'dragstart' events.
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
Keywords: InRadar
Depends on: 136946
  Show dependency treegraph
Reported: 2014-09-15 14:24 PDT by Jer Noble
Modified: 2015-01-26 08:43 PST (History)
3 users (show)

See Also:

Patch (5.11 KB, patch)
2014-09-15 14:30 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Follow up patch (1.68 KB, patch)
2014-09-16 08:59 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Follow up patch (2.36 KB, patch)
2014-09-19 09:16 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2014-09-15 14:24:17 PDT
Videos with controls enabled never receieve 'dragstart' events.
Comment 1 Jer Noble 2014-09-15 14:30:01 PDT
Created attachment 238143 [details]
Comment 2 WebKit Commit Bot 2014-09-15 14:32:16 PDT
Attachment 238143 [details] did not pass style-queue:

ERROR: Source/WebCore/page/DragController.cpp:724:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 4 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Simon Fraser (smfr) 2014-09-15 14:36:34 PDT
Comment on attachment 238143 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=238143&action=review

> Source/WebCore/page/DragController.cpp:732
> +    if (!state.source->isMediaElement() && !state.source->contains(hitTestResult.innerNode()))
>          // The original node being dragged isn't under the drag origin anymore... maybe it was
>          // hidden or moved out from under the cursor. Regardless, we don't want to start a drag on
>          // something that's not actually under the drag origin.
> +        // FIXME(136836): Investigate whether all elements should use the containsIncludingShadowDOM() path here.
>          return false;
> +
> +    if (state.source->isMediaElement() && !state.source->containsIncludingShadowDOM(hitTestResult.innerNode()))
> +        return false;

I think this would be cleaner as:

if (!state.source->isMediaElement()) {
  if (!state.source->contains(hitTestResult.innerNode()))
} else {
  if (!state.source->containsIncludingShadowDOM(hitTestResult.innerNode()))

Or better:
bool includeShadowDOM = state.source->isMediaElement();
bool sourceContainsHitNode = false;
if (!includeShadowDOM)
  sourceContainsHitNode = state.source->contains(hitTestResult.innerNode());
  sourceContainsHitNode = state.source->containsIncludingShadowDOM(hitTestResult.innerNode());

if (! sourceContainsHitNode)
  return false
Comment 4 Jer Noble 2014-09-15 14:44:16 PDT
Comment 5 Jer Noble 2014-09-15 14:47:33 PDT
Committed r173631: <http://trac.webkit.org/changeset/173631>
Comment 6 Jer Noble 2014-09-15 14:52:49 PDT
Follow up fix committed r173632: <http://trac.webkit.org/changeset/173632>
Comment 7 Csaba Osztrogonác 2014-09-16 02:19:35 PDT
(In reply to comment #5)
> Committed r173631: <http://trac.webkit.org/changeset/173631>

It broke fast/events/mousedown-inside-dragstart-should-not-cause-crash.html
on all Apple Mac bots, as the EWS noticed it before landing:

Comment 8 Jer Noble 2014-09-16 08:52:18 PDT
Wow, Node::contains() will return FALSE if `this` is NULL.  So previously, `state.source->contains(hitTestResult.innerNode())` would return FALSE if `state.source` NULL. Now that we're calling a virtual method, a null-dereference crash occurs.

The fact that this ever passed is entirely accidental.

I'll fix this in a follow-up patch.
Comment 9 Jer Noble 2014-09-16 08:59:29 PDT
Reopening to attach new patch.
Comment 10 Jer Noble 2014-09-16 08:59:32 PDT
Created attachment 238182 [details]
Follow up patch
Comment 11 Jer Noble 2014-09-16 11:09:41 PDT
Committed r173664: <http://trac.webkit.org/changeset/173664>
Comment 12 Jer Noble 2014-09-19 09:16:37 PDT
Reopening to attach new patch.
Comment 13 Jer Noble 2014-09-19 09:16:39 PDT
Created attachment 238376 [details]
Follow up patch
Comment 14 WebKit Commit Bot 2014-09-19 11:15:18 PDT
Comment on attachment 238376 [details]
Follow up patch

Clearing flags on attachment: 238376

Committed r173764: <http://trac.webkit.org/changeset/173764>