Bug 136837 – Videos with controls enabled never receive 'dragstart' events.

• Status: REOPENED
• Component: New Bugs

[Jer Noble]

Videos with controls enabled never receieve 'dragstart' events.

[Jer Noble]

Created attachment 238143 [details]
Patch

[WebKit Commit Bot]

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.

[Simon Fraser (smfr)]

Comment on attachment 238143 [details]
Patch

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());
else
  sourceContainsHitNode = state.source->containsIncludingShadowDOM(hitTestResult.innerNode());

if (! sourceContainsHitNode)
  return false

[Jer Noble]

<rdar://problem/18187713>

[Jer Noble]

Committed r173631: <http://trac.webkit.org/changeset/173631>

[Jer Noble]

Follow up fix committed r173632: <http://trac.webkit.org/changeset/173632>

[Csaba Osztrogonác]

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

http://build.webkit.org/results/Apple%20Mavericks%20Release%20WK1%20%28Tests%29/r173655%20%288592%29/fast/events/mousedown-inside-dragstart-should-not-cause-crash-crash-log.txt

[Jer Noble]

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.

[Jer Noble]

Reopening to attach new patch.

[Jer Noble]

Created attachment 238182 [details]
Follow up patch

[Jer Noble]

Committed r173664: <http://trac.webkit.org/changeset/173664>

[Jer Noble]

Reopening to attach new patch.

[Jer Noble]

Created attachment 238376 [details]
Follow up patch

[WebKit Commit Bot]

Comment on attachment 238376 [details]
Follow up patch

Clearing flags on attachment: 238376

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