[CSS 3] missing cursor support for 'none' on Windows

• Component: CSS
• Status: UNCONFIRMED ---

[Ryan]

Created attachment 88020 [details]
Minimal test based on manual-tests/cursor.html

The CSS cursor selector 'none' does not work on Windows, the cursor remains an arrow.

[Ryan]

Created attachment 88031 [details]
Proposed patch

This patch allows a native cursor handle to be null on Windows, and creates such a cursor for the Cursor::None type. This gives the desired behavior for the CSS property 'cursor: none'.

[WebKit Review Bot]

Attachment 88031 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1

Source/WebKit/win/WebCoreSupport/WebChromeClient.cpp:799:  Use 0 or null instead of NULL (even in *comments*).  [readability/null] [4]
Source/WebKit/win/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

Comment on attachment 88031 [details]
Proposed patch

Thanks for working on this! You should address the style bot's complaints, because they seem valid.

Also, it would be great to fix this for WebKit2 at the same time. The relevant code is in Source/WebKit2/UIProcess/win/WebView.cpp. You can test your changes using MiniBrowser.

[Ryan]

Created attachment 88581 [details]
Updated patch for code style

Fixed code style.

I am having trouble running MiniBrowser on Windows - running the run-minibrowser script from cygwin seems to do nothing, and running the .exe directly from the build directory results in an error saying that CFNetwork.dll can't be found.

I would be happy to fix WebKit2 also, is there something I'm missing in getting the MiniBrowser to run?

[Adam Roben (:aroben)]

