189719 - Automatic image sizing: Use a custom magnifying glass cursor

Status: VERIFIED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect, P1)

Description

[Jan Varga [:janv]]

Comment 1

[Hixie (not reading bugmail)]

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

Comment 2

[Jan Varga [:janv]]

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

Comment 3

[Samir Gehani]

Nav triage team: nsbeta1+/adt1

Comment 4

[Samir Gehani]

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.

Comment 5

[Lori Kaplan]

I believe Judy ([e-mail]) handed off the icons. Let me know if you need
anything else. Jan, can you post the icon files in the bug?

Comment 6

[Jan Varga [:janv]]

Attachment: zoom in

Comment 7

[Jan Varga [:janv]]

Attachment: zoom out

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 9

[Jan Varga [:janv]]

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.

Comment 10

[Jan Varga [:janv]]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

Comment 12

[Jan Varga [:janv]]

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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Well, approach two just needs moa from dbaron.

As long as those are -moz-whatever properties, I'm fine with them in principle...

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 15

[Brian 'netdragon' Bober]

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.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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).

Comment 18

[Hixie (not reading bugmail)]

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. 

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 20

[Hixie (not reading bugmail)]

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.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, I meant value, sorry.

Comment 22

[Brian 'netdragon' Bober]

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 :-)

Comment 23

[Jan Varga [:janv]]

Attachment: new zoom in cursor

Comment 24

[Jan Varga [:janv]]

Attachment: new zoom out cursor

Comment 25

[Jan Varga [:janv]]

Attachment: patch to implement solution 2

Comment 26

[Jan Varga [:janv]]

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 ?

Comment 27

[Jan Varga [:janv]]

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 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 29

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

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?

Comment 30

[Jan Varga [:janv]]

Attachment: final zoom in icon

Comment 31

[Jan Varga [:janv]]

Attachment: final zoom out icon

Comment 32

[Jan Varga [:janv]]

Attachment: new patch

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

Comment 33

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

(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 34

[Jan Varga [:janv]]

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

requesting approval and working on mac support in the meantime

Comment 35

[(not reading, please use [e-mail] instead)]

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

a=sspitzer

Comment 36

[Jan Varga [:janv]]

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

checked in

Comment 37

[Adam D. Moss]

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.

Comment 38

[Adam D. Moss]

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

Comment 39

[Hixie (not reading bugmail)]

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

Comment 40

[Jan Varga [:janv]]

Attachment: patch for mac and cocoa

Comment 41

[Jan Varga [:janv]]

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

I already checked in new icons for Mac (nsMacWidget.rsrc)

Comment 42

[Sergei Dolgov]

2 [e-mail]

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?

Comment 43

[Mike Pinkerton (not reading bugmail)]

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

Comment 44

[Jan Varga [:janv]]

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

adjusting flags (r=pink, sr=bryner) and requesting an approval

Comment 45

[(not reading, please use [e-mail] instead)]

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

a=sspitzer

Comment 46

[Jan Varga [:janv]]

all checked in, marking fixed

Comment 47

[Hermann Schwab]

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.

Comment 48

[Hixie (not reading bugmail)]

See comment 39.

Comment 49

[Chris Petersen]

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.