Bug 100550 – No tests for changing mouse cursors

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Rick Byers]

No tests for changing mouse cursors

[Rick Byers]

Whenever we change/extend support for custom mouse cursors, we don't add any tests because there is no machinery for them.  Eg. in http://trac.webkit.org/changeset/96566 darin@ wrote 'No tests because we currently don't have any test machinery for cursors.'.

This bug tracks adding some minimal tests for changing mouse cursors.  I'm not going to attempt to cover all the scenarios and different edge cases here, but we need to stop the bleeding so that there's no excuse not to add more tests in the future.

It looks like this should probably be done in a port-specific fashion so we can see the effects of any port-specific code (eg. see chromium's WebCursor).  See bug 99493 for more discussion.

[Rick Byers]

Created attachment 170982 [details]
Patch

[Alexey Proskuryakov]

There is a number of tests in ManualTests, can these be converted using this machinery?

[Rick Byers]

(In reply to comment #3)
> There is a number of tests in ManualTests, can these be converted using this machinery?

Thanks, I didn't know about those.  Yes it looks easy to cover all those cases in this test.  There is, of course, some additional coverage from the manual tests (eg. embedder and os code, image appearance, etc.) but it's easy to get most of the value.  In particular:

cur-hotspot.html: I intended to add this case when fixing it in chromium - bug 19059
cursor-empty-url.html: trivial, I'll add it
cursor-maxsize.html: I cover most of this already, I can add the few extra variations but not sure it has any real value.  This test also seems to expect behavior that doesn't match the code - I'll check a couple ports.
cursor.html: This exhaustively lists all standard cursors.  I considered doing this but decided it wasn't worth it - but maybe it is.
cursorfallback.xml: looks easy to add svg cases - I'll do it
custom-cursors.html: already covered here

[Rick Byers]

(In reply to comment #4)
> cursor-maxsize.html: I cover most of this already, I can add the few extra variations but not sure it has any real value.  This test also seems to expect behavior that doesn't match the code - I'll check a couple ports.

I've confirmed this test doesn't currently pass on Safari on Mac (we get the help cursor).  It expects that hotspots outside the boundary of the cursor will make the entire cursor invalid.  This probably isn't reasonable since we won't necessarily have loaded the image by the time we're choosing which cursor to use.  Current behavior for safari port is to just ignore the hotspot (fall back to 0,0).  On chromium we instead snap the hotspot to the image boundaries but I'll probably change that to be consistent with safari in bug 100059.  

Also, this  manual test incorrectly passes (shows no custom cursor) on chromium, apparently because we don't support TIFF images for cursors.  All the sites I can find on-line recommend CUR, PNG, GIF, etc cursors not tiff - so this is probably a pretty bogus test anyway.

[Rick Byers]

Created attachment 171307 [details]
Patch

[Rick Byers]

A couple more manual tests I missed:

canvas-cursor.html - this is really cool, but it's explicitly looking for flickering which I can't test here.  It also doesn't work at all on chromium for some reason, and doesn't animate smoothly on safari unless you're actively moving the mouse (we change the cursor on mouse events).

css3-cursor-fallback-quirks.html/css3-cursor-fallback-strict.html: I've included a couple of the relevant cases here.  I haven't attempted here to test the subtle differences between quirks and strict mode.  Some of this difference appears to no longer exist (parsing of hotspots is done in both cases) - neither of these tests seems to pass completely on current safari builds.

link-cursor-auto.html - interesting regression case, I've added it

[Build Bot]

Comment on attachment 171307 [details]
Patch

Attachment 171307 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14620544

New failing tests:
fast/events/mouse-cursor.html

[Rick Byers]

Created attachment 171485 [details]
Fix mac failure

[Build Bot]

Comment on attachment 171485 [details]
Fix mac failure

Attachment 171485 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14629636

New failing tests:
fast/events/mouse-cursor.html

[Rick Byers]

Created attachment 172347 [details]
Explicitly expect failure on non-chromium platforms for now

[Alexey Proskuryakov]

Comment on attachment 172347 [details]
Explicitly expect failure on non-chromium platforms for now

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

The latest patch doesn't apply, so EWS didn't test it.

> Tools/ChangeLog:10
> +        * DumpRenderTree/chromium/DRTTestRunner.cpp:

How much platform logic is in the code path you are testing? It might be meaningful to put getCurrentCursorInfo in Internals if you are mostly testing WebCore.

[Rick Byers]

Created attachment 172360 [details]
Update to ToT

[Rick Byers]

(In reply to comment #12)
> (From update of attachment 172347 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172347&action=review
> 
> The latest patch doesn't apply, so EWS didn't test it.

Thanks.  TestExpectations change often I guess.

> > Tools/ChangeLog:10
> > +        * DumpRenderTree/chromium/DRTTestRunner.cpp:
> 
> How much platform logic is in the code path you are testing? It might be meaningful to put getCurrentCursorInfo in Internals if you are mostly testing WebCore.

I debated this in some detail in bug 99493.  There isn't a ton of port-specific code, but there is some and it is precisely that code which I need to test in bugs like bug 100059.  So I definitely need to add some port-specific hook here, the only question is if it's worth having both mechanisms and testing at both layers.  Thoughts?

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/platform/gtk/TestExpectations:1315:  Path does not exist.  [test/expectations] [5]
LayoutTests/platform/mac/TestExpectations:804:  Path does not exist.  [test/expectations] [5]
LayoutTests/platform/win/TestExpectations:1999:  Path does not exist.  [test/expectations] [5]
Total errors found: 3 in 11 files


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

[Rick Byers]

(In reply to comment #15)
> Attachment 172360 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
> LayoutTests/platform/gtk/TestExpectations:1315:  Path does not exist.  [test/expectations] [5]
> LayoutTests/platform/mac/TestExpectations:804:  Path does not exist.  [test/expectations] [5]
> LayoutTests/platform/win/TestExpectations:1999:  Path does not exist.  [test/expectations] [5]
> Total errors found: 3 in 11 files

This is failing upstream at the moment (these aren't the lines I added).

[Adam Barth]

Comment on attachment 172347 [details]
Explicitly expect failure on non-chromium platforms for now

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

> Tools/DumpRenderTree/chromium/DRTTestRunner.cpp:2306
> +static const char* cursorTypeToString(WebCursorInfo::Type cursorType)

This function is kind of ugly.  Will we need to replicate it for every port?

[Adam Barth]

Looks like the enum comes from Cursor.h eventually.  If we need to test port-specific behavior, as you wrote in Comment #14, I wonder if we can avoid replicating this blob of code somehow...  One approach is to try implementing this for another port to see how it might work there.

[EFL EWS Bot]

Comment on attachment 172360 [details]
Update to ToT

Attachment 172360 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14721850

[Rick Byers]

(In reply to comment #18)
> Looks like the enum comes from Cursor.h eventually.  If we need to test port-specific behavior, as you wrote in Comment #14, I wonder if we can avoid replicating this blob of code somehow...  One approach is to try implementing this for another port to see how it might work there.

Thanks Adam.  I've started to look at implementing this for the mac port as well.  Ideally I think we'd share more than just this function - we could also share some of the logic for constructing the cursor info string (just pass in the values, have common string formatting).

Unless I'm missing something, it looks like we don't currently share much code between the mac DRT and chromium DRT.  Is it OK to just add this stuff to DumpRenderTreeCommon.cpp?

[Adam Barth]

We don't share much code.  The question is more of a design question.  On the surface, this TestRunner method seems very tightly coupled with the Chromium WebKit API.  A better design would be one that we could run on many different ports, either because the functionality was part of Internals or because the TestRunner method made sense.

[Adam Barth]

Comment on attachment 172360 [details]
Update to ToT

We should be able to avoid the giant switch statement.

[Rick Byers]

In terms of the port-specific nature of the testing: It looks like most/all other ports have USE(LAZY_NATIVE_CURSOR), which gives a Cursor object which can be tested in a port-agnostic fashion.

Without this, Cursor is entirely port-specific (just a wrapper around a port-specific PlatformCursor type), and so testing would have to be completely port-specific (really doesn't make sense to try to share test code across ports).

Talking offline with Adam, I'm going to look into switching chromium to use the LAZY_NATIVE_CURSOR path just for the sake of unification.  See bug 101501.

[Rick Byers]

Created attachment 173100 [details]
Make tests port-agnostic

[Rick Byers]

This new patch uses the same basic approach, except that it adds an API to internals.idl that works for all ports that have USE(LAZY_NATIVE_CURSOR) (nearly all of them).  I talked with Adam offline about using a formatting string and the need for a big switch statement to convert the enum to a string, and he said as long as it's not in chromium-specific code then it's fine.

Adam, how does this look now?

[Build Bot]

Comment on attachment 173100 [details]
Make tests port-agnostic

Attachment 173100 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14765648

[Build Bot]

Comment on attachment 173100 [details]
Make tests port-agnostic

Attachment 173100 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14765653

[Rick Byers]

Created attachment 173174 [details]
Add export

[Rick Byers]

Created attachment 173175 [details]
Ignore - wrong bug

[Build Bot]

Comment on attachment 173174 [details]
Add export

Attachment 173174 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14763862

[Rick Byers]

Created attachment 173281 [details]
More exports

[Rick Byers]

Created attachment 173297 [details]
One more export for gtk

[Rick Byers]

Created attachment 173368 [details]
Merge with ToT

[Adam Barth]

Comment on attachment 173368 [details]
Merge with ToT

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

> Source/WebCore/testing/Internals.cpp:1531
> +    return "UNKNOWN";

This should be ASSERT_NOT_REACHED(), right?

> Source/WebCore/testing/Internals.cpp:1545
> +    String result = String("type=") + cursorTypeToString(cursor.type())

You should use StringBuilder because you're got a conditional in here.

[Rick Byers]

Created attachment 173383 [details]
Patch for landing

[Rick Byers]

Created attachment 173395 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 173395 [details]
Patch for landing

Clearing flags on attachment: 173395

Committed r134144: <http://trac.webkit.org/changeset/134144>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 101876

[Aivo Paas]

Spotted missing braces.

@@ -1862,7 +1862,8 @@ bool EventHandler::handleMouseMoveEvent(const PlatformMouseEvent& mouseEvent, Hi
         if (FrameView* view = m_frame->view()) {
             OptionalCursor optionalCursor = selectCursor(mev, scrollbar);
             if (optionalCursor.isCursorChange())
-                view->setCursor(optionalCursor.cursor());
+                m_currentMouseCursor = optionalCursor.cursor();
+                view->setCursor(m_currentMouseCursor);
         }
     }

[Rick Byers]

(In reply to comment #40)
> Spotted missing braces.
> 
> @@ -1862,7 +1862,8 @@ bool EventHandler::handleMouseMoveEvent(const PlatformMouseEvent& mouseEvent, Hi
>          if (FrameView* view = m_frame->view()) {
>              OptionalCursor optionalCursor = selectCursor(mev, scrollbar);
>              if (optionalCursor.isCursorChange())
> -                view->setCursor(optionalCursor.cursor());
> +                m_currentMouseCursor = optionalCursor.cursor();
> +                view->setCursor(m_currentMouseCursor);
>          }
>      }

Dammit, thank you!

[Rick Byers]

(In reply to comment #41)
> (In reply to comment #40)
> > Spotted missing braces.
> > 
> > @@ -1862,7 +1862,8 @@ bool EventHandler::handleMouseMoveEvent(const PlatformMouseEvent& mouseEvent, Hi
> >          if (FrameView* view = m_frame->view()) {
> >              OptionalCursor optionalCursor = selectCursor(mev, scrollbar);
> >              if (optionalCursor.isCursorChange())
> > -                view->setCursor(optionalCursor.cursor());
> > +                m_currentMouseCursor = optionalCursor.cursor();
> > +                view->setCursor(m_currentMouseCursor);
> >          }
> >      }
> 
> Dammit, thank you!

I'm just figuring out how to add a test case for the no-cursor-change scenario (don't want to rely on chromium browser tests here), and then I'll resubmit with that new case.

Sorry for the trouble this stupid error caused.

[Darin Adler]

Comment on attachment 173395 [details]
Patch for landing

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

> Source/WebCore/testing/Internals.cpp:1530
> +    case Cursor::Pointer: return "TypePointer";
> +    case Cursor::Cross: return "TypeCross";
> +    case Cursor::Hand: return "TypeHand";
> +    case Cursor::IBeam: return "TypeIBeam";
> +    case Cursor::Wait: return "TypeWait";
> +    case Cursor::Help: return "TypeHelp";
> +    case Cursor::EastResize: return "TypeEastResize";
> +    case Cursor::NorthResize: return "TypeNorthResize";
> +    case Cursor::NorthEastResize: return "TypeNorthEastResize";
> +    case Cursor::NorthWestResize: return "TypeNorthWestResize";
> +    case Cursor::SouthResize: return "TypeSouthResize";
> +    case Cursor::SouthEastResize: return "TypeSouthEastResize";
> +    case Cursor::SouthWestResize: return "TypeSouthWestResize";
> +    case Cursor::WestResize: return "TypeWestResize";
> +    case Cursor::NorthSouthResize: return "TypeNorthSouthResize";
> +    case Cursor::EastWestResize: return "TypeEastWestResize";
> +    case Cursor::NorthEastSouthWestResize: return "TypeNorthEastSouthWestResize";
> +    case Cursor::NorthWestSouthEastResize: return "TypeNorthWestSouthEastResize";
> +    case Cursor::ColumnResize: return "TypeColumnResize";
> +    case Cursor::RowResize: return "TypeRowResize";
> +    case Cursor::MiddlePanning: return "TypeMiddlePanning";
> +    case Cursor::EastPanning: return "TypeEastPanning";
> +    case Cursor::NorthPanning: return "TypeNorthPanning";
> +    case Cursor::NorthEastPanning: return "TypeNorthEastPanning";
> +    case Cursor::NorthWestPanning: return "TypeNorthWestPanning";
> +    case Cursor::SouthPanning: return "TypeSouthPanning";
> +    case Cursor::SouthEastPanning: return "TypeSouthEastPanning";
> +    case Cursor::SouthWestPanning: return "TypeSouthWestPanning";
> +    case Cursor::WestPanning: return "TypeWestPanning";
> +    case Cursor::Move: return "TypeMove";
> +    case Cursor::VerticalText: return "TypeVerticalText";
> +    case Cursor::Cell: return "TypeCell";
> +    case Cursor::ContextMenu: return "TypeContextMenu";
> +    case Cursor::Alias: return "TypeAlias";
> +    case Cursor::Progress: return "TypeProgress";
> +    case Cursor::NoDrop: return "TypeNoDrop";
> +    case Cursor::Copy: return "TypeCopy";
> +    case Cursor::None: return "TypeNone";
> +    case Cursor::NotAllowed: return "TypeNotAllowed";
> +    case Cursor::ZoomIn: return "TypeZoomIn";
> +    case Cursor::ZoomOut: return "TypeZoomOut";
> +    case Cursor::Grab: return "TypeGrab";
> +    case Cursor::Grabbing: return "TypeGrabbing";
> +    case Cursor::Custom: return "TypeCustom";

Could we drop all these Type prefixes?

[Rick Byers]

Created attachment 174444 [details]
Patch

[Rick Byers]

(In reply to comment #43)
> 
> Could we drop all these Type prefixes?

Done.  Sorry, those were a hold-over from when this code was chromium specific (the Chromium version of that enum uses Type prefixes).

[Rick Byers]

Created attachment 174446 [details]
Just the changes from the originally landed version

[Rick Byers]

The stupid error I made (missing braces) was a little hard to write a direct test for, because it resulted in a Cursor object with an uninitialized field (unpredictable value).

So instead I added an explicit initialization and ASSERTs to the code to check that such uninitialized Cursor objects are never used.  I verified that (before fixing the missing braces) some existing WebKit layout tests failed due to these ASSERTs.  In particular, plugins/resize-from-plugin.html hits the rare NoCursorChange case, and hit this ASSERT until I added the missing braces.

I've attached both a new full patch, as well as just the diff from what I previously landed.  Adam, can you please review?  Sorry again for missing this stupid typo.

[Rick Byers]

Created attachment 174448 [details]
Just the changes from the originally landed version

[Brent Fulgham]

Comment on attachment 174444 [details]
Patch

r=me.  Rollout if the tests fail again.

[WebKit Review Bot]

Comment on attachment 174444 [details]
Patch

Clearing flags on attachment: 174444

Committed r134803: <http://trac.webkit.org/changeset/134803>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.