The default bug view has changed. See this FAQ.

Automatic image sizing: Use a custom magnifying glass cursor

VERIFIED FIXED in mozilla1.4beta

Status

()

Core
Layout: Images
P1
normal
VERIFIED FIXED
14 years ago
8 years ago

People

(Reporter: janv, Assigned: janv)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {icon})

Trunk
mozilla1.4beta
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(4 attachments, 5 obsolete attachments)

(Assignee)

Description

14 years ago
 
Good call. In fact, we should use two cursors IMHO: one for zooming in and one
for zooming out.
(Assignee)

Comment 2

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

Updated

14 years ago
Depends on: 190545

Comment 3

14 years ago
Nav triage team: nsbeta1+/adt1
Assignee: varga → gail
Keywords: nsbeta1 → icon, nsbeta1+
QA Contact: tpreston → petersen
Whiteboard: [adt1]

Comment 4

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

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

Comment 6

14 years ago
Created attachment 119088 [details]
zoom in
(Assignee)

Comment 7

14 years ago
Created attachment 119089 [details]
zoom out
(Assignee)

Updated

14 years ago
Priority: -- → P1
Target Milestone: --- → mozilla1.4beta
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

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

Comment 9

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

Comment 10

14 years ago
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? 
(Assignee)

Comment 12

14 years ago
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)
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 :-)
(Assignee)

Updated

14 years ago
Attachment #119088 - Attachment is obsolete: true
(Assignee)

Comment 23

14 years ago
Created attachment 121735 [details]
new zoom in cursor
Attachment #119089 - Attachment is obsolete: true
(Assignee)

Comment 24

14 years ago
Created attachment 121736 [details]
new zoom out cursor
(Assignee)

Comment 25

14 years ago
Created attachment 121737 [details] [diff] [review]
patch to implement solution 2
(Assignee)

Comment 26

14 years ago
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 ?
(Assignee)

Comment 27

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

Updated

14 years ago
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?(roc+moz)
(Assignee)

Updated

14 years ago
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?
(Assignee)

Comment 30

14 years ago
Created attachment 122315 [details]
final zoom in icon
Attachment #121735 - Attachment is obsolete: true
(Assignee)

Comment 31

14 years ago
Created attachment 122316 [details]
final zoom out icon
Attachment #121736 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #121737 - Attachment is obsolete: true
(Assignee)

Comment 32

14 years ago
Created attachment 122318 [details] [diff] [review]
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
(Assignee)

Updated

14 years ago
Attachment #122318 - Flags: review?(dbaron)
(Assignee)

Updated

14 years ago
Attachment #121737 - Flags: review?(dbaron)
Attachment #122318 - Flags: review?(dbaron) → review+
(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.)
(Assignee)

Comment 34

14 years ago
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+
(Assignee)

Comment 36

14 years ago
Comment on attachment 122318 [details] [diff] [review]
new patch

checked in

Comment 37

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

14 years ago
Silly me, forgot to say -- linux/GTKfe/x86, XFree86 4.1.99.1
That's a general bug with 'cursor' and ':hover'.
(Assignee)

Comment 40

14 years ago
Created attachment 122397 [details] [diff] [review]
patch for mac and cocoa
(Assignee)

Comment 41

14 years ago
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)

Comment 42

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

Comment 44

14 years ago
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+
(Assignee)

Comment 46

14 years ago
all checked in, marking fixed
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 47

14 years ago
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.

Comment 49

14 years ago
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
Blocks: 190545
No longer depends on: 190545

Updated

8 years ago
Depends on: 204841
You need to log in before you can comment on or make changes to this bug.