Bug 57731 - [CSS 3] missing cursor support for 'none' on Windows
Summary: [CSS 3] missing cursor support for 'none' on Windows
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-03 13:59 PDT by Ryan
Modified: 2012-12-12 21:00 PST (History)
5 users (show)

See Also:


Attachments
Minimal test based on manual-tests/cursor.html (156 bytes, text/html)
2011-04-03 13:59 PDT, Ryan
no flags Details
Proposed patch (3.09 KB, patch)
2011-04-03 22:07 PDT, Ryan
aroben: review-
Details | Formatted Diff | Diff
Updated patch for code style (3.03 KB, patch)
2011-04-07 00:05 PDT, Ryan
no flags Details | Formatted Diff | Diff
Updated patch including WebKit2 (4.65 KB, patch)
2011-04-07 23:45 PDT, Ryan
mjs: review-
Details | Formatted Diff | Diff
Revised patch addressing comments. (9.68 KB, patch)
2011-08-21 22:43 PDT, Ryan
aroben: review-
Details | Formatted Diff | Diff
Updated patch addressing latest comments. (8.84 KB, patch)
2011-10-01 16:32 PDT, Ryan
no flags Details | Formatted Diff | Diff
Fixed patch, previous update was missing a ChangeLog (9.64 KB, patch)
2011-10-01 16:56 PDT, Ryan
aroben: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryan 2011-04-03 13:59:38 PDT
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.
Comment 1 Ryan 2011-04-03 22:07:03 PDT
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'.
Comment 2 WebKit Review Bot 2011-04-03 22:09:13 PDT
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.
Comment 3 Adam Roben (:aroben) 2011-04-04 11:28:20 PDT
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.
Comment 4 Ryan 2011-04-07 00:05:26 PDT
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?
Comment 5 Adam Roben (:aroben) 2011-04-07 05:56:35 PDT
(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.
Comment 6 Ryan 2011-04-07 23:45:16 PDT
Created attachment 88773 [details]
Updated patch including WebKit2

This patch also includes a fix for cursor: none in WebKit2.
Comment 7 Ryan 2011-04-07 23:51:13 PDT
(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...)
Comment 8 Adam Roben (:aroben) 2011-04-08 08:08:00 PDT
(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!
Comment 9 Adam Roben (:aroben) 2011-04-08 08:20:07 PDT
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.
Comment 10 Maciej Stachowiak 2011-04-26 16:43:13 PDT
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.
Comment 11 Ryan 2011-05-25 18:10:44 PDT
(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!
Comment 12 Adam Roben (:aroben) 2011-05-26 05:59:25 PDT
(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.
Comment 13 Ryan 2011-08-21 22:43:05 PDT
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.
Comment 14 Adam Roben (:aroben) 2011-08-25 08:33:24 PDT
(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.)
Comment 15 Adam Roben (:aroben) 2011-08-25 08:38:57 PDT
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)
Comment 16 Ryan 2011-10-01 16:32:53 PDT
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?
Comment 17 Ryan 2011-10-01 16:56:22 PDT
Created attachment 109410 [details]
Fixed patch, previous update was missing a ChangeLog
Comment 18 Eric Seidel (no email) 2011-12-19 11:02:35 PST
Adam roben is your man here.  This patch looks rather forgotten. :(
Comment 19 Adam Roben (:aroben) 2011-12-21 12:14:14 PST
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);
Comment 20 Ryan 2012-12-12 21:00:35 PST
Unfortunately, due to contractual obligations I have, I won't be able to submit any further patches. I'm sorry.