Good call. In fact, we should use two cursors IMHO: one for zooming in and one for zooming out.

Yeah, there was already a discussion about this. I think, Marlon is working on these icons.

Nav triage team: nsbeta1+/adt1

Jan, If you already have received the icons from Gail please reassign this bug to your self for checkin of the icons and hooking them up.

I believe Judy (JudyJoo1@aol.com) handed off the icons. Let me know if you need anything else. Jan, can you post the icon files in the bug?

I think the right way to do this is to make 'cursor: url(...);' usable and then use it. It takes more work but we get more back. I think Web authors would love it. And I assume that this bug is not a "must have" for the short term so we have time to do it right.

There are 3 possible solutions at the moment how to fix this. 1. Use a positioned <div> with a background image I don't like this one at all, It's not possible to hide the cursor and it also flickers. 2. Add new proprietary cursor properties (zoom-in/zoom-out) and implement them for windows, mac and linux/gtk This requires "a bit" more work but it would work nicely. I'm afraid some people may have objection to the proprietary properties so I ask in advance what people think about it. Maybe there are plans to add something similar in CSS3 ? 3. Implement full support for |cursor: uri| (bug 38447) This would require quite good amount of work and also good design and general consensus on the behaviour (support platform specific icon formats or all image formats or even both?). Actually this may be quite hard and I'd rather spend time on other bugs too. >And I assume that this bug is not a "must have" for the short term so we >have time to do it right. Actually this is the highest priority on my list of bugs. We definitely need it for Mozilla 1.4.

To be more exact, I'm looking for a feedback on which solution will be module owner approved and reviewed.

So you're looking to have this fixed by tomorrow night, then?

Sure no problem. </sarcasm> Sorry, I'm really trying to find a consensus. I don't want to spend time on something which won't be approved and reviewed. If there are any objections to the 2nd approach let's talk about it.

Well, approach two just needs moa from dbaron. As long as those are -moz-whatever properties, I'm fine with them in principle...

I don't object to proprietary properties in principle, as long as we call them -moz-zoom-in and -moz-zoom-out. I do object to adding new properties just to support this one application when CSS defines a url(...) property that we could use just as well, if only we implemented it properly. Gotta run. Be back later.

If solution 2 is done, it wouldn't lessen the chances of solution 3 eventually being implemented, right? If not, then solution 2 sounds best.

Yeah, except that after 3) is implemented taking out the hack for 2) is extra work.

I guess I don't have strong objections to 2). I just wish this had been started earlier so there was time to do it right via method 3).

I have an objection to 2. Implementing 'cursor:url(...)' for just .cur (same format as .ico with hotspot information) should be quite easy cross-platform since we already have a decoder for .ico files. On the long run we could then add support for other icon files, such as Mac or X icons, Windows .ani icons, PNG images with hotspot blocks, or SVG. But for this bug we just need to support .cur files. Decode the bitmap, convert it into the cursor format native to the platform, and pass it to the OS.

It might be easy but I doubt we would get it coded up, reviewed and approved in time for 1.4, which Jan says he absolutely needs. Of course, it wasn't really a good idea to show up a few days before freeze and say we need this extra feature.

I humbly suggest that at this late stage before a milestone freeze it is too late for any of the proposals given above, and Jan should just work on cursor:url() during the next milestone. As you say, it is a little late to suddenly require that. Incidentally, I assume that the suggestion of a new '-moz-zoom-in' property actually was a suggestion for a new _value_, not property? As in: cursor: -moz-zoom-in; For this bug I still say we should do the url() thing, but I may suggest these new values ('zoom-in' and 'zoom-out') to the working group when we next suggest cursor values.

Yeah, I meant value, sorry.

I agree with number 3. There doesn't seem to be any rush, in my opinion. Win32 .cur support is also simpler to code than even .ico files. We could also define our own cursor format based on any image format with XML metadata for the hotspot :-)

