Bug 105041 – Allow disabling drag-and-drop at runtime

• Status: NEW
• Component: New Bugs

[Sadrul Habib Chowdhury]

Allow disabling drag-and-drop at runtime

[Sadrul Habib Chowdhury]

Created attachment 179500 [details]
Patch

[Sadrul Habib Chowdhury]

Hi! The chromium side of this patch is at: https://codereview.chromium.org/11565024/ (and the bug is crbug.com/165206)

Does this look like a reasonable approach for doing this?

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Alexey Proskuryakov]

It is surprising if there are not a few existing ways to do this already.

Is this about all drag and drop, dragging HTML elements around, starting navigation with a drag, or dragging into specific elements, like form file uploads?

[Sadrul Habib Chowdhury]

(In reply to comment #4)
> It is surprising if there are not a few existing ways to do this already.
> 
> Is this about all drag and drop, dragging HTML elements around, starting navigation with a drag, or dragging into specific elements, like form file uploads?

This will disable all drag-n-drop. We don't need a more granular control for drag-n-drop at the moment. Do you think that could be useful?

[Maciej Stachowiak]

Comment on attachment 179500 [details]
Patch

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

r- to address comments above. It might be that this functionality is useful but the motivation is not explained, and the patch doesn't seem to do things in a clean way.

> Source/WebCore/ChangeLog:9
> +        It may be desirable to disallow drag-n-drop for particular pages at runtime. This
> +        change introduces Page::resetDragController() for such cases.

Why would it be desirable?

And isn't it already achievable using regular web platform features, such as setting capturing event listeners on the window for all drag/drop events which prevents default and stops propagation?

> Source/WebCore/page/Page.cpp:245
> +    m_dragController.clear();

Nulling out the drag controller seems like a poor way to represent a "don't allow any drags to start in this page" flag. It's in-band signaling. And it means that any code path that forgets to do the proper check will crash instead of inadvertently doing dragging. An explicit flag would only need to be checked in a few places (ones that can start a drag) instead of in all code paths that can interact with DnD.

[Sadrul Habib Chowdhury]

Thanks for the feedback. Please see response inline:

(In reply to comment #6)
> (From update of attachment 179500 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179500&action=review
> 
> r- to address comments above. It might be that this functionality is useful but the motivation is not explained, and the patch doesn't seem to do things in a clean way.
> 
> > Source/WebCore/ChangeLog:9
> > +        It may be desirable to disallow drag-n-drop for particular pages at runtime. This
> > +        change introduces Page::resetDragController() for such cases.
> 
> Why would it be desirable?
> 
> And isn't it already achievable using regular web platform features, such as setting capturing event listeners on the window for all drag/drop events which prevents default and stops propagation?

Indeed it is. However, the desire here is to disallow the dnd from the embedder, for all web-pages. Specifically in this case, we want to disable dnd in all pages in a browser-plugin (a 'browser plugin' is a special kind of plugin for chromium that allows hosting a web-page in a different process).

> 
> > Source/WebCore/page/Page.cpp:245
> > +    m_dragController.clear();
> 
> Nulling out the drag controller seems like a poor way to represent a "don't allow any drags to start in this page" flag. It's in-band signaling. And it means that any code path that forgets to do the proper check will crash instead of inadvertently doing dragging. An explicit flag would only need to be checked in a few places (ones that can start a drag) instead of in all code paths that can interact with DnD.

Some of the existing code currently checks for a non-NULL Page::dragController() before using it. So I assumed this would be a reasonable check in the rest of the places too. Perhaps my assumption was incorrect?

One other approach I considered was to have the DragController talk to the DragClient more often. To give an example, in order to disable initiating a drag, DragController::draggableNode() would return a null-node (when instructed by the DragClient), etc. Does this sound like a more reasonable approach? Should I write up some code to better explain what I mean?

[Sadrul Habib Chowdhury]

Created attachment 180182 [details]
Patch

[Sadrul Habib Chowdhury]

(In reply to comment #7)
> Thanks for the feedback. Please see response inline:
> 
> (In reply to comment #6)
> > (From update of attachment 179500 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=179500&action=review
> > 
> > r- to address comments above. It might be that this functionality is useful but the motivation is not explained, and the patch doesn't seem to do things in a clean way.
> > 
> > > Source/WebCore/ChangeLog:9
> > > +        It may be desirable to disallow drag-n-drop for particular pages at runtime. This
> > > +        change introduces Page::resetDragController() for such cases.
> > 
> > Why would it be desirable?
> > 
> > And isn't it already achievable using regular web platform features, such as setting capturing event listeners on the window for all drag/drop events which prevents default and stops propagation?
> 
> Indeed it is. However, the desire here is to disallow the dnd from the embedder, for all web-pages. Specifically in this case, we want to disable dnd in all pages in a browser-plugin (a 'browser plugin' is a special kind of plugin for chromium that allows hosting a web-page in a different process).
> 
> > 
> > > Source/WebCore/page/Page.cpp:245
> > > +    m_dragController.clear();
> > 
> > Nulling out the drag controller seems like a poor way to represent a "don't allow any drags to start in this page" flag. It's in-band signaling. And it means that any code path that forgets to do the proper check will crash instead of inadvertently doing dragging. An explicit flag would only need to be checked in a few places (ones that can start a drag) instead of in all code paths that can interact with DnD.

The new patch adds an entry in WebCore::Settings, and it is indeed much simpler and cleaner. Thanks! Does this look reasonable? (I will add tests if this is [ closer to being ] the correct way of doing this)

[Brady Eidson]

(In reply to comment #7)
> > > Source/WebCore/page/Page.cpp:245
> > > +    m_dragController.clear();
> > 
> > Nulling out the drag controller seems like a poor way to represent a "don't allow any drags to start in this page" flag. It's in-band signaling. And it means that any code path that forgets to do the proper check will crash instead of inadvertently doing dragging. An explicit flag would only need to be checked in a few places (ones that can start a drag) instead of in all code paths that can interact with DnD.
> 
> Some of the existing code currently checks for a non-NULL Page::dragController() before using it. So I assumed this would be a reasonable check in the rest of the places too. Perhaps my assumption was incorrect?

A Page always has a dragController.  The sites that null check it are either wrong, or leftover from a time when a page didn't always have a drag controller.

We should not be clearing it out.

[Maciej Stachowiak]

(In reply to comment #7)
> Thanks for the feedback. Please see response inline:
> 
> (In reply to comment #6)
> > (From update of attachment 179500 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=179500&action=review
> > 
> > r- to address comments above. It might be that this functionality is useful but the motivation is not explained, and the patch doesn't seem to do things in a clean way.
> > 
> > > Source/WebCore/ChangeLog:9
> > > +        It may be desirable to disallow drag-n-drop for particular pages at runtime. This
> > > +        change introduces Page::resetDragController() for such cases.
> > 
> > Why would it be desirable?

Doesn't seem like you answered this.

> > 
> > And isn't it already achievable using regular web platform features, such as setting capturing event listeners on the window for all drag/drop events which prevents default and stops propagation?
> 
> Indeed it is. However, the desire here is to disallow the dnd from the embedder, for all web-pages. Specifically in this case, we want to disable dnd in all pages in a browser-plugin (a 'browser plugin' is a special kind of plugin for chromium that allows hosting a web-page in a different process).

What I'm saying is, I believe it's already possible of ran embedder to accomplish what is described here, with no WebCore changes, by attaching appropriate event listeners from the embedder level. Have you tried that? Is there a reason changing WebCore is better?

(In reply to comment #9)
> 
> The new patch adds an entry in WebCore::Settings, and it is indeed much simpler and cleaner. Thanks! Does this look reasonable? (I will add tests if this is [ closer to being ] the correct way of doing this)

That does look much cleaner.

[Sadrul Habib Chowdhury]

Comment on attachment 180182 [details]
Patch

> What I'm saying is, I believe it's already possible of ran embedder
> to accomplish what is described here, with no WebCore changes, by
> attaching appropriate event listeners from the embedder level. Have
> you tried that? Is there a reason changing WebCore is better?

We went with the fix in the embedder as you suggested here. That works,
without having to change anything in WebCore. Thanks for the suggestion!
(and sorry about the delay in getting back to this)