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

RESOLVED FIXED in mozilla11

Status

()

Core
Widget: Gtk
--
minor
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: Chris Coulson, Assigned: Chris Coulson)

Tracking

({regression})

Trunk
mozilla11
All
Linux
regression
Points:
---

Firefox Tracking Flags

(firefox9 affected, firefox10+ affected)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 580512 [details]
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
(Assignee)

Comment 1

6 years ago
Created attachment 580514 [details]
Video of the "spinning" cursor in Thunderbird 9 (broken)

Updated

6 years ago
(Assignee)

Comment 2

6 years ago
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 :(
Blocks: 627699
Is gdk_cursor_new_from_name() an option?

Comment 4

5 years ago
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

5 years ago
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 ;))
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.
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

5 years ago
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).
(Assignee)

Updated

5 years ago
Assignee: nobody → chrisccoulson
(Assignee)

Comment 9

5 years ago
Created attachment 580970 [details] [diff] [review]
Try creating a named cursor before a bitmap cursor
Attachment #580970 - Flags: review?(karlt)
(Assignee)

Comment 10

5 years ago
Created attachment 580971 [details] [diff] [review]
Try creating a named cursor before a bitmap cursor

Oops, that wasn't the most recent version
Attachment #580970 - Attachment is obsolete: true
Attachment #580970 - Flags: review?(karlt)
Attachment #580971 - Flags: review?(karlt)
Comment on attachment 580971 [details] [diff] [review]
Try creating a named cursor before a bitmap cursor

Thanks very much.
Attachment #580971 - Flags: review?(karlt) → review+
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?
Attachment #580971 - Flags: review+ → review?(karlt)
Attachment #580971 - Flags: review?(karlt) → review-
(Assignee)

Comment 13

5 years ago
Created attachment 581198 [details] [diff] [review]
Try creating a named cursor before a bitmap cursor (v2)

Good catch, thanks
Attachment #580971 - Attachment is obsolete: true
Attachment #581198 - Flags: review?(karlt)
Attachment #581198 - Flags: review?(karlt) → review+
Keywords: checkin-needed
Whiteboard: [not part of WINNT build]
https://hg.mozilla.org/mozilla-central/rev/596e3eca4196
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [not part of WINNT build]
Target Milestone: --- → mozilla11

Updated

5 years ago
status-firefox10: --- → affected
status-firefox9: --- → affected
Keywords: regression

Updated

5 years ago
Duplicate of this bug: 693517
Please nominate for Beta 10 approval if this fix is considered low risk.
tracking-firefox10: --- → +
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
Attachment #581198 - Flags: approval-mozilla-esr10?
(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 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.
Attachment #581198 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10-

Updated

3 years ago
Blocks: 1034064

Updated

3 years ago
No longer blocks: 1034064
You need to log in before you can comment on or make changes to this bug.