Closed Bug 189719 Opened 22 years ago Closed 21 years ago

Automatic image sizing: Use a custom magnifying glass cursor

Categories

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

x86
All
defect

Tracking

()

VERIFIED FIXED
mozilla1.4beta

People

(Reporter: janv, Assigned: janv)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: icon, Whiteboard: [adt1] [UI] WG (zoom-in/zoom-out cursors))

Attachments

(4 files, 5 obsolete files)

 
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.
Depends on: 190545
Nav triage team: nsbeta1+/adt1
Assignee: varga → gail
Keywords: nsbeta1icon, nsbeta1+
QA Contact: tpreston → petersen
Whiteboard: [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?
Assignee: gail → varga
Attached image zoom in (obsolete) —
Attached image zoom out (obsolete) —
Priority: -- → P1
Target Milestone: --- → mozilla1.4beta
Status: NEW → ASSIGNED
Whiteboard: [adt1] → [adt1] [UI]
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.
Depends on: 38447
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.
Whiteboard: [adt1] [UI] → [adt1] [UI] WG (zoom-in/zoom-out cursors)
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 :-)
Attachment #119088 - Attachment is obsolete: true
Attached image new zoom in cursor (obsolete) —
Attachment #119089 - Attachment is obsolete: true
Attached image new zoom out cursor (obsolete) —
Attached patch patch to implement solution 2 (obsolete) — Splinter Review
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.
Attachment #121737 - Flags: review?(roc+moz)
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.
Attachment #121737 - Flags: superreview+
Attachment #121737 - Flags: review?(dbaron)
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?
Attached image final zoom in icon
Attachment #121735 - Attachment is obsolete: true
Attached image final zoom out icon
Attachment #121736 - Attachment is obsolete: true
Attachment #121737 - Attachment is obsolete: true
Attached patch new patchSplinter Review
- introduced eCursorCount and fixed related code
- added support for gtk2 and xlib
- stubbed out other toolkits
- filed bug 204219 for nsWidget:SetCursor consolidation
Attachment #122318 - Flags: review?(dbaron)
Attachment #121737 - Flags: review?(dbaron)
(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
Attachment #122318 - Flags: superreview+
Attachment #122318 - Flags: approval1.4b?
Comment on attachment 122318 [details] [diff] [review]
new patch

a=sspitzer
Attachment #122318 - Flags: approval1.4b? → approval1.4b+
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)
Attachment #122397 - Flags: superreview?(sfraser)
Attachment #122397 - Flags: review?(bryner)
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?
Attachment #122397 - Flags: review?(bryner) → review+
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
Attachment #122397 - Flags: superreview?(sfraser)
Attachment #122397 - Flags: superreview+
Attachment #122397 - Flags: approval1.4b?
Comment on attachment 122397 [details] [diff] [review]
patch for mac and cocoa

a=sspitzer
Attachment #122397 - Flags: approval1.4b? → approval1.4b+
all checked in, marking fixed
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → 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.
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.
Status: RESOLVED → VERIFIED
Blocks: 280331
Depends on: 204841
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: