411273 - Improve appearance of busy/busyButClickable cursor

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, enhancement)

Description

[Louise]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2008010801 Minefield/3.0b3pre
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2008010801 Minefield/3.0b3pre

Firefox changes mouse cursor to old black and white wheel when busy.
It should use the rainbow windmill for better OS integration.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Jesse Ruderman]

Works fine for me on Tiger: I get the colorful beachball when I do e.g.
  javascript:while(1);
to hang Firefox.

Comment 2

[Louise]

I actually tried your example and in my Leopard (10.5.1) it also showed the rainbow windmill.
However, I still see the black and white wheel.  I can't clarify exactly when but I see them more often than the colorful ones.

Comment 3

[philippe (part-time)]

Do you mean a little spinning wheel with an arrow (I'll attach a test case next) ? You can see that sometimes on slow loading sites. That is the correct CSS cursor for 'progress' (showing page activity).

The colour beachball indicates a hang/freeze

Comment 4

[philippe (part-time)]

Attachment: test

Is that the icon you mean ?

Comment 5

[Louise]

(In reply to comment #4)
> Created an attachment (id=296085) [details]
> test
> 
> Is that the icon you mean ?
> 

yes, I was talking about this black and white wheel.  Was this intentional?

Comment 6

[Jesse Ruderman]

Interesting, Firefox uses the black-and-white wheel for cursor:progress but Safari uses a blue stationary beachball.

Comment 7

[philippe (part-time)]

(In reply to comment #5)

> ...  Was this intentional?

Yes, that is intentional. That cursor icon has different semantics than the spinning beachball (rainbow windmill). See bug 362623.

Shall I dupe this bug to 362623 ? Or morph this bug as a RFE to improve the appearance of this particular cursor icon ?
(I'm not sold on the look of Safari's implementation, btw).

Comment 8

[Louise]

(In reply to comment #7)
> Shall I dupe this bug to 362623 ? Or morph this bug as a RFE to improve the
> appearance of this particular cursor icon ?
> (I'm not sold on the look of Safari's implementation, btw).
> 

Thanks, I would like to have this bug changed for improving the look of the progress cursor ( the black and white wheel).  
I read bug 362623 and while I personally think the behavior of Firefox in OS X should be close to that of Safari, this seems to take time to get fixed so we should have better icon that match the entire OS in the mean time.

Comment 9

[Louise]

changing the title.
Firefox should use something that match to the theme of OS X better for the progress cursor.

Comment 10

[Josiah Bruner [:jsbruner]]

We should be using the blue circular icon that Safari uses here.

Comment 11

[Andrew Thompson]

That cursor is available either through the Carbon API or by linking to the JavaVM Framework. It's not part of the general NSCursor API, only via additions in the JavaVM Framework.

I did file a radar asking for it to be promoted to the regular NSCursor API.

Comment 12

[Andrew Thompson]

That cursor is available either through the Carbon API or by linking to the JavaVM Framework. It's not part of the general NSCursor API, only via additions in the JavaVM Framework.

I did file a radar asking for it to be promoted to the regular NSCursor API.

Comment 13

[Josiah Bruner [:jsbruner]]

I'll go ahead and address this when I have a little extra time (since this is definitely not high-priority)

(In reply to Andrew Thompson from comment #12)
> That cursor is available either through the Carbon API or by linking to the
> JavaVM Framework. It's not part of the general NSCursor API, only via
> additions in the JavaVM Framework.
> 
> I did file a radar asking for it to be promoted to the regular NSCursor API.

Correct, however, we do not use the NSCursor API at all here. Instead, we have custom cursors and create them ourselves. It is possible (though I haven't checked), that we use initWithImage:hotSpot: to create NSCursors with our images. This would make the most sense.

Comment 14

[Andrew Thompson]

The code is here:
http://dxr.mozilla.org/mozilla-central/widget/cocoa/nsCursorManager.mm

But then I don't need to look very hard, because I wrote most of it ;) I see someone has added Retina support, which is neat.

I am hoping Apple exposes this cursor in the NSCursor API in the next release of Mac OS X. I just checked and RADAR #12939978 is still OPEN. You could file a similar RADAR because multiple votes for the same issue apparently count for something inside Apple, so their engineers say. You can read what I posed in RADAR here: http://openradar.appspot.com/12939978

Comment 15

[Josiah Bruner [:jsbruner]]

(In reply to Andrew Thompson from comment #14)
> The code is here:
> http://dxr.mozilla.org/mozilla-central/widget/cocoa/nsCursorManager.mm
> 
> But then I don't need to look very hard, because I wrote most of it ;) I see
> someone has added Retina support, which is neat.
> 
> I am hoping Apple exposes this cursor in the NSCursor API in the next
> release of Mac OS X. I just checked and RADAR #12939978 is still OPEN. You
> could file a similar RADAR because multiple votes for the same issue
> apparently count for something inside Apple, so their engineers say. You can
> read what I posed in RADAR here: http://openradar.appspot.com/12939978

Cool thanks! Just took a look at the code there, and apparently using native NSCursor APIs are not frowned upon (To my great pleasure). Unfortunately, even if the cursor was made available in the next version of the OSX SDK, we wouldn't be able to use it. Our API calls must be available since 10.6 I believe.

What I will do though is create the images and make a new cursor for earlier versions, and we can #IFDEF for 10.9 (If it is made available), and use native NSCursors instead.

Comment 16

[Josiah Bruner [:jsbruner]]

Filed another bug to Apple about the issue. Also, if it is possible to access this cursor via a private call, that could be considered as a possible use, especially if we receive the cursor in the 10.9 SDK, in which case any altercations would go unnoticed.

Not saying we necessarily should (Creating another custom cursor may be the best way to go), but it is definitely a possibility.

Comment 17

[Josiah Bruner [:jsbruner]]

Attachment: Busy but still clickable cursor

Here's the image supplied via the Webkit.framework. However, it only supplies this single image. The animations looks like it still needs to be created...

Comment 18

[Josiah Bruner [:jsbruner]]

Attachment: Patch.

So this ended up being substantially easier than I thought.

This patch switches from using our custom spinX cursors and instead uses the undocumented busyButClickableCursor method instead. Though undocumented, Markus Stange went through Appkit dumps from 10.9 to 10.4 and found they all contain the same method, so I conclude that it is safe to use.

Steven, could you review this?

Comment 19

[Steven Michaud [:smichaud] (Retired)]

> Steven, could you review this?

Sure.  I'll also test it on 10.6 and up.

I should be able to get to it later today or tomorrow.

Comment 20

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8359413 [details] [diff] [review]
Patch.

One thing I can say right now, even before testing:

Since +[NSCursor busyButClickableCursor] is undocumented, there won't be a declaration for it in any of the standard AppKit header files.  So using it will (or should) trigger a warning.

To get around this you need to add something like the following to one of the Cocoa widgets header files (like nsCursorManager.h):

@interface NSCursor (Undocumented)
+[NSCursor busyButClickableCursor];
@end

Technically this is a category that will be added to the NSCursor class (which gets rid of the warnings).  Move your comment about busyButClickableCuror from where it is now to just above its declaration in this category.

Comment 21

[Steven Michaud [:smichaud] (Retired)]

> +[NSCursor busyButClickableCursor];

+ (NSCursor*)NSCursor busyButClickableCursor];

Comment 22

[Steven Michaud [:smichaud] (Retired)]

> + (NSCursor*)NSCursor busyButClickableCursor];

+ (NSCursor*)[NSCursor busyButClickableCursor];

Sigh :-(

Comment 23

[Josiah Bruner [:jsbruner]]

You mean "+ (NSCursor*)busyButClickableCursor;"? 

I don't think "(NSCursor*)[NSCursor busyButClickableCursor];" will compile. Maybe I'm wrong though.

Comment 24

[Steven Michaud [:smichaud] (Retired)]

> You mean "+ (NSCursor*)busyButClickableCursor;"?

Yes.  I guess I was more tired than I realized :-(

Comment 25

[Josiah Bruner [:jsbruner]]

Attachment: Patch.

Added the interface.

Comment 26

[Josiah Bruner [:jsbruner]]

Hmm. Maybe you meant move the comment inside the interface. I'll do that after the review though if that's what you wanted.

Comment 27

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8359794 [details] [diff] [review]
Patch.

This looks fine to me.  And I've now tested it on all the major versions of OS X that we support (10.6 and up).  It worked fine, and looks much better than what we had before.

Yes, I did mean for the comment to be just above the declaration of busyButClickableCursor.

Comment 28

[Josiah Bruner [:jsbruner]]

Attachment: Final Patch.

Moved comment. Carrying r+ flag...

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/12ec0bd6b782

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/12ec0bd6b782