Bug 34305 – Drag and Drop: Windows uses "stop" sign as cursor when dragging

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Brian Weinstein]

Drag and Drop: Windows uses "stop" sign as cursor when dragging

[Brian Weinstein]

Created attachment 47672 [details]
Patch

[Brian Weinstein]

Created attachment 47722 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 47722 [details]
Patch

> +    RECT webViewRect;
> +    HWND viewWindow;
> +    if (FAILED(m_webView->viewWindow((OLE_HANDLE*)&viewWindow)))
> +        return DRAGDROP_S_USEDEFAULTCURSORS;
> +    ::GetWindowRect(viewWindow, &webViewRect);

reinterpret_cast would be better than the C-style cast.

You should wait to declare webViewRect until just before you call GetWindowRect.

> +    if (point.x() > (webViewRect.right - webViewRect.left) || point.x() < 0 || point.y() > (webViewRect.bottom - webViewRect.top) || point.y() < 0) {
> +        // If our cursor is outside the bounds of the webView, we want to let Windows select the cursor.
> +        return DRAGDROP_S_USEDEFAULTCURSORS;
> +    }

Comparing an X coordinate to the width, and a Y coordinate to the height, only makes sense if you are assuming that the rect has its origin at 0,0. But even then you might as well compare the X coordinate to the left and right edges of the window, and the Y coordinate to the top and bottom edges of the window. Comparing to the width and height just makes the code more fragile and less clear.

In this case, the rect *does not* have its origin at 0,0, so this code is wrong. GetWindowRect returns a rect in screen coordinates. I would rewrite this like so:

RECT webViewRect;
GetWindowRect(&webViewRect);

POINT point;
GetCursorPos(&point);

if (!IntRect(webViewRect).contains(point)) {

> +    FrameView* view = m_webView->page()->mainFrame()->view();
> +    if (view) {

You should make this an early return, just like all the other ifs in this function.

> +    virtual HRESULT STDMETHODCALLTYPE setShowCustomDragCursors(BOOL);
> +    virtual HRESULT STDMETHODCALLTYPE showCustomDragCursors(BOOL*);

This name makes me think that when I call setShowCustomDragCursors(TRUE), suddenly custom cursors will appear on my screen. I think putting "Enabled" in the name of these functions would fix this. Something like [set]CustomDragCursorsEnabled. (You should change the preference key to match.)

[Brian Weinstein]

(In reply to comment #3)
> (From update of attachment 47722 [details])
> > +    RECT webViewRect;
> > +    HWND viewWindow;
> > +    if (FAILED(m_webView->viewWindow((OLE_HANDLE*)&viewWindow)))
> > +        return DRAGDROP_S_USEDEFAULTCURSORS;
> > +    ::GetWindowRect(viewWindow, &webViewRect);
> 
> reinterpret_cast would be better than the C-style cast.
> 
> You should wait to declare webViewRect until just before you call
> GetWindowRect.
> 

Will do, got this code from a combination of WebDropSource and WebView, so I'll clean it up.

> > +    if (point.x() > (webViewRect.right - webViewRect.left) || point.x() < 0 || point.y() > (webViewRect.bottom - webViewRect.top) || point.y() < 0) {
> > +        // If our cursor is outside the bounds of the webView, we want to let Windows select the cursor.
> > +        return DRAGDROP_S_USEDEFAULTCURSORS;
> > +    }
> 
> Comparing an X coordinate to the width, and a Y coordinate to the height, only
> makes sense if you are assuming that the rect has its origin at 0,0. But even
> then you might as well compare the X coordinate to the left and right edges of
> the window, and the Y coordinate to the top and bottom edges of the window.
> Comparing to the width and height just makes the code more fragile and less
> clear.
> 
> In this case, the rect *does not* have its origin at 0,0, so this code is
> wrong. GetWindowRect returns a rect in screen coordinates. I would rewrite this
> like so:
> 
> RECT webViewRect;
> GetWindowRect(&webViewRect);
> 
> POINT point;
> GetCursorPos(&point);
> 
> if (!IntRect(webViewRect).contains(point)) {

That's a lot cleaner, I'll fix.

> 
> > +    FrameView* view = m_webView->page()->mainFrame()->view();
> > +    if (view) {
> 
> You should make this an early return, just like all the other ifs in this
> function.

Will do.

> 
> > +    virtual HRESULT STDMETHODCALLTYPE setShowCustomDragCursors(BOOL);
> > +    virtual HRESULT STDMETHODCALLTYPE showCustomDragCursors(BOOL*);
> 
> This name makes me think that when I call setShowCustomDragCursors(TRUE),
> suddenly custom cursors will appear on my screen. I think putting "Enabled" in
> the name of these functions would fix this. Something like
> [set]CustomDragCursorsEnabled. (You should change the preference key to match.)

I think that makes more sense. 

A new patch will be out shortly, thanks for the review!

[Brian Weinstein]

Created attachment 47738 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 47738 [details]
Patch

> +    if (!IntRect(webViewRect).contains(cursorPoint)) {

You could actually use PtInRect here instead.

r=me

[Eric Seidel (no email)]

Attachment 47738 [details] was posted by a committer and has review+, assigning to Brian Weinstein for commit.

[Brian Weinstein]

Sorry, landed in r54083.