(In reply to comment #4)
> I am having trouble running MiniBrowser on Windows - running the run-minibrowser script from cygwin seems to do nothing, and running the .exe directly from the build directory results in an error saying that CFNetwork.dll can't be found.

It looks like run-minibrowser hasn't been implemented for Windows. It would be great to make that work!

> I would be happy to fix WebKit2 also, is there something I'm missing in getting the MiniBrowser to run?

If you add %COMMONGPROGRAMFILES%\Apple\Apple Application Support to your PATH, MiniBrowser should launch just fine.

[Ryan]

Created attachment 88773 [details]
Updated patch including WebKit2

This patch also includes a fix for cursor: none in WebKit2.

[Ryan]

(In reply to comment #5)
> It looks like run-minibrowser hasn't been implemented for Windows. It would be great to make that work!

Should this take place under a different bug?

> If you add %COMMONGPROGRAMFILES%\Apple\Apple Application Support to your PATH, MiniBrowser should launch just fine.

Thanks, this worked perfectly! (should have thought to just search my hard drive...)

[Adam Roben (:aroben)]

(In reply to comment #7)
> (In reply to comment #5)
> > It looks like run-minibrowser hasn't been implemented for Windows. It would be great to make that work!
> 
> Should this take place under a different bug?

Yes, please.

> > If you add %COMMONGPROGRAMFILES%\Apple\Apple Application Support to your PATH, MiniBrowser should launch just fine.
> 
> Thanks, this worked perfectly! (should have thought to just search my hard drive...)

Great!

[Adam Roben (:aroben)]

Comment on attachment 88773 [details]
Updated patch including WebKit2

Have you tested that this change does not reintroduce bug 45692 or bug 52024?

I'm surprised that you were able to fix this for WebKit2 without touching WebView::onSetCursor.

Is there a way we can assert that if we're calling ::SetCursor(0) it's because Cursor::None is being used? That would help us catch mistakes where we're calling ::SetCursor(0) for some incorrect reason.

I don't believe you're a committer, so you should probably mark your patches cq?. That will get them landed once they've been reviewed.

[Maciej Stachowiak]

Comment on attachment 88773 [details]
Updated patch including WebKit2

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

See Adam's suggestions in a previous comment. Further comments below. Please revise and resubmit.

> Source/WebKit2/UIProcess/win/WebView.cpp:1011
> +    // It's ok if m_lastCursorSet is null:
> +    // that's how Cursor::None is represented.

This comment seems unnecessary, you don't have to explain the lack of a special case.

> Source/WebKit2/UIProcess/win/WebView.cpp:1020
> +    // It's ok if m_webCoreCursor is null:
> +    // that's how Cursor::None is represented.

Also here. Particularly, having the comment a second time seems unnecessary,

> Source/WebKit/win/WebCoreSupport/WebChromeClient.cpp:800
> +    // It's ok if platformCursor is null:
> +    // that's how Cursor::None is represented.

And here too, I would drop the comment.

[Ryan]

(In reply to comment #9)
> (From update of attachment 88773 [details])
> Have you tested that this change does not reintroduce bug 45692 or bug 52024?

Yes, I cannot reproduce those bugs with the patch.

> I'm surprised that you were able to fix this for WebKit2 without touching WebView::onSetCursor.

Ah, I may still have to change it - I noticed some flickering of the cursor while it is set to 'none' with my patch. Not sure if it is related, I will investigate further.
 
> Is there a way we can assert that if we're calling ::SetCursor(0) it's because Cursor::None is being used? That would help us catch mistakes where we're calling ::SetCursor(0) for some incorrect reason.

This should be straightforward for WebKit, but less so for WebKit2 for at least two reasons:

1. The WebView class stores only HCURSORs, so it doesn't have any information about the intended type of the cursor. Would be acceptable to change the HCURSOR members to WebCore::Cursors?

2. The overrideCursor only seems to be set with an HCURSOR, so it also has no information about its intended cursor type.

> I don't believe you're a committer, so you should probably mark your patches cq?. That will get them landed once they've been reviewed.

That's correct, I'm not a committer, but I am new to WebKit development! I'll add the flag from now on. Thanks!

[Adam Roben (:aroben)]

(In reply to comment #11)
> (In reply to comment #9)
> > (From update of attachment 88773 [details] [details])
> > Is there a way we can assert that if we're calling ::SetCursor(0) it's because Cursor::None is being used? That would help us catch mistakes where we're calling ::SetCursor(0) for some incorrect reason.
> 
> This should be straightforward for WebKit, but less so for WebKit2 for at least two reasons:
> 
> 1. The WebView class stores only HCURSORs, so it doesn't have any information about the intended type of the cursor. Would be acceptable to change the HCURSOR members to WebCore::Cursors?

Yes.

> 2. The overrideCursor only seems to be set with an HCURSOR, so it also has no information about its intended cursor type.

setOverrideCursor(0) means not to use an override cursor at all, so that shouldn't be a problem for the assertion.

[Ryan]

Created attachment 104642 [details]
Revised patch addressing comments.

Updated patch to address comments. It was simple to add an ASSERT for WebKit, but I ended up not doing an ASSERT for WebKit2 for the following reasons:
1. Not asserting and just returning without setting the cursor was closer to how the code worked before this patch, so it seems less risky.
2. I noticed that WebView::cursorToShow returns 0 if the page isn't valid, which may be the case even if the WebCore cursor's Type isn't None. I figured it would be better to continue with DefWindowProc's cursor handling rather than asserting.

If these are bad reasons, I'll be happy to update the patch.

With this method of implementing cursor: none, the convention of a null override cursor meaning "don't use an override" won't work if the override cursor itself ever needs to be 'none'. Is this a case that we need to handle?

The flickering I was seeing was happening because in some cases the DefWindowProc was being called even though the WM_SETCURSOR message was being handled. DefWindowProc handles WM_SETCURSOR by setting the cursor to the window class' cursor, which for the WebView is an arrow. Since the WebView always calls ::SetCursor when it gets a WM_SETCURSOR message, it should never need to call DefWindowProc in this case.

[Adam Roben (:aroben)]

(In reply to comment #13)
> Created an attachment (id=104642) [details]
> Revised patch addressing comments.
> 
> Updated patch to address comments. It was simple to add an ASSERT for WebKit, but I ended up not doing an ASSERT for WebKit2 for the following reasons:
> 1. Not asserting and just returning without setting the cursor was closer to how the code worked before this patch, so it seems less risky.
> 2. I noticed that WebView::cursorToShow returns 0 if the page isn't valid, which may be the case even if the WebCore cursor's Type isn't None. I figured it would be better to continue with DefWindowProc's cursor handling rather than asserting.

I think it would still be good to assert, even if we also bail out in Release builds. We'll just have to take the page's validity into account.

> With this method of implementing cursor: none, the convention of a null override cursor meaning "don't use an override" won't work if the override cursor itself ever needs to be 'none'. Is this a case that we need to handle?

I don't think we need to handle that case currently. (This is already something we don't support.)

[Adam Roben (:aroben)]

Comment on attachment 104642 [details]
Revised patch addressing comments.

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

> Source/WebKit2/UIProcess/win/WebView.cpp:800
> +    return handled? TRUE: FALSE;

WebKit coding style says to put spaces on both sides of ? and :, like this: return handled ? TRUE : FALSE;

> Source/WebKit2/UIProcess/win/WebView.cpp:1033
> +    static const HCURSOR arrowCursor = ::LoadCursor(0, IDC_ARROW);
> +    const HCURSOR webCoreNativeCursor = m_webCoreCursor.platformCursor()->nativeCursor();

We tend not to use "const" in this manner. We normally only used const for objects, not pointers. (I.e., we'll say "const int* foo" but not "int* const foo", which is essentially what "const HCURSOR" means.) So I'd remove the "const"s here.

> Source/WebKit2/UIProcess/win/WebView.cpp:1047
> +    // Only remove the cursor from the screen (by passing a null parameter
> +    // to ::SetCursor) if the WebCore cursor is of type None.
> +    if (m_lastCursorSet && m_webCoreCursor.type() != Cursor::None)
> +        return false;

Since we know the exact conditions in which we expect to get a null HCURSOR, I think we should assert about it in addition to this bail out:

ASSERT(m_lastCursorSet || m_webCoreCursor.type() == Cursor::None || !m_page->isValid());
if (m_lastCursorSet && m_webCoreCursor.type() != Cursor::None)
    return false;

> Source/WebKit/win/WebCoreSupport/WebChromeClient.cpp:808
> +        ASSERT(platformCursor || (!platformCursor && (cursor.type() == Cursor::None)));

This can be simplified to: ASSERT(platformCursor || cursor.type() == Cursor::None)

[Ryan]

Created attachment 109409 [details]
Updated patch addressing latest comments.

I've addressed the comments, including removing the const qualifiers.

This seems odd to me, considering the current general opinion on C++ best practices is to use const whenever possible. I can understand wanting to keep the code consistent, but in the case of consts, is it really worth it considering the bugs const-correctness can help avoid?

[Ryan]

Created attachment 109410 [details]
Fixed patch, previous update was missing a ChangeLog

[Eric Seidel (no email)]

Adam roben is your man here.  This patch looks rather forgotten. :(

[Adam Roben (:aroben)]

Comment on attachment 109410 [details]
Fixed patch, previous update was missing a ChangeLog

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

Sorry for the ridiculously long delay in reviewing this! I think you're really close!

> Source/WebKit2/UIProcess/win/WebView.cpp:1048
> +    // Only remove the cursor from the screen (by passing a null parameter
> +    // to ::SetCursor) if the WebCore cursor is of type None.
> +    ASSERT(m_lastCursorSet || m_webCoreCursor.type() == Cursor::None || !m_page->isValid());
> +    if (m_lastCursorSet && m_webCoreCursor.type() != Cursor::None)
> +        return false;

The code doesn't seem to match the comment. Code that matches the comment would be:
ASSERT(m_lastCursorSet || m_webCoreCursor.type() == Cursor::None || !m_page->isValid());
if (!m_lastCursorSet && m_webCoreCursor.type() != Cursor::None)
    return false;

Looks like we can get rid of m_lastCursorSet entirely now. It's only used within this function!

I think this would all be a bit clearer if this function bailed out immediately at the start if the page isn't valid. So something like this:

if (!m_page->isValid())
    return false;

HCURSOR cursor = cursorToShow();
ASSERT(!cursor == (m_webCoreCursor.type() == Cursor::None));
::SetCursor(cursor);
return true;

> Source/WebKit/win/WebCoreSupport/WebChromeClient.cpp:808
> +        // A null platformCursor should only be used to represent Cursor::None.
> +        // Catch other invalid usages here.
> +        ASSERT(platformCursor || cursor.type() == Cursor::None);

I think it would be slightly better to put this assertion just after line 794. You could even use ASSERT_ARG to get a slightly clearer error message:

ASSERT_ARG(cursor, platformCursor || cursor.type() == Cursor::None);

[Ryan]

Unfortunately, due to contractual obligations I have, I won't be able to submit any further patches. I'm sorry.