38444 - Complete support of CSS2/3 cursors on Gtk

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Michael Lowe]

Implementation of CSS2 and CSS3 cursor properties needs to be completed on Gtk.
  The Windows implementation of these is mostly complete now.   

Work needs to be done in nsWidget::SetCursor(nsCursor aCursor) to support the 
following cases correctly:

      case eCursor_sizeWE:
      case eCursor_sizeNS:
      case eCursor_sizeNW:
      case eCursor_sizeSE:
      case eCursor_sizeNE:
      case eCursor_sizeSW:

      case eCursor_crosshair:
      case eCursor_move:
      case eCursor_help:

      case eCursor_copy: // CSS3
      case eCursor_alias:
      case eCursor_cell:
      case eCursor_grab:
      case eCursor_grabbing:
      case eCursor_spinning:
      case eCursor_context_menu:
      case eCursor_count_up:
      case eCursor_count_down:
      case eCursor_count_up_down:

Refer to the following URL for a test case of CSS2 cursors:

http://www.people.fas.harvard.edu/~dbaron/css/test/sec1801

Comment 1

[Stuart Parmenter]

i'm accepting patches.  this should be easy to do, but I'm not gonna have time
to look at it for a while.

Comment 2

[Peter Trudelle]

Mass-moving all M20-M30 XPToolkit bugs to Future

Comment 3

[Scott Schram]

Available Gnome cursors are defined in:

http://lxr.mozilla.org/gnome/source/gtk+/gdk/gdkcursors.h

For example, GDK_HAND2

The Gnome cursors are created by calling XCreateFontCursor()

You can see what they all look like (with the exception of GDK_NUM_GLYPHS (154)) 
at:  (brace yourself, it's not a pretty sight)

http://www.tigr.net/afterstep/X/xlib/appendix/b/

We might be able to choose something reasonable from that list for each of our 
eCursor_xxx.  Or, maybe not.

Gnome is considering adding their own cursors, as we see in:

http://lxr.mozilla.org/gnome/source/gnome-libs/TODO.shtml#310

Because, "X's default cursors are butt ugly."

I suggest that we contact whoever wrote that at Gnome, and ask them to consider 
CSS2 / CSS3 cursors while they're adding some.

In the meantime, I suggest we pick from the available list.

Does anyone know where the standard for CSS2 / 3 cursors is?

This also needs fixing in nsWindow.cpp as described in bug 40421.

Comment 4

[Scott Schram]

I'm working on a patch for this and bug 40421.

Comment 5

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

The CSS2 standard describes cursors in:
http://www.w3.org/TR/REC-CSS2/ui.html#cursor-props

Comment 6

[Scott Schram]

Attachment: Patch to fix 40421 and get much closer on 38444

Comment 7

[Scott Schram]

My patch should fix bug 40421 completely, support pre-defined CSS2 cursors, and
some of the CSS3 cursors.  I picked the closest available cursor.

For several of the CSS3 cursors, there is no appropriate cursor available from
XCreateFontCursor().

As yet unimplemented:

eCursor_copy, eCursor_alias, eCursor_count_up, eCursor_count_down,
eCursor_count_up_down.


CSS2 also supports loading custom cursors from URLs which doesn't seem to be
supported yet.

The CSS3 Cursor spec is at:

http://www.w3.org/TR/css3-userint#cursor

Comment 8

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

The CSS3 cursors are still in (early) working draft, and are subject to change. 
If they are implemented (which they really shouldn't be unless there's a real
need for them) then the status of the draft should be tracked closely.

Comment 9

[Stuart Parmenter]

sorry I havn't responded earlier... like weeks ago...

this patch looks fine.  I'm nominating it for nsbeta2 so that I can check it in
for you.

PDT: this patch is fine and from an outside contributor that does not have a cvs
account.  Please let me check this in for him.

Comment 10

[Phil Peterson]

[nsbeta2-] not required for seamonkey beta2, but you can check in mozilla 
community contributions as long as brendan or waterson approve.

Comment 11

[Stuart Parmenter]

Attachment: patch -u style

Comment 12

[Stuart Parmenter]

ok, i've checked in the patch.  i'm going to leave this open until we get the
rest of the css3 cursors.  I think that we should probably make some xbms for
the cursors we need/don't like.

Comment 13

[Scott Schram]

I've downloaded the daily snapshot of the source, and the patch has been applied 
to widget/src/gtk/nsWindow.cpp

However, the patch put the comment in the wrong place in the function 
nsWindow::Setcursor.  The 3 line comment should be just above the 
switch(aCursor) statement.

There were two files patched in that diff I uploaded, and the other one did not 
get the patch applied at all.  It's identical patch applied to nsWidget.cpp in 
the same directory.

If you will, please move that comment down a few lines, and patch the other 
file.

Thanks!

Comment 14

[Pierre Saslawsky]

Pavv, can you look at this one? On the Mac and Windows, we just added the 
resources and a couple of switch() cases... It's a 30 minutes work.

Comment 15

[Tobias Burnus]

The CSS2 cursors seem to work well (tested at
http://www.people.fas.harvard.edu/~dbaron/css/test/sec1801), but in the third
case (cursor=auto in <a href="">) the cursor is a I-beam instead of a pointing
hand. (2000-08-13-20/Linux). (I haven't tested uri('*.cur').)

Comment 16

[Hixie (not reading bugmail)]

Netscape's standard compliance QA team reorganised itself once again, so taking 
remaining non-tables style bugs. Sorry about the spam. I tried to get this done 
directly at the database level, but apparently that is "not easy because of the 
shadow db", "plus it screws up the audit trail", so no can do...

Comment 17

[Pierre Saslawsky]

The problem reported by Tobias last year about "cursor:auto" is addressed in bug 
30971.  If that was the only problem left, maybe you can close this bug now.

Comment 18

[pocemit]

Attachment: patch for widget/src/gtk to implement most (except 3) cursors

Comment 19

[pocemit]

Attachment: widget/src/gtk/nsGtkCursor.h (part of patch)

Comment 20

[pocemit]

Attachment: widget/src/gtk/nsGtkCursorData.h (part of patch)

Comment 21

[pocemit]

Attachment: testcase to demonstrate new cursors

Comment 22

[pocemit]

Okay, this patch adds GtkCreateCursor() function which creates a GdkCursor from
bitmap data + mask.
First patch is a diff for widget/src/gtk, and 2 files following are header files
to be placed in the same directory.
One contains the cursor names + structure name another has the cursor bits.

3 unimplemented cursors are countup countdown and countupdown, but I have *no*
idea what they should look like.
This patch also improves "question-arrow" look to be a bit more user friendly.

looking for reviews

Comment 23

[pocemit]

Attachment: new GtkCursorData with nice Spinning cursor from bug 48839

Comment 24

[pocemit]

How to review this bug:

1. http://bugzilla.mozilla.org/showattachment.cgi?attach_id=40674
2. http://bugzilla.mozilla.org/showattachment.cgi?attach_id=40675
3. http://bugzilla.mozilla.org/showattachment.cgi?attach_id=40732

1) is the only "code" change, the other 2 files are just cursor data and some
structs.

Looking for r/sr
Identical code (adapted for xlib) has been already reviewed at bug 88457

Comment 25

[tor]

r=tor

Comment 26

[Christopher Blizzard (:blizzard)]

Please use enum names other than:

'GTK_CURSOR_QUESTION_ARROW'

They are in the GTK name space but they are specific to Mozilla.  That will help
tell when you should be using GtkCreateCursor() instead of gdk_cursor_new().  

Also, please cache the resulting values for the cursors somewhere instead of
re-creating them every time.  It's expensive if you don't.

Comment 27

[pocemit]

Attachment: new patch with cursor cache enabled

Comment 28

[pocemit]

Attachment: widget/src/gtk/nsGtkCursors.h

Comment 29

[pocemit]

To review this code:

a) http://bugzilla.mozilla.org/showattachment.cgi?attach_id=41717
b) http://bugzilla.mozilla.org/showattachment.cgi?attach_id=41718