Thanks for all the comments, I appreciate it. I attached a patch to implement solution 2. It wasn't too hard and I think it's not risky for 1.4b even. >I have an objection to 2. Implementing 'cursor:url(...)' for just .cur (same >format as .ico with hotspot information) should be quite easy cross-platform >since we already have a decoder for .ico files. On the long run we could then >add support for other icon files, such as Mac or X icons, Windows .ani icons, >PNG images with hotspot blocks, or SVG. > >But for this bug we just need to support .cur files. Decode the bitmap, convert >it into the cursor format native to the platform, and pass it to the OS. All this sounds good and I wish we had it implemented, but we don't. Yeah we do have an image decoder for .ico, but as you said .cur files contain also hotspot record, and that requires imagelib hacking. I think the most difficult part would be the conversion to platform native format. I think sol. 2 is sufficient for now, I also talked to glazou and he thinks that these new icons just make sense. ok, who is willing to review it ?

oh, I forgot to mention that the patch is only for linux and windows. I'll add Mac support once I figure out how to add icons to nsMacWidget.rsrc Sorry for SPAM.

Comment on attachment 121737 [details] [diff] [review] patch to implement solution 2 I'm not a module owner for any of this stuff. I'd prefer if you got review from dbaron for the style system at least.

Comment on attachment 121737 [details] [diff] [review] patch to implement solution 2 >Index: widget/public/nsIWidget.h >@@ -201,7 +201,9 @@ > eCursor_spinning, > eCursor_count_up, > eCursor_count_down, >- eCursor_count_up_down >+ eCursor_count_up_down, >+ eCursor_zoom_in, >+ eCursor_zoom_out > }; How about adding eCursor_LENGTH at the end of this... >Index: widget/src/gtk/nsWidget.cpp Any idea when nsWidget::SetCursor is called? If ever, could you file a bug on making it have the same functionality as nsWindow::SetCursor? >Index: widget/src/gtk/nsWindow.cpp >-GdkCursor *nsWindow::gsGtkCursorCache[eCursor_count_up_down + 1]; >+GdkCursor *nsWindow::gsGtkCursorCache[eCursor_zoom_out + 1]; This could be eCursor_LENGTH >Index: widget/src/gtk/nsWindow.h >- static GdkCursor *gsGtkCursorCache[eCursor_count_up_down + 1]; >+ static GdkCursor *gsGtkCursorCache[eCursor_zoom_out + 1]; as could this. Should things be stubbed out for other widget toolkits?

- introduced eCursorCount and fixed related code - added support for gtk2 and xlib - stubbed out other toolkits - filed bug 204219 for nsWidget:SetCursor consolidation

(You'll probably want to get mac help in the reasonably near future to port these cursors to mac if you can't do it yourself.)

Comment on attachment 122318 [details] [diff] [review] new patch requesting approval and working on mac support in the meantime

Comment on attachment 122318 [details] [diff] [review] new patch a=sspitzer

Comment on attachment 122318 [details] [diff] [review] new patch checked in

I think there's one non-terminal bug with this: when you're toggling between zoomed and unzoomed, the cursor doesn't actually change until you move the pointer.

Silly me, forgot to say -- linux/GTKfe/x86, XFree86 4.1.99.1

That's a general bug with 'cursor' and ':hover'.

Comment on attachment 122397 [details] [diff] [review] patch for mac and cocoa I already checked in new icons for Mac (nsMacWidget.rsrc)

2 arougthopher@lizardland.net Paul, it seems in additions to changes in beos/nsWindow.cpp we need new cursors in BeOSCursors.h. As previous cursors were your art, can you take care about it?

r=pink on mac/cocoa patch. use it as you wish.

Comment on attachment 122397 [details] [diff] [review] patch for mac and cocoa adjusting flags (r=pink, sr=bryner) and requesting an approval

Comment on attachment 122397 [details] [diff] [review] patch for mac and cocoa a=sspitzer

all checked in, marking fixed

A mouse click changes the zoom state, but only the image is drawn, not the mouse pointer. The change from Zoom In Icon to Zoom Out Icon and vice versa is done only when the mouse moves.

See comment 39.

Yes, I 'm seeing the problem in comment 47# on both the Mac and win32 trunk builds. Since this actual report has been resolved, we should probably write up a new one for the magnifying glass cursor not updating automatically.