709259 - Appearance of "spinning" cursor regressed sometime between Thunderbird 8 and 9

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Chris Coulson]

Attachment: Video of the "spinning" cursor in Thunderbird 8

I've noticed whilst running Thunderbird 9 beta that when my mailboxes refresh, my mouse pointer changes to a really ugly busy cursor. In Thunderbird 8, the cursor seemed to not look too bad.

I just rebuilt the latest comm-beta after reverting http://hg.mozilla.org/releases/mozilla-beta/rev/875cb4f20eac, and it seems to fix it. I've not fully investigated why this patch would break it just yet.

The less attractive spinning cursor also appears occasionally in Firefox too, although it appears a lot more in Thunderbird.

This can be easily reproduced in Firefox or Thunderbird by using a JS shell and doing window.setCursor("spinning"). I've attached 2 videos of me doing this before and after reverting the mentioned patch

Comment 1

[Chris Coulson]

Attachment: Video of the "spinning" cursor in Thunderbird 9 (broken)

Comment 2

[Chris Coulson]

Ok, so this is a side-effect of no longer using gdk_cursor_new_from_pixmap (as it's not available in GTK3).

gdk_cursor_new_from_pixmap uses XCreatePixmapCursor to create a cursor, and this does some magic via libxcursor to match a hash of the cursor bitmap to a themed pointer (left_ptr_watch), which gave us the nice looking pointer. There doesn't seem to be any support in xlib or gdk for creating a pointer of this type with any other mechanism though :(

Comment 3

[Karl Tomlinson (:karlt)]

Is gdk_cursor_new_from_name() an option?

Comment 4

[Benjamin Otte]

FWIW, I removed that code, because we got rid of pixmaps in GTK3. I certainly did not intend to remove any functionality previously available, nor do I think I did that. Of course, it made some things different and possibly even harder.

From looking at the patch, I can't understand why it would break either though, because gdk_cursor_new_from_pixmap() cannot be used to create animated cursors afaics.

I guess what I'm saying is: If there's functionality missing in GTK, poke us so we can add it back.

Comment 5

[Benjamin Otte]

Oh, I'm just now seeing the explanation in comment 2. Do you have a cgit link to the X code for this handy? Because this sounds like something really really ugly inside X (wow, what a surprise ;))

Comment 6

[Karl Tomlinson (:karlt)]

Looks like this is the Xcursor code:
http://cgit.freedesktop.org/xorg/lib/libXcursor/tree/src/xlib.c?id=e086eb1bf49f2a8c270eaebd5beb595c1dc2973e#n415

gdk_cursor_new_from_name() also uses XcursorLibraryLoadCursor(), so it should be possible to get the same behavior.

If the cursor can also be found under the "left_ptr_watch" name then that would be preferable for the lookup.  As a last resort, the name from the hash can be used.

Comment 7

[Karl Tomlinson (:karlt)]

Looks like a left_ptr_watch lookup should be the same as a 08e8e1c95fe2fc01f976f1e063a24ccd.

http://cgit.freedesktop.org/xorg/data/cursors/tree/handhelds/Makefile.cfg

I guess we can't be certain the lookup will succeed, so we'd still have to fallback to the pixbuf.

Comment 8

[Benjamin Otte]

Yeah, I think the solution is to try a new_from_name() first for all cursors and if that returns NULL fall back to the image (or the default cursor if you're reasonably sure everybody supports the cursor you care about).

Comment 9

[Chris Coulson]

Attachment: Try creating a named cursor before a bitmap cursor

Comment 10

[Chris Coulson]

Attachment: Try creating a named cursor before a bitmap cursor

Oops, that wasn't the most recent version

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 580971 [details] [diff] [review]
Try creating a named cursor before a bitmap cursor

Thanks very much.

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 580971 [details] [diff] [review]
Try creating a named cursor before a bitmap cursor

>+  { moz_none_bits,           moz_none_mask_bits,           0,  0,  NULL }

>         newType = MOZ_CURSOR_NONE;
>         break;

>+    // If by now we don't have a xcursor, this means we have to make a custom
>+    // one. First, we try creating a named cursor based on the hash of our
>+    // custom bitmap, as libXcursor has some magic to convert bitmapped cursors
>+    // to themed cursors
>+    if (newType != 0xFF) {
>+        gdkcursor = gdk_cursor_new_from_name(gdk_display_get_default(),
>+                                             GtkCursors[newType].hash);

Doesn't this need to check that hash is non-null?

Comment 13

[Chris Coulson]

Attachment: Try creating a named cursor before a bitmap cursor (v2)

Good catch, thanks

Comment 14

[Dão Gottwald [:dao]]

https://hg.mozilla.org/mozilla-central/rev/596e3eca4196

Comment 16

[Alex Keybl [:akeybl]]

Please nominate for Beta 10 approval if this fix is considered low risk.

Comment 17

[Mike Hommey [:glandium]]

Comment on attachment 581198 [details] [diff] [review]
Try creating a named cursor before a bitmap cursor (v2)

[Approval Request Comment]
User impact if declined: Regression in aspect of various mouse cursors not following the system theme on Linux systems in Firefox with ui.use_activity_cursor set to true, and in Thunderbird.
Fix Landed on Version: 11
Risk to taking this patch (and alternatives if risky): 
String changes made by this patch: None

Comment 18

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #17)
> Risk to taking this patch (and alternatives if risky): 

(forgot to fill here) very low risk, and has been tested widely for a while now (since it was released in 11). It would be nice if it was applied on ESR, though.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 581198 [details] [diff] [review]
Try creating a named cursor before a bitmap cursor (v2)

this didn't make it into beta 10 and it doesn't meet the criteria for ESR.