(a) is the patch and (b) is the cursor data

Comment 30

[pocemit]

Attachment: removed incorrect line of code (new patch, use same cursor data)

Comment 31

[Christopher Blizzard (:blizzard)]

Two things:

Change:

+// #define DEBUG_CURSORCACHE
+

To:

+#undef DEBUG_CURSORCACHE

and remove this:

+  static GdkCursor  *GtkCursorCache[eCursor_count_up_down];

it doesn't appear to be used.  Do that and you have an sr=blizzard

Comment 32

[pocemit]

Attachment: final patch from blizzard's comments

Comment 33

[pocemit]

a) attachment 41750 [details] [diff] [review]
b) attachment 41718 [details] [diff] [review]

thank you

Comment 34

[pocemit]

Attachment: new patch, critical crash bug missed by review

Comment 35

[pocemit]

to check-in

a) attachment 41792 [details] [diff] [review] - patch
b) attachment 41718 [details] [diff] [review] - new file - widget/src/gtk/nsGtkCursors.h

Comment 36

[timeless]

Attachment: minor changes to nsGtkCursors.h

Comment 37

[pocemit]

Attachment: updated version of nsGtkCursors.h

Comment 38

[pocemit]

Attachment: updated version of nsGtkCursors.h

Comment 39

[pocemit]

Sorry about the spam.

Final version of GTK cursor patch ready to check-in:

a) attachment 41792 [details] [diff] [review]
b) attachment 41856 [details] [diff] [review]

the reason I had to re-post the cursor data was because I copied wrong license
boilerplate (NPL instead of MPL)

Comment 40

[timeless]

I just checked in pocemit's fix; I think we're done. If not someone 
should provide a list of what's left.

Comment 41

[pocemit]

Attachment: fix for NS_ASSERTION

Comment 42

[Madhur Bhatia]

verified