346690 - Implement CSS3 CR cursor:none

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jonathan Watt [:jwatt]]

The CSS3 Basic User Interface Module Candidate Recommendation adds the value |none| to the values allowed for the |cursor| property. The description given by the CR for this value is: "No cursor is rendered for the element." This behavior can be simulated by using a completely transparent png as a custom cursor, but it would be more convenient to have support for the value |none| built in.

Comment 1

[Jonathan Watt [:jwatt]]

Pointers on where to start with this would be gratefully received.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

See whether something that causes no cursor to show up is in the lists of OS cursor constants?  (Look at the nsWindow::SetCursor or nsWidget::SetCursor in each platform widget implementation.)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Aaron, are there accessibility issues to consider here?

Comment 4

[Aaron Leventhal]

(In reply to comment #3)
> Aaron, are there accessibility issues to consider here?

Thank you for asking. It will be disorienting if it's for a large area. 

Yes, users with cognitive or learning challenges will be especially affected, but so will elderly users etc. In fact, only users who are technically savvy have a chance at not being confused by it. 

It may not be an accessibility issue as much as a potential usability annoyance and security issue. Can you still click with the invisible cursor? What is it useful for? Will it make the web better or worse?

I could see it being useful over videos and images where the pointer is annoying. However, in that case the cursor should still be visible while it's moving and only become invisible when it is still for a moment. That's what I see in video player software.

Comment 5

[Jonathan Watt [:jwatt]]

David: Thanks for the tip.

Aaron: Yup, videos is the only real use I can think of for this. Whether the user should be able to click is an interesting question. I guess for my use I'd want to make the cursor visible on the first click while it's hidden and not take any other actions that may otherwise have occurred. As to whether implementing this would make the Web a better place, I guess it depends on whether people will abuse it. There will be moz people who can speak to that from previous issues where abuse has been a concern, but I'd just be speculating.

Maybe a -moz-hide-cursor:<number-of-milliseconds> would be more useful for me instead (the cursor is hidden after being stationary for <number-of-milliseconds>). Again, I don't see how you'd prevent it from being abused.

Comment 6

[Aaron Leventhal]

> I guess it depends on whether people will abuse it
Hehe. Why would that ever happen on the web? :)

Comment 7

[Aaron Leventhal]

Actually, if this is for videos then I think it's the plugin itself which should implement mouse cursor hiding. The plugin isn't looking at the CSS anyway, I don't believe. Since the plugin knows its a video it can hide the mouse cursor when the mouse is still and unclicked, especially in fullscreen mode.

My 2 cents: don't implement cursor: none

Comment 8

[Jonathan Watt [:jwatt]]

I don't know what the reasons for adding cursor:none to CSS3 were. Video was my own use case, but for an XR app, not for video embedded in a web page. Perhaps cursor:none is not the way to do this though, especially if it's going to be controversial.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Given that we support url() cursors, I don't see the harm in cursor:none.

Comment 10

[Jonathan Watt [:jwatt]]

Indeed.

For a patch to be accepted for this bug, which platforms would it need to cater to? Looking at widget/src I see gtk, gtk2, mac, os2, photon, qt, windows and xlib. For the platforms that the patch doesn't have to fix, what's the procedure? Do you look for people who are interested in those platforms and CC them on the bug? Or do you put in stub code?

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I suspect most platforms have a constant for this, so it shouldn't be hard to look it up in appropriate documentation and write code for all platforms, even if you can't test it.

Comment 12

[Aaron Leventhal]

David, should the cursor still respond to clicks?

What if the author puts up a fake mouse cursor based on an absolutely positioned element that is offset from the real position, designed to try to get you to click on something other than what you intended?

Comment 13

[Jonathan Watt [:jwatt]]

I think mouse events should be dispatched so that a script that hides/shows the mouse cursor can show it when it gets a click/mousemove event, but maybe preventDefault should be set on click events if the cursor is hidden?

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

See bug 381744 for some code that might be useful when implementing this.

Comment 15

[Jeff Walden [:Waldo]]

The cursor 'none' is tested in acid3 test 47, although to be clear it's only testing for the presence of such a cursor type (so if you had some of the concerns mentioned here you could just make 'none' display as 'auto' and still pass, although that arguably goes against the spirit of the test).

Comment 16

[Jeff Walden [:Waldo]]

Attachment: Patch

This works on Windows, Linux, and OS X and has stubs for other widget implementations; we pass an extra acid3 test with this patch.

A test URL for ensuring the cursor actually isn't displayed (and that style works correctly) is:

data:text/html,%20<div%20style="width:%20200px;%20height:%20200px;%20background:%20cyan;%20cursor:%20auto;">auto</div>%20%20<div%20style="width:%20200px;%20height:%20200px;%20background:%20gray;%20cursor:%20none;">none</div>%20%20<script>%20onload%20=%20function%20()%20alert(document.getElementsByTagName("div")[1].style.cursor);%20</script>

This passes the layout/style Mochitests.

Comment 17

[Jeff Walden [:Waldo]]

Requesting blocking because it's acid3 and the fix is mostly menial updates to
existing functionality without any real complexity...

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The normal way to handle 'none' values for CSS properties is to pass VARIANT_NONE as one of the flags to ParseVariant (in CSSParserImpl::ParseCursor) instead of adding none to the keyword list in nsCSSProps.cpp.  However, in this case, what you did may actually be preferable; it's probably better to keep it within eCSSUnit_Enumerated rather than having to add an eCSSUnit_None case.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm a little suspicious of the cocoa code -- are you sure it's safe to call set/unset with the non-cursor?

And are the hot_x/hot_y 0-based or 1-based?  If the latter, then you may want 1,1 instead of 0,0.  Then again, they're probably 0-based...

Other than that this looks fine.

Comment 20

[Jeff Walden [:Waldo]]

You can't see it in the patch, but setCursor first looks in a cache for the cursor; if it's not there yet, it calls [nsCursorManager createCursor: aCursor].
The createCursor method, if passed a cursor it doesn't recognize, hits this default and returns an arrow cursor:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/widget/src/cocoa/nsCursorManager.mm&rev=1.8&mark=179-180#160

So the cocoa part's safe.

hot_x and hot_y are relative to the cursor's origin, so 0's right.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 297268 [details] [diff] [review]
Patch

Well, it might be better not to go through the don't-know-what-to-do codepath (i.e., just hide the cursor straight away), but r=dbaron.

Comment 22

[Jeff Walden [:Waldo]]

(In reply to comment #21)
> (From update of attachment 297268 [details] [diff] [review])
> Well, it might be better not to go through the don't-know-what-to-do codepath
> (i.e., just hide the cursor straight away), but r=dbaron.

The problem with that is set has internal side effects with animated timers (at least I gather that's the reason), and I don't believe set is idempotent -- going from memory, I think we'd leak a timer if we called set twice.  That could be changed, but this seems easier and more self-contained.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

ok, sounds good

Comment 24

[Mats Palmgren (inactive)]

Comment on attachment 297268 [details] [diff] [review]
Patch

For many .rc files it seems we have a special version for OS/2 -
do we need to update those too?  ie. the following:
browser/app/splashos2.rc
calendar/sunbird/app/splashos2.rc
mail/app/splashos2.rc
suite/app/splashos2.rc
xulrunner/app/splashos2.rc
toolkit/library/xulrunos2.rc


Please add a XXX placeholder to Photon also, see "eCursor_zoom_in":
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/widget/src/photon/nsWidget.cpp&rev=1.133&root=/cvsroot&mark=578#534


>Index: widget/src/os2/nsWindow.cpp
>+    case eCursor_none:
>+      newPointer = gPtrArray[IDC_NONE-IDC_BASE];

You need to make room in the array.  Add IDC_NONE and update
IDC_COUNT here:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/widget/src/os2/wdgtos2rc.h&rev=1.5&root=/cvsroot


>Index: widget/src/os2/widget.rc
> POINTER IDC_HELP           res\help.ptr
>+POINTER IDC_NONE           res\help.ptr

Ok, but please file a new bug on making a "none.ptr" and updating
the line above, CC [e-mail] I think he can make one from
the Windows "none.cur" file.

Comment 25

[Jeff Walden [:Waldo]]

Attachment: With more .rc file madness

This better?

I was planning on CCing people associated with the various ports after committing the patch, at which point they could do whatever needed to be done to deal with my hacks.  I doubt I'll file separate bugs for them as I'm not super-interested in getting that bugmail; whatever the ports need to do to handle this should be straightforward and basically uninteresting to me. :-)

Comment 26

[Mats Palmgren (inactive)]

Comment on attachment 297775 [details] [diff] [review]
With more .rc file madness

sr=mats

Comment 27

[Jeff Walden [:Waldo]]

Comment on attachment 297775 [details] [diff] [review]
With more .rc file madness

A fairly simple, if tedious, set of changes to increase our numeric acid3 score by 1 (not to mention support more of the CSS3 UI spec)...

Comment 28

[Mike Schroepfer]

Comment on attachment 297775 [details] [diff] [review]
With more .rc file madness

I'm approving this over my better judgement.  Jeff we really need to fix that stuff already landed - not add new code! :-)

Comment 29

[Jeff Walden [:Waldo]]

CCing a bunch of people who work on various ports...

I stubbed out code for a transparent cursor in a bunch of widget implementations, but I've only tested Windows, OS X, and gtk2.  It shouldn't be too much effort to fully implement it on new platforms, but I'd prefer if someone who can actually test the code did it.  Note that OS/2 should, if I've done things correctly, display the "help" cursor when |cursor: none| is specified, but it should be fairly obvious how to fix that.  :-)

Comment 30

[Sergei Dolgov]

for BeOS real implementation is simple, but requires code restructuring a bit.
case "none" should look like:
be_app->HideCursor(); - and that's all.

but we also should add statement somewhere:
be_app->ShowCursor() for all remaining cases.
Will try to create patch ASAP

Comment 31

[Sergei Dolgov]

Attachment: patch for BeOS against current trunk

that is.

Comment 32

[Mike Schroepfer]

Yep - not a blocker but we'll take the patch in hand.  

Comment 33

[Jeff Walden [:Waldo]]

Patch is in; tinderboxen seem okay for all the various apps on all the various platforms (excluding what looks like a spurious and unrelated Windows error on a xulrunner tinderbox), so I think we're good here.

I'm marking in-testsuite+ on the basis of the property_database.js change.  Since we don't have a way to programmatically test that the cursor doesn't show or displays as nothing, however, we probably should have a manual test to ensure this.  Simply loading the following URL and mousing over the box to verify the cursor's hidden is a sufficient test:

data:text/html,<div%20style="min-width:100px;height:100px;background:#0f0;cursor:none">Shouldn't%20see%20cursor%20if%20you%20hover%20over%20this%20box</div>

Comment 34

[Mats Palmgren (inactive)]

Attachment: Testcase

FWIW, here's a testcase I originally did to test bug 163174.
It allows you to see how the cursors look against different backgrounds.
I've added cursor:none to it so it now has all the values we implement.
Maybe it can be used as a base for a litmus test...

Comment 35

[Peter Weilbacher]

Attachment: OS/2 fix (checked in)

Thanks for CCing OS/2 people on this bug!

This is the patch with a new fully transparent OS/2 pointer and related changes to the splashos2.rc files to get it to display -- or actually not display. ;-)

Comment 36

[Sergei Dolgov]

Comment on attachment 297821 [details] [diff] [review]
patch for BeOS against current trunk

hope logic there is quite clear

Comment 37

[tqh]

Comment on attachment 297821 [details] [diff] [review]
patch for BeOS against current trunk

r=[e-mail]

Comment 38

[Sergei Dolgov]

Do I need approval to land BeOS patch?

Comment 39

[Peter Weilbacher]

I guess for files in beos directories NPOTB (not part of the build) applies. If you want to be really sure you should post in mozilla.dev.planning and get an answer from the release managers.

Comment 40

[Mike Schroepfer]

(In reply to comment #38)
> Do I need approval to land BeOS patch?
> 

Nope. NPOB so you are good to go!

Comment 41

[Sergei Dolgov]

(In reply to comment #40)
> Nope. NPOB so you are good to go!
> 
Thanks, landed:
/cvsroot/mozilla/widget/src/beos/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.142; previous revision: 1.141
done

Comment 42

[kguru]

I want there to be an easily accessible and clearly labeled preference to enable or disable this functionality as I could see this being a very dangerous feature.  Perhaps leave it off by default, and if the browser hits cursor:none it should popup asking what you want it to do.

Something like this:

(In Preferences)
Enable cursor invisibility [yes] [no] [ask] {should default to ask}

(Upon hitting cursor:none)
If Preference is still ask:

"This site can cause your mouse cursor to become invisible.  Do you want this to occur." [yes [would then change the preference]] [no]

Comment 43

[Boris Zbarsky [:bzbarsky]]

> I want there to be an easily accessible and clearly labeled preference

What's the point?  See comment 9.  A transparent png cursor is just as invisible...

Comment 44

[uamjet602]

(In Preferences)
Enable cursor shape manipulation [yes] [no] {should default to yes}

Comment 45

[Vincent Lefevre]

(In reply to comment #44)
> (In Preferences)
> Enable cursor shape manipulation [yes] [no] {should default to yes}

This would be useful. I've submitted a RFE in bug 596581.