Implement Handling of URI Values on CSS "cursor" Properties

VERIFIED FIXED in mozilla1.8beta2

Status

()

Core
CSS Parsing and Computation
P4
normal
VERIFIED FIXED
18 years ago
3 years ago

People

(Reporter: Michael Lowe, Assigned: Biesinger)

Tracking

(Blocks: 4 bugs, {css2})

Trunk
mozilla1.8beta2
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(8 attachments, 20 obsolete attachments)

39.74 KB, text/plain
Details
3.44 KB, patch
roc
: review+
Details | Diff | Splinter Review
41.96 KB, patch
Details | Diff | Splinter Review
817 bytes, patch
Details | Diff | Splinter Review
7.58 KB, patch
Details | Diff | Splinter Review
15.78 KB, patch
Details | Diff | Splinter Review
7.87 KB, patch
Details | Diff | Splinter Review
2.45 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

18 years ago
This bug has been broken off from bug #1916, which is now a meta bug about 
CSS2/3 cursor property support.
(Reporter)

Updated

18 years ago
Blocks: 1916
(Reporter)

Updated

18 years ago
Keywords: css2
Hmmm... what cursor formats should be accepted?  Are there any good XP cursor
formats?
SVG?

Comment 3

18 years ago
Resolved as Later. It raises about the same problems as downloadable fonts and 
has nowhere near the same usefulness.
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → LATER
By which of course, you meant "Milestone: Future". Right?
Status: RESOLVED → REOPENED
Resolution: LATER → ---
Target Milestone: --- → Future

Comment 5

18 years ago
My Latered bugs are meant to be reconsidered after we ship this version. I think 
it's what Future is for too but I prefer Later because the bugs don't appear on 
top of my list. I understand that it can make them harder to find but I don't 
have a lot of them and I don't think I ever received a dup for any of these, so I 
prefer to mark them Later and avoid the noise. I may change my habits if 
bug 38477 gets fixed (or if you give me a compelling reason :-).
Re-latered for now.
Status: REOPENED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → LATER
Netscape's standard compliance QA team reorganised itself once again, so taking 
remaining non-tables style bugs. Sorry about the spam. I tried to get this done 
directly at the database level, but apparently that is "not easy because of the 
shadow db", "plus it screws up the audit trail", so no can do...
QA Contact: chrisd → ian
Despite comments above, LATER is deprecated, so reopening and moving to Future.
Status: RESOLVED → REOPENED
Resolution: LATER → ---

Comment 8

16 years ago
Windows builds should support .cur and .ani, which are expected to be the
majority of URI requests. Support on other platforms for these formats isn't
expected, so it's safe to use Windows API.
Recognizing a platform-specific cursor format for web pages (which should be
platform-independent) is a Bad Thing (TM).

Comment 10

16 years ago
dbaron: What platform-independent cursor format are you thinking of, then?
PNG with a custom hotspot block would do the trick.

dbaron: Of course, we could `just' implement a CUR/ANI decoder for the other 
platforms, so then it would be XP. ;-)

Comment 12

16 years ago
Ahh, but creating browser-proprietary formats is A Much More Bad Thing! If IE6
supports one format (.CUR/.ANI, basically the same thing), and NS6 supports
another, we'll just be back to the old days of incompatibility between browsers.
Until W3C creates a standardized cursor format, it would be best just to stick
with .CUR and .ANI. I don't think any webmasters will be tolerant enough of
Netscape to use a proprietary format just so they can style the cursor (as it is
lots of sites block out NS6 and send NS4 to a text-only page).

Comment 13

16 years ago
And in case it wasn't clear from Skewer's comments, IE6 will support .cur 
and .ani. Read all about it:
http://msdn.microsoft.com/workshop/author/dhtml/reference/properties/cursor.asp?
frame=true

Mozilla should support .cur and .ani as well, since they'll undoubtably be the 
most common format, especially if IE6 gets there with support first. Microsoft 
faces the same XP problem with the Mac version of IE. I wonder if it will 
support .cur and .ani? For that matter, how detrimental is it to not support 
the cursor URL syntax on all platforms? It seems like you could do something 
like 
cursor: url("yourcursor.cur"); cursor: pointer 
for browsers that don't support the url syntax.

I'd suggest just using the Windows API and add XP support later. As hixie 
notes, it would be possible to implement a .cur or .ani decoder. I don't think 
those image formats are very complex.

Comment 14

16 years ago
Actually, the correct format is cursor: url("yourcursor.cur"), pointer; (broken,
bug 77974). Question: Do MacOS and Linux have their own cursor formats? If so,
it would make sense to support those as well in their respective versions.

Comment 15

16 years ago
Needs a dev relnote.
Keywords: relnote

Comment 16

16 years ago
Macintosh cursors are traditionally embedded "crsr" Resources rather than
standalone files. These Resources appear to contain a series of numbered bitmap
graphic resources each of which has a single "hot" pixel.

I suppose you could specify ".rsrc" file that contained a single "crsr" resource
as the Mac OS (Classic) solution. I'm unaware of changes made for Mac OS X.

Comment 17

16 years ago
Actually, my comments are a little incomplete. See
[http://developer.apple.com/techpubs/mac/QuickDraw/QuickDraw-401.html] for
complete Mac OS cursor details.

Comment 18

16 years ago
I suppose that begs the question, "Can a URI refer to a resource within a
Macintosh file, and if so, how?"

Comment 19

16 years ago
CCing the guys responsible for the BMP/icon decoder for bug 18502. They may be
able to help with a cross-platform solution. It may be possible to load the
bitmap cursor then reformat it for each platform's own cursor format--and maybe
eventually vice-versa. We should avoid displaying the cursor as a dynamically
drawn bitmap at all costs, even if it means not supporting it.

With all this talk of decoding cursor formats some may suggest creating a new
special format for Mozilla. I'm still against this--nobody will use it unless it
appears somewhere at W3C or is in use in a popular operating system (what does
Linux use?), and the existing formats would have no hotspot, severely limiting
their use. If we tried, for example, a PNG-CUR conversion, that would just be a
waste of space (and bloat the browser even more).
Status: REOPENED → NEW
Keywords: mozilla1.0

Comment 20

16 years ago
A .cur file is the same format as an .ico file, so we have a decoder for it.

Comment 21

16 years ago
In response to concerns about the Mac, we could support 2 types of cursors:
- windows CUR file: the file can be converted to the binary representation of 
'crsr' resource.  It is basically a problem of conversion between Windows bitmaps 
and Mac bitmaps.
- native 'crsr': the file should contain in its data fork the binary data of a 
'crsr' resource.

Then in both cases, the data can be dynamically added to the application's 
resource fork as a 'crsr' resource.  For more info on the CUR format, see
http://www.oreilly.com/centers/gff/formats/miccur/

Updated

16 years ago
Summary: Implement CSS2 'cursor: url("yourcursor.cur")' → Implement Handling of URI Values on CSS2 "cursor" Properties

Comment 22

16 years ago
*** Bug 114938 has been marked as a duplicate of this bug. ***
Priority: P3 → P4
Assigning pierre's remaining Style System-related bugs to myself.
Assignee: pierre → dbaron

Comment 24

15 years ago
worth mentioning... the current W3C CSS 2.1 *Draft* no longer defines the uri
property... nor does the CSS3 User Inferface *Draft*.

http://www.w3.org/TR/2002/WD-CSS21-20020802/ui.html#cursor-props
http://www.w3.org/TR/2000/WD-css3-userint-20000216.html#cursor

I haven't taken the time to track down any relevant discussions on www-style so
I don't know if this is an intentional move or simply an oversight in the
current drafts.

Comment 25

15 years ago
To Comment 24: I don't understand what you're talking about?

http://www.w3.org/TR/2003/WD-CSS21-20030128/ui.html#cursor-props
http://www.w3.org/TR/CSS21/ui.html#cursor-props

I can see the <uri> value specified. This one:

http://www.w3.org/TR/2000/WD-css3-userint-20000216.html#cursor

states "extensions to CSS2" and "New Values" only. And here:

http://www.w3.org/TR/2002/WD-css3-ui-20020802/#cursor
http://www.w3.org/TR/css3-ui/#cursor

the <uri> presents too.

Comment 26

15 years ago
Apologies... I have been looking at an older 2.1 draft... so scratch my previous
comments.


Looking at the updated drafts I see mention of SVG cursors... does this give us
any more direction as to a cross platform/standard cursor?
http://www.w3.org/TR/SVG/interact.html#CursorElement

Comment 27

15 years ago
If there is a standard cross-platform cursor format, we should support it. But
in the meantime it is absurd for Mozilla not to support a platform-dependent
cursor file on its native platform (e.g. a .cur file). The reason CSS2 (and
CSS3) supports repeated URI's is because you can provide a cursor file in each
different format to ensure compatibility.
Blocks: 189719

Updated

15 years ago
Keywords: helpwanted
*** Bug 177193 has been marked as a duplicate of this bug. ***

Comment 29

14 years ago
I recommend that while everyone is arguing :-) someone should set it up so that
if Mozilla cannot display the cursor, it would fall back to a specified cursor
(like n-resize, crosshair, etc.) sort of like alternate text for images. Example
CSS to get that effect:

.cursor { cursor: crosshair; }
.cursor { cursor: url(cool-cursor.cur); }

I would hope that this makes the crosshair show up if the cool-cursor does not.
If I could find where to edit (and if it does not require compiling) I would not
mind at least tring to make this work. I am almost HTML/CSS guru, and have an
idea what I am doing with Javascript. 
comment 29 is the wrong syntax, but the right idea.  See bug 77974.

Comment 31

14 years ago
Why don't we use formats already supported by Mozilla, like for the favicon
(where .ico, PNG, GIF, MNG... are accepted) ?
Because Mozilla doesn't draw the cursor itself -- the OS draws the cursor.

Comment 33

14 years ago
stupid question:

hiding os cursor and painting own cursor ?

Comment 34

14 years ago
if in CSS to write: ".aaa {cursor: hand;}" and then to use it in HTML as:
<button class="aaa">bla bla bla</button> the mouse pointer is not done "hand" in
Mozilla 1.6 :(
Why?

Comment 35

14 years ago
That would be because 'hand' does not occur in any specification (but 'pointer'
does, and is rendered as a hand on some OS) so Mozilla is fully conformant there.

Updated

14 years ago
Keywords: mozilla1.0, relnote → css3
Summary: Implement Handling of URI Values on CSS2 "cursor" Properties → Implement Handling of URI Values on CSS "cursor" Properties

Comment 36

14 years ago
For additional testing,see the SVG Test suite in particular
http://www.w3.org/Graphics/SVG/Test/20030813/svggen/interact-cursor-01-f.svg
linked from
http://www.w3.org/Graphics/SVG/Test/20030813/htmlframe/full-interact-cursor-01-f.html
SVG-enabled builds of Mozilla have partial support.
What exactly does this bug need to get fixed? A cross-platform .cur converter?
I've got a textfile (of unknown origin) with specifications of MS Win
BMP/ICO/CUR format. Is there any Mac's? Let's start that Windows <--> Macintosh
thing.

Comment 38

14 years ago
Well, the issue of 'what format' was solved in SVG by inventing a cursor
*element* which has an x and y hotspot as attributes, plus a link to a PNG file.
Since all SVG implementations have to support PNG (and JPEG) then this gives
enough info for a platform-independent cursor property (the cursor property
points to url(url-of-cursor-element) which can be in a separate file of course.

Pointing to platform-specific cursor files is harmful IMHO

CSS3 takes the SVG idea and adds the hotspot x and y values right into the
property definition, removing the need to indirect via an XML snippet.

So what it takes to implement this, at least for SVG and for CSS3, is the
ability to call whatever the platform has for a 'construct custom cursor from
this image and this x and y data' function call.

Comment 39

14 years ago
> Pointing to platform-specific cursor files is harmful IMHO

I tend to agree, though Internet Explorer already supports this property and has
two different file formats: .cur/.ani (see also comment 12).

Comment 40

14 years ago
Yes, IE supports something like this property (although it calls pointer 'hand'
despite the explanation that this is offensive in some cultures which I ,and the
CSS WG, explained to them about 6,7 years ago ... its never been updated to CSS2
in the interim and its unlikely that it ever will since the project is disbanded.

The broader point is that if MacoS X folks point toMacOS X cursor formats and
Gnome and KDE folks point to X11 cursor formats and set-top-box fols point to
set-top-box formats and PDA .... you get a bunch of platform specific content. 
Its way better to have cross-platform contentand put the platform-specific
aspects (which function to call) in the implementation rather than exposed in
the content.

Comment 41

14 years ago
IE6 support both 'hand' and 'pointer', but I was not talking about that.

There are indeed cross-platform problems. And a PNG image in combination with a
SVG would be nice. But I don't think authors are going to use that when it
doesn't work in Internet Explorer.

We could try to add support for SVG in combination with PNG first (leave alone
that SVG is not enabled by default), but I think there should be a
function/class that converts '.cur' or '.ani' into '.png'. This is probably not
necessary for Mac and Unix cursors, since there isn't any browser yet on those
platforms that has support for the 'cursor:url()'.
Keywords: qawanted

Comment 42

14 years ago
Chris: I ask out of curiosity:

The disbanded project you mention is IE, isn't it? CSS2 is alive and kicking
(new 2.1 spec being the evidence)

Comment 43

14 years ago
(yes the disbanded projects are IE/Win and IE/Mac, both officially listed as not
under development)

If we are limited to what IE supports then why bother with standards? If stuff
looks 'ok' on legacy browsers (remember, url has a fallback to a predefined
cursor shape) but better on newer, standards-based browsers then that s just a
migration pressure, right?

BTW there are of course implementations of custom cursors on MacOS and X11.
Perhaps you meant 'arent any HTML browsers that support ...'. The SVG
implementations do.

For Mozilla, if the desire is to not introduce any dependency on SVG (despite
Brendan Eich listing SVG as crucial for the development of Mozilla) then the
CSS3 cursor property with the hotspot additions would suffice for pointing to a
PNG without indirecting through SVG.

Comment 44

14 years ago
Bug 169678 may be related ["Cursor shape should reflect the type of link hovered
on (e.g. pop-up/download/mailto...) (different cursors for links to new windows)"]

Prog.
well personally, I'd say the way to go is to add a function to set any
imgIContainer as a cursor, and implement decoders for .cur (I believe we have
that, except hotspot information (via the .ico decoder))/.ani/macos's cursor format.

> function/class that converts '.cur' or '.ani' into '.png'

why would that be desirable?
My name is being invoked bogusly.  Anyone with experience managing software by
requirements knows you don't turn on SVG in the default builds at this point to
fix this bug.  SVG's day will come; the day when we don't have to care about
what IE supports may come, but I doubt it.  Realism, please.

/be

Comment 47

14 years ago
> why would that be desirable?

That or a similar function to create a 'bridge' accross different platforms. If
we can simple encode them it is fine with me. I was just suggesting something
Mozilla could do, to show it on every platform.

When can we expect SVG? I think that Mozilla should support at least the CSS3
solution, since that is probably a lot easier to understand than SVG (not
probably, it is).
As I said in comment 32, I think the hard part would be the "function to set any
imgIContainer as a cursor".  Though it might not be as bad as I'd think.

If somebody plans to do this across platforms, I could write a patch to upgrade
our 'cursor' parsing to css3-ui ( http://www.w3.org/TR/css3-ui/#cursor ), which
has the syntax:
  [ [<uri> [<x> <y>],]* <keyword> | inherit ]
instead of just
  [ [<uri>,]* <keyword> | inherit ]
and propagate both the URIs and the coordinates through the style system.
(In reply to comment #48)
> As I said in comment 32, I think the hard part would be the "function to set any
> imgIContainer as a cursor".

I didn't mean to imply that it was easy :)

(In reply to comment #47)
> > why would that be desirable?
> 
> That or a similar function to create a 'bridge' accross different platforms. 

I was asking because I doubt any platform uses png as their native cursor
format... it seems so unlikely that toolkit apis require a compressed format.

Comment 50

14 years ago
Clearly most API calls would require uncompressed image data. Decoding the PNG
is not that hard, though ;-)

Wrapping allthe platform-specific apis with a platform independent api that took
a PNG and hotspot would be reasonable.

Comment 51

14 years ago
"When can we expect SVG?" well, its already on the trunk, so just compile it up
if you are curious. SVG-enabled Mozilla passes all of the cursor test except for
the custom, URI-defined cursor subtest.

Comment 52

14 years ago
Brendan said "My name is being invoked bogusly."

No, I just mentioned exactly what your slides said, in passing.

"Anyone with experience managing software by
requirements knows you don't turn on SVG in the default builds at this point to
fix this bug."

Correct. You turn it on because people want the functionality, and it gives them
a reason to upgrade their browser to a good, modern, standards compliant one. It
also, btw, would give a second reason to fix this bug (note that the SVG builds
*also* have this bug for the exact same reason).


"SVG's day will come" 

well it would come a lot quicker if people got the functionality built in and
could use it everywhere (CSS backgrounds, chrome, etc) instead of being limited
to what a plugin can do.

"the day when we don't have to care about what IE supports may come, but I doubt
it."

Chuckle. If you want to lock yourself into some four year old, bugwards
compatible definition of reality then feel free. Mozilla as a whole has to
decide if it wants to use the time between the cancellation of IE and the
shipment of Longhorn to grab market share or to tiptoe around being careful not
to give anyomne a reason to switch.

"Realism, please."

My thoughts exactly. We seem to have a different view of what is realistic and
desirable. It has been very refreshing to see the rapid advances in XHTML, SMIL,
SVG compliance of the cellphone market which is untrammelled by the bugwars
compatible morass. Hopefully the desktop market, stagnant for years, will catch
up at some point.
(In reply to comment #50)
> Wrapping allthe platform-specific apis with a platform independent api that took
> a PNG and hotspot would be reasonable.

An imgIContainer as I suggested works just fine for that, without complexifying
it by a specific image format...
chris@w3.org, please stop spamming this bug.  Trying to attach glory and realism
to yourself from some other peoples' achievements in cell-phone browsers has no
place in this bug, and no relevance to desktop browsers that have to deal with
the bulk of the web-as-it-is, not the web-as-you-wish-it-were, or
the-tiny-cell-phone-targeted-subset of the web.

Oh, wait -- you're telling me those cell-phone-targeted SVGT pages work in IE? 
I didn't think so.

Making something work in Mozilla, but not in Safari and Opera, and probably in
IE via a plugin or extension, does almost nothing to make that something succeed
on the web.  It can actually be a negative, due to opportunity costs.

Mozilla is in jeopardy of losing several current "sales" (not that we make any
money, necessarily) precisely because we do not support IE proprietary crap. 
That's reality, I face it every day.  I suggest that you are in an ivory tower,
namely, the w3c.  Try developing a desktop browser and getting distribution
channels for it some time -- your casual opinion of others' realism may change.
 A lot.

I'm done here.

/be

Updated

14 years ago
Blocks: 163174

Comment 55

13 years ago
(In reply to comment #3)
> It raises about the same problems as downloadable fonts and 
> has nowhere near the same usefulness.

Custom cursors can be very useful if the user wants to make certain types of
elements easily recognizable via userContent.css (I ran into this bug when I
tried to set up a special cursor for links that open new windows). You don't
need cross-platform support for this, just supporting the native cursor format
of the underlying OS would be sufficient.
Created attachment 151144 [details]
Manual on Windows bitmaps (BMP, ICO, CUR)

This is the file I mentioned three months ago, in comment 37.

I feel that this kind of specs will come handy in coding Win32 solution for
conversion Mac <--> Win, or SVG <--> CUR, or CUR <--> imgIContainer.

Unfortunately, I've got no ANI specs.

And if someone is still curious about the file's origin: I've discovered it to
be once UUE-decoded by me from some Fidonet echo, region R50. Still I don't
know exactly, what echo was it.

The file itself is dated May 1994. Wow, it's a really old thing! ;-)
Blocks: 164421
nsGNOMEShellService.cpp has a way to convert an gfxIImageFrame into a GdkPixbuf.
(WriteImage does that)

With Gtk 2.4, you can set a GdkPixbuf as cursor:
http://developer.gnome.org/doc/API/2.0/gdk/gdk-Cursors.html#gdk-cursor-new-from-pixbuf
http://developer.gnome.org/doc/API/2.0/gdk/gdk-Windows.html#gdk-window-set-cursor

.cur files are really just .ico files, and mozilla supports those. the only
thing missing currently is the hotspot information, but that's easily addable.

Assignee: dbaron → cbiesinger
Keywords: helpwanted
Target Milestone: Future → mozilla1.8alpha3
Created attachment 154944 [details] [diff] [review]
imagelib part
Attachment #154944 - Flags: review?(pavlov)
Created attachment 154952 [details] [diff] [review]
xp widget part (checked in)

interface changes for widget/ + stub impl in nsBaseWidget
Attachment #154952 - Flags: superreview?(roc)
Attachment #154952 - Flags: review?(roc)
Created attachment 154956 [details] [diff] [review]
content/layout changes

this implements the CSS 2.1 syntax only. CSS 3 is left as an exercise to the
reader :) I didn't know the CSS Parser well enough to know how to store the two
coordinates as well on the nsCSSValue.

This depends on the previous two attachments.

There are a few unrelated string changes in there; I can separate them if
desired.
Attachment #154956 - Flags: superreview?(dbaron)
Attachment #154956 - Flags: review?(dbaron)
I have an impl for gtk2 too. I don't want to attach it just yet though, it
currently only works with gtk 2.4 (and prevents running mozilla on older versions).

for this bug, I will do gtk2 and windows implementations, and file bugs for
other platforms.
Status: NEW → ASSIGNED
Attachment #154952 - Flags: superreview?(roc)
Attachment #154952 - Flags: superreview+
Attachment #154952 - Flags: review?(roc)
Attachment #154952 - Flags: review+

Comment 62

13 years ago
Comment on attachment 154944 [details] [diff] [review]
imagelib part

I'd really like to see how this is being used prior to r='ing this patch.  I
don't really like adding ICO specific parts to the imagelib APIs.  Does
anything else use this?
Why don't you like the idea? The resulting APIs (after the above patch applied)
are still backwards compatible, aren't they? If they aren't, please quote a few
codestrings proving their backward incompatibility. If they are, then what are
you actually worried about?

Comment 64

13 years ago
If the CUR changes are approved, I can add the same functionality for XBM
cursors.  (Of course it would be nice if my other XBM changes are checked in
first...)
Created attachment 154977 [details] [diff] [review]
gtk2 changes, v0, NOT FOR CHECKIN

OK.... pavlov, this shows how gtk2 would use this. win32 would use it in a
similar way, for the X- and Y-hotspot members of
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/winui/WinUI/WindowsUserInterface/Resources/Icons/IconReference/IconStructures/ICONINFO.asp
(In reply to comment #63)
> Why don't you like the idea? The resulting APIs (after the above patch applied)
> are still backwards compatible, aren't they?

why would that matter?

(In reply to comment #62)
> I
>don't really like adding ICO specific parts to the imagelib APIs.  Does
>anything else use this?

as ask said, XBM supports this too -
http://netghost.narod.ru/gff/graphics/summary/xbm.htm#XBM-DMYID.2

Comment on attachment 154956 [details] [diff] [review]
content/layout changes

>Index: layout/base/public/nsIFrame.h
>+  /**
>+   * This structure holds information about a cursor. mContainer represents a
>+   * loaded image that should be preferred. If it is not possible to use it, or
>+   * if it is null, mCursor should be used.
>+   */
>+  struct Cursor {
>+    nsCOMPtr<imgIContainer> mContainer;
>+    PRInt32                 mCursor;
>+  };

Is it true that any image that we successfully decode we can use as a cursor? 
If so, this makes sense; otherwise you need a list.

>   /**
>    * Get the cursor for a given frame.
>    */
>-  NS_IMETHOD  GetCursor(nsPresContext* aPresContext,
>-                        nsPoint&        aPoint,
>-                        PRInt32&        aCursor) = 0;
>+  NS_IMETHOD  GetCursor(nsPoint&        aPoint,
>+                        Cursor&         aCursor) = 0;

If you're here, you should make this |const nsPoint&|.

And yes, I'd prefer if you edited the unrelated changes out of the patch so I
know what it is I'm reviewing.
(In reply to comment #67)
> Is it true that any image that we successfully decode we can use as a cursor? 
> If so, this makes sense; otherwise you need a list.

Yeah.. that should be true. I certainly intend it to be that way.

> If you're here, you should make this |const nsPoint&|.

good point.

> And yes, I'd prefer if you edited the unrelated changes out of the patch so I
> know what it is I'm reviewing.

well, I meant to have them reviewed as well and check them in w/ the rest.
they're just unrelated in that they are not needed to fix this bug.

thinking about it, splitting them out is probably better. I'll attach a new
patch tomorrow.
Created attachment 155173 [details] [diff] [review]
content/layout changes, v2

- unrelated changes removed
- using const nsPoint& in the GetCursor signature
- make sure never to add null requests to nsStyleUserInterface::mCursorArray
Attachment #154956 - Attachment is obsolete: true
Attachment #155173 - Flags: superreview?(dbaron)
Attachment #155173 - Flags: review?(dbaron)
Attachment #154956 - Flags: superreview?(dbaron)
Attachment #154956 - Flags: review?(dbaron)
Created attachment 155390 [details] [diff] [review]
windows part
Attachment #155390 - Flags: review?(ere)
Comment on attachment 155390 [details] [diff] [review]
windows part

oops, this mishandles images w/ 1-bit transparency (.gif)
Attachment #155390 - Attachment is obsolete: true
Attachment #155390 - Flags: review?(ere) → review-
Created attachment 155401 [details] [diff] [review]
windows patch, v2

now supports opaque cursors too, and gifs as cursors. also, checks allocations
for failure.
Attachment #155401 - Flags: review?(ere)
Created attachment 155458 [details] [diff] [review]
gtk2 patch, v1

gtk2 patch v1. supports all testcases I could come up with. (alpha-blended
cursors are nice! ;) ). unfortunately this version still requires gtk 2.4.

note that gtk 2.x x <4, as well as gtk 1.2, only support monochrome cursors...
Attachment #154977 - Attachment is obsolete: true

Comment 74

13 years ago
What do we need to do to allow monochrome cursors for older GTK clients?  XBM,
of course, is monochrome, but it seems that the current XBM code converts the
image to 32-bit... I suppose somebody should look at bug 143046 and apply those
concepts to keep XBMs at 2-bit... otherwise we're wasting CPU and memory by
converting from 2 to 32 and back to 2-bit.  Of course this is a tangental issue
and probably should have a separate bug opened.
(In reply to comment #74)
> What do we need to do to allow monochrome cursors for older GTK clients?

convert the 24-bit color data to 1-bit data...
I'll probably do that, but I'm not sure of a good algorithm...
maybe: one_bit_data = red > 127 || green > 127 || blue > 127;

> I suppose somebody should look at bug 143046 and apply those
> concepts to keep XBMs at 2-bit... otherwise we're wasting CPU and memory by
> converting from 2 to 32 and back to 2-bit.  Of course this is a tangental issue
> and probably should have a separate bug opened.

ew, then the code here becomes even more complex, to deal with all the various
possible depths...
Comment on attachment 155173 [details] [diff] [review]
content/layout changes, v2

>Index: content/base/src/nsRuleNode.cpp

>@@ -2342,19 +2342,23 @@ nsRuleNode::ComputeUserInterfaceData(nsS
>+      ui->mCursorArray.AppendObjects(parentUI->mCursorArray);

This isn't quite right. I think you want to clear mCursorArray before doing
that (for reasons explained later).

>Index: content/html/style/src/nsComputedDOMStyle.cpp

>+    for (int i = 0; i < count; i++) {

PRInt32 i, please?

>Index: content/shared/src/nsStyleStruct.cpp

You need to fix the copy constructor to copy the parent's mCursorArray. 
Otherwise the default "inherit" specified by CSS won't work.

This is why rulenode needs to clear first; otherwise in some cases you would
end up doubling the length of the list.

Please use C++-style initializers for the copy constructor too (I know that's
not what it does at the moment; just fix the existing 3 members to use them).

I wish we could usefully refcount nsCOMArray instances instead of copying. 
Really, using nsIMutableArray or nsISupportsArray would only make code worse in
3 places (here, by a tad, and in nsFrame and nsComputedDOMStyle) but would
reduce memory usage on some pages (those that use user-interface struct
properties other than cursor, though I guess there are precious few of
those...).

>Index: layout/html/base/src/nsFrame.cpp
>+void nsFrame::FillCursorInformationFromStyle(const nsStyleUserInterface* ui,

I really wish we had a way to ask the toolkit whether it supports the cursor...
Document here that this code assumes that if a toolkit supports cursor images
like this at all it will support all image formats Mozilla knows about?

And maybe file a bug on a lighweight api to ask the toolkit whether a cursor is
supported?

>+}
>+
>+

Extra newline here.

>Index: layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp
>+      // XXX set aCursor.mContainer too?

Yes.  Why not just use your static nsFrame method here (called on the ui struct
off the childContext)?
> This isn't quite right. I think you want to clear mCursorArray before doing
> that

Actually, a simpler approach is to just check whether mCursorArray is empty.  If
it's not, we've already copied from the parent; if it is, just append the parent
array.  Just comment that that's what's going on if you do it that way.

> I really wish we had a way to ask the toolkit whether it supports the
> cursor...

Never mind me; we're using the imagelib decoders for all this stuff anyway.

Using nsCOMArray should be fine; just add comments in nsStyleStruct.h explaining
why the memory overhead will be minimal.
(In reply to comment #76)
> This isn't quite right. I think you want to clear mCursorArray before doing
> that (for reasons explained later).

OK. However, it seems like this is not a correctness problem, only a perf one;
since the newly appended entries will be the same ones as the ones already
there, and we are only using the first cursor that's loaded, if any.
Attachment #155173 - Attachment is obsolete: true
Attachment #155173 - Flags: superreview?(dbaron)
Attachment #155173 - Flags: superreview-
Attachment #155173 - Flags: review?(dbaron)
Attachment #155173 - Flags: review-
Created attachment 155483 [details] [diff] [review]
content/layout changes, v3
Attachment #155483 - Flags: superreview?(bzbarsky)
Attachment #155483 - Flags: review?(bzbarsky)
Did I get it right: this patch currently does not support Windows ANI (animated
cursors), and someone will have to file a separate bug for that?

[Just curiosity, not implying any criticism.]
That is correct. Animated gifs are not supported either.
Comment on attachment 155483 [details] [diff] [review]
content/layout changes, v3

> nsChangeHint nsStyleUserInterface::CalcDifference(const nsStyleUserInterface& aOther) const
> {
>   if (mCursor != aOther.mCursor)
>     return NS_STYLE_HINT_VISUAL;
> 
>+  // We could do better. But it wouldn't be worth it, URL-specified cursors are
>+  // rare.
>+  if (mCursorArray.Count() > 0 || aOther.mCursorArray.Count() > 0)
>+    return NS_STYLE_HINT_VISUAL;
>+

I think both the existing code and the new code you're adding are wrong.  How
does a repaint cause the cursor to be updated?	Doesn't that just happen on
mouse move?  If a repaint does actually update the cursor, make this code
correct (comparing the array), otherwise replace both it and the existing
cursor code with a comment.  (BTW, if it doesn't update the cursor, probably
the best solution is to add a new hint -- we have room for plenty more hints.)
Comment on attachment 155483 [details] [diff] [review]
content/layout changes, v3

looks like you are right. I'll add a new style hint (eChangeHint_UpdateCursor)
Attachment #155483 - Flags: superreview?(bzbarsky)
Attachment #155483 - Flags: superreview-
Attachment #155483 - Flags: review?(bzbarsky)
Attachment #155483 - Flags: review-
Created attachment 155496 [details] [diff] [review]
content/layout changes v4

now with a new style change hint, so that dynamic cursor changes work correctly
(this also fixes dynamic changes of non-url cursors). also, triggers start of
cursor loads in nsStyleContext::ApplyStyleFixups, so that they are already
available when users hover over an element with a custom cursor.
Attachment #155483 - Attachment is obsolete: true
Attachment #155496 - Flags: superreview?(bzbarsky)
Attachment #155496 - Flags: review?(bzbarsky)
Comment on attachment 155496 [details] [diff] [review]
content/layout changes v4

Looks good.  r+sr=bzbarsky
Attachment #155496 - Flags: superreview?(bzbarsky)
Attachment #155496 - Flags: superreview+
Attachment #155496 - Flags: review?(bzbarsky)
Attachment #155496 - Flags: review+

Comment 86

13 years ago
Comment on attachment 155401 [details] [diff] [review]
windows patch, v2

This doesn't compile:
c:\mozilla_source\mozilla\widget\src\windows\nsWindow.h(361) : error C2061:
syntax error : identifier 'imgIContainer'

+  HDC dc = ::GetDC(NULL);

ReleaseDC seems to be missing from DataToBitmap.

+  head.biHeight = h; // Our image is top-down

Is this comment relevant?
Attachment #155401 - Flags: review?(ere) → review-
Created attachment 157887 [details] [diff] [review]
windows patch, v3

fixes ere's comments. also, updated to trunk (previous patch doesn't apply
cleanly)
Attachment #155401 - Attachment is obsolete: true
Attachment #157887 - Flags: review?(ere)

Updated

13 years ago
Attachment #157887 - Flags: review?(ere) → review+
Attachment #157887 - Flags: superreview?(roc)
Comment on attachment 157887 [details] [diff] [review]
windows patch, v3

Code looks fine. Two questions:

-- Why aren't we supporting 8bit alpha on Windows?

-- I'd be happier if the malloc(w*h) were converted to the correct size. It's
not hard...
Attachment #157887 - Flags: superreview?(roc) → superreview+

Comment 89

13 years ago
This patch will only implement the CSS 2.1 version as mentioned in comment 60,
correct?
yes, that is correct.
(In reply to comment #88)
> -- Why aren't we supporting 8bit alpha on Windows?

Hm... testing seems to show that windows does not really support that. But since
I can save code by supporting 8bpp in DataToBitmap and getting rid of Conv8to1,
which also makes the code support cursors with a width that is not a multiple of
8, I'm doing it.

> -- I'd be happier if the malloc(w*h) were converted to the correct size. It's
> not hard...

Ah yeah, now that I do calculate out_bpr here, it's trivial - out_bpr * h.
Created attachment 158801 [details] [diff] [review]
windows patch, v4

> Ah yeah, now that I do calculate out_bpr here, it's trivial - out_bpr * h.

although the code is now gone anyway :-)
Attachment #157887 - Attachment is obsolete: true
Attachment #158801 - Flags: superreview?(roc)
Attachment #154944 - Attachment is obsolete: true
Attachment #154944 - Flags: review?(pavlov) → review-
Created attachment 158928 [details] [diff] [review]
imagelib part, v2

per discussion w/ pavlov, adds an "readonly attribute nsIProperties properties"
attribute on imgIContainer on which the coordinates are stored as wrapped
integers.
Attachment #158928 - Flags: review?(pavlov)
Attachment #158801 - Attachment is obsolete: true
Attachment #158801 - Flags: superreview?(roc)

Comment 94

13 years ago
Comment on attachment 158928 [details] [diff] [review]
imagelib part, v2

I suspect that this information should live on the gfxIImageFrames instead of
on the container since you could have different data per frame.. for hotspots,
that would be a little weird, i guess, but you could certainly have different
comments per frame in APNG...
Created attachment 158930 [details] [diff] [review]
windows patch, v5

updated per imagelib changes. also, removes unused bpr argument from
DataToBitmap,  and changes another malloc(w*h) to use the correct dimensions.
Attachment #158930 - Flags: superreview?(roc)
pavlov: yes, but I may also have image-global data I want to store. maybe I want
to attach EXIF data to an image, or the comment field of an image format.

Comment on attachment 158930 [details] [diff] [review]
windows patch, v5

hAlpha uninitialized if format == gfxIFormats::BGR and the malloc fails

Win2K and WinXP definitely support 8bit alpha in cursors. The system cursors
have it. Why shouldn't it work for us?
Attachment #158930 - Flags: superreview?(roc) → superreview-
ok, I found:
http://support.microsoft.com/default.aspx?scid=kb;en-us;318876

"Alpha blended cursors and alpha blended icons are only supported on Microsoft
Windows XP."

so it seems that windows wants the alpha information to be in the HBITMAP
itself. since mozilla stores it elsewhere, I'll have to rewrite the image data,
inserting the alpha channel. fun... and it looks like I can't reuse the gtk2
code for this. *sigh*

oh, and I probably need OS-detection somewhere in there.

Updated

13 years ago
Blocks: 169678

Comment 99

13 years ago
Should the target milestone be moved since it has passed?
how does that matter. but ok, doing it just for you.
Target Milestone: mozilla1.8alpha3 → mozilla1.8beta

Comment 101

13 years ago
(In reply to comment #100)
> how does that matter. but ok, doing it just for you.

If it doesn't matter, it should just be taken out of bugzilla then.
(In reply to comment #74)
> What do we need to do to allow monochrome cursors for older GTK clients?

I'm starting to wonder whether this is worth it... I now saw how a monochrome
cursor looks (after I implemented this for qt. which, as I then noticed, does
not support colored cursors. grrr), I decided that I'll only implement this for
gtk 2.4.
Target Milestone: mozilla1.8beta → mozilla1.8alpha5
Created attachment 160412 [details] [diff] [review]
gtk2 patch, v2

this should work w/ gtk2.x x<4 users (in the sense that mozilla will work),
although they will not see such cursors.
Attachment #155458 - Attachment is obsolete: true
Attachment #160412 - Flags: review?(bryner)
Attachment #158928 - Attachment is obsolete: true
Attachment #158928 - Flags: review?(pavlov) → review-
Created attachment 161501 [details] [diff] [review]
imagelib patch, v3

not going to attach an updated gtk patch just yet, the change is trivial anyway
Attachment #161501 - Flags: review?(pavlov)
Comment on attachment 160412 [details] [diff] [review]
gtk2 patch, v2

I'd really like it if we could share this code to convert a gfxIImage to a
GdkPixbuf.  One way we could do this would be to subclass gfxImageFrame for
gtk, and add a new interface that has a method something like:

GdkPixbuf* ConvertToPixbuf(PRBool aForceAlphaChannel);

What do you think?
Comment on attachment 160412 [details] [diff] [review]
gtk2 patch, v2

I guess this is ok, see my previous comment about code sharing though.
Attachment #160412 - Flags: review?(bryner) → review+

Comment 107

13 years ago
Hi, cursor from CSS is just perfect for our website. I just tried with smal
32x32 pixel PNG but it doesn't work in Firefox 1.0. What formats are currently
supported?
Not supported. The bug is not yet fixed.

Updated

13 years ago
Keywords: css3, qawanted
I'm not sure whether it's a closely related problem or not, but anyway there's a
problem to solve. Windows icons have a MIME type (image/x-icon or, more
standard-looking, image/vnd.microsoft.icon -- see
http://www.iana.org/assignments/media-types/image/vnd.microsoft.icon for more
details). However, there's probably no standard MIME for Windows cursors.

IIRC, Mozilla always preferred MIME-based image type guessing -- not
extension-based. (Here "extension" is "file name suffix", not "XUL overlay XPI".)

So are we going to break this usual preference, or are we going to introduce yet
another non-standard MIME type (e.g. image/x-mswindows-cursor), or are there any
already existent MIME type for cursors?

I have a strong feeling that it would be better to make this guess result
extension-based, since that's how the already existent MSIE implementation
works, and so we cannot expect most of webservers tuned to support something
unique as the MIME type used for cursors; quite the contrary, most possible of
all, it's usuallly set to something as mundane as application/octet-stream, or,
worse, text/plain.

If you feel that the above described problem is actually a separate bug, then
please file it as a separate bug in Bugzilla, and add this bug as a blocker of
that new bug, and add me to CC list of that new bug. Thank you.
>IIRC, Mozilla always preferred MIME-based image type guessing -- not
>extension-based.

content-based, actually. (if that doesn't give something useful, it falls back
to mimetype based. the extension, if any, is ignored). see also
http://www.mozilla.org/docs/web-developer/mimetypes.html#http

Comment 111

13 years ago
(In reply to comment #109)

AFAICS Mozilla's image handler recognizes the file type by file content, i.e. as
long as it is clear that the file must be an image (for example because it is
referred to as a shortcut icon or loaded from an html <img> element) the image
will be displayed properly, regardless of the MIME type sent by the server. I
think cursors would (or at least should) be handled in the same way.
Ok, so the server-given MIME type is irrelevant to the matter in hand. Fine.
Great. Now to some other issue.

The above given patches are not implementing ANI animated cursors for Windows.
And now we've got a reason not to hurry for it. Microsoft Windows Kernel is
vulnerable (and there's no patch except WinXP SP2), see details here:
http://www.securityfocus.com/archive/1/385340
However, LoadImage API for Windows also has unpatched vulnerabilities not only
for ANI (as described by URL given in my previous comment). The CUR files are
also affected: http://www.securityfocus.com/archive/1/385342
So, if the above given patches somehow (directly or, more likely, consequently)
use LoadImage() calls on Windows, then additional checks should be added unless
we want the whole system compromised by a maliciously composed CUR file.

However, I'm not enough skilled to see whether LoadImage() is used. Maybe it's a
false alarm after all.
people, we are not using LoadImage. we wouldn't use it for ANI either (at least,
not if I am to implement it).
Created attachment 169904 [details] [diff] [review]
content/layout patch, v5 (checked in)

updated per file move + bug 263671
Attachment #155496 - Attachment is obsolete: true
Created attachment 169908 [details] [diff] [review]
dom patch, v5 (checked in)

forgot to post this as part of the content/layout patch v5. it was included in
the previous versions though, so it was reviewed.
Attachment #154952 - Attachment description: xp widget part → xp widget part (checked in)
Attachment #169904 - Attachment description: content/layout patch, v5 → content/layout patch, v5 (checked in)
Comment on attachment 169908 [details] [diff] [review]
dom patch, v5 (checked in)

The cross-platform parts have been checked in (except imagelib).

note that there are no visible changes yet; waiting on review for the imagelib
patch.
Attachment #169908 - Attachment description: dom patch, v5 → dom patch, v5 (checked in)

Comment 119

13 years ago
This might be a stupid question, but will the cursor handling routines follow
the user's privacy settings regarding images, namely not to load external images
in mailnews, not to load images from third party servers, or not to load images
at all?
this will do the same as content:url() does now - it checks
nsContentUtils::CanLoadImage. this one does a contentpolicy check. looking at
the code, this will do the right thing as far as mailnews/remote images are
concerned, as well as for disabled images.
Depends on: 276692

Comment 121

13 years ago
The cross-platform change has broken the build under BeOS. It's an error about 
one of the BeOS files shadowing the new BaseWidget::SetCursor method (I guess 
we inherit the old SetCursor one but not the new one).

BeOS cursors are very limited - pixels are either black, white, or 
transparent, limited to 16x16 IIRC, and static (I think they chose a "low 
common denominator" so they could have hardware cursor support on as many 
machines as possible).

What's the best way to get it to build again? Override the method and return 
NS_ERROR_NOT_IMPLEMENTED, as in the BaseWidget implementation?
>It's an error about 
>one of the BeOS files shadowing the new BaseWidget::SetCursor method 

that should be just a warning, not an error... if adding a
SetCursor(imgIContainer*) that just returns NS_ERROR_NOT_IMPLEMENTED makes it
build again though, then yes, that would be an ok fix...

Comment 123

13 years ago
(In reply to comment #122)
> >It's an error about 
> >one of the BeOS files shadowing the new BaseWidget::SetCursor method 
> 
> that should be just a warning, not an error... if adding a
> SetCursor(imgIContainer*) that just returns NS_ERROR_NOT_IMPLEMENTED makes it
> build again though, then yes, that would be an ok fix...

It actually is a warning: the actual error is above that line. But anyway, what
harm can it do, that this method is 'hidden'? (What does it mean actually?)
Assignee: cbiesinger → dbaron
Status: ASSIGNED → NEW

Comment 124

13 years ago
Sorry for this mess of reassigning... I hate these laptop touchpad mice (left
mine at home).
Assignee: dbaron → cbiesinger

Updated

13 years ago
Status: NEW → ASSIGNED

Updated

13 years ago
Attachment #161501 - Flags: review?(pavlov) → review+
Attachment #161501 - Flags: superreview?(tor)

Comment 125

13 years ago
Comment on attachment 161501 [details] [diff] [review]
imagelib patch, v3

The CID should probably be updated.
Attachment #161501 - Flags: superreview?(tor) → superreview+
Created attachment 171061 [details] [diff] [review]
imagelib patch, v4 (checked in)

with the updated CID, and updated to trunk
Attachment #161501 - Attachment is obsolete: true
Created attachment 171828 [details] [diff] [review]
gtk2 patch, v3

this patch depends on bug 276692. re-requesting review since the patch now
looks quite a bit different.
Attachment #160412 - Attachment is obsolete: true
Attachment #171828 - Flags: review?(bryner)
Created attachment 171829 [details] [diff] [review]
windows patch, v6

due to popular demand, now with alpha support on winxp. the check does not
include win2k since the microsoft.com link I gave above claims it does not work
there.
Attachment #158930 - Attachment is obsolete: true
Attachment #171829 - Flags: review?(emaijala)

Comment 129

13 years ago
Comment on attachment 171829 [details] [diff] [review]
windows patch, v6

I didn't have time to really go throught it yet, but I have one request right
away: please make the variable names more readable. I can guess that parameter
w is the width, but aWidth would be so much better. Or iBpr.. I'd really like
aImageBytesPerRow better. I know it's longer, but it's oh so clean.

> osversion.dwMajorVersion > 5 || // Newer than Windows Server 2003

Are you sure the next version isn't 5.something?

Why are many of the functions static and not members of nsWindow?
Attachment #171829 - Flags: review?(emaijala) → review-
(In reply to comment #129)
> > osversion.dwMajorVersion > 5 || // Newer than Windows Server 2003
> 
> Are you sure the next version isn't 5.something?

Not actually... I should probably rephrase the comment.

> Why are many of the functions static and not members of nsWindow?

No good reason I guess... I didn't expect callers outside of nsWindow.cpp, and
this way the compiler can maybe generate better code.

I'll change it if you want.
Blocks: 246481
Depends on: 90213
Blocks: 90213
No longer depends on: 90213
Created attachment 172452 [details] [diff] [review]
windows patch, v7

now with static member functions and better variable names. I left
IsCursorTranslucencySupported as a global static for consistency with
IsAlphaTranslucencySupported.
Attachment #171829 - Attachment is obsolete: true
Attachment #172452 - Flags: review?(emaijala)

Comment 132

13 years ago
Comment on attachment 172452 [details] [diff] [review]
windows patch, v7

Image data is left locked in SetCursor if frame->GetAlphaBytesPerRow(&abpr)
fails.

+  // We will have 32 bpp, so bpr will be 4 * w

Make that bpr "bytes per row" and I'm happy.
Attachment #172452 - Flags: review?(emaijala) → review-
Created attachment 175953 [details] [diff] [review]
windows patch, v8

ok, made those two changes
Attachment #172452 - Attachment is obsolete: true
Attachment #175953 - Flags: review?(emaijala)
Having this bug fixed is an important step in the matter of CSS2 compliance (bug
1916), and also MSIE parity (bug 164421) in the hard times of forthcoming
MSIE7beta. So I am setting now blocking-? flags for the 1.8beta and aviary1.1
branch -- in hope that Moz1.8 and FF1.1 final releases could support cursor:
url(...).
Flags: blocking1.8b2?
Flags: blocking-aviary1.1?

Comment 135

13 years ago
Comment on attachment 175953 [details] [diff] [review]
windows patch, v8

r=emaijala
Attachment #175953 - Flags: review?(emaijala) → review+
Attachment #175953 - Flags: superreview?(roc)
Comment on attachment 175953 [details] [diff] [review]
windows patch, v8

> alpharow

Make it alphaRow

+	   // Make up an opaque alpha channel
+	   PRUint32 abpr = ((width / 8) + 3) & ~3;
+	   PRUint8* opaque = (PRUint8*)malloc(abpr * height);
+	   if (opaque) {
+	     memset(opaque, 0xff, abpr * height);
+
+	     hAlpha = DataToBitmap(opaque, width, height, 1);
+	     free(opaque);
+	   }

Factor this into its own static function
Attachment #175953 - Flags: superreview?(roc) → superreview+
Blocks: 82130
Depends on: 285048
Created attachment 176514 [details] [diff] [review]
windows patch, final
Attachment #175953 - Attachment is obsolete: true
When will it be checked in? I am eager to try this improvement, even if in a
nightly build!
"soon" :) a recent unrelated checkin made it necessary to fix bug 285048,
because otherwise, certain cursors would display very wrong. so, I'm waiting for
review on the patch there.
Comment on attachment 171828 [details] [diff] [review]
gtk2 patch, v3

Just a couple of small things:

>--- widget/src/gtk2/nsWindow.cpp	19 Jan 2005 02:38:58 -0000	1.127
>+++ widget/src/gtk2/nsWindow.cpp	20 Jan 2005 01:49:17 -0000
>@@ -33,12 +33,14 @@
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
>+#include <prlink.h>
>+

We generally use "" instead of <> for NSPR includes.

> NS_IMETHODIMP
>+nsWindow::SetCursor(imgIContainer* aCursor)
>+{
>+    if (!sPixbufCursorChecked) {
>+        PRLibrary* lib;
>+        _gdk_cursor_new_from_pixbuf = (_gdk_cursor_new_from_pixbuf_fn)
>+            PR_FindFunctionSymbolAndLibrary("gdk_cursor_new_from_pixbuf", &lib);
>+        sPixbufCursorChecked = PR_TRUE;
>+    }
>+    if (!_gdk_cursor_new_from_pixbuf)
>+        return NS_ERROR_NOT_IMPLEMENTED;
>+
>+    // if we're not the toplevel window pass up the cursor request to
>+    // the toplevel window to handle it.
>+    if (!mContainer && mDrawingarea) {
>+        GtkWidget *widget =
>+            get_gtk_widget_for_gdk_window(mDrawingarea->inner_window);
>+        nsWindow *window = get_window_for_gtk_widget(widget);
>+        return window->SetCursor(aCursor);
>+    }

Can you move this toplevel window check up to the beginning of the method? 
That way we don't recheck sPixbufCursorChecked / _gdk_cursor_new_from_pixbuf.

Looks fine otherwise.
Attachment #171828 - Flags: review?(bryner) → review+
Created attachment 176749 [details] [diff] [review]
gtk2 patch, v4

transferring r=bryner
Attachment #171828 - Attachment is obsolete: true
Attachment #176749 - Flags: superreview?(roc)
Attachment #176749 - Flags: review+
Comment on attachment 176749 [details] [diff] [review]
gtk2 patch, v4

+    if (cursor) {

If this fails, shouldn't you unref the pixbuf?
Attachment #176749 - Flags: superreview?(roc) → superreview+
windows patch checked in
Checking in widget/src/windows/Makefile.in;
/cvsroot/mozilla/widget/src/windows/Makefile.in,v  <--  Makefile.in
new revision: 3.24; previous revision: 3.23
done
Checking in widget/src/windows/nsWindow.cpp;
/cvsroot/mozilla/widget/src/windows/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 3.544; previous revision: 3.543
done
Checking in widget/src/windows/nsWindow.h;
/cvsroot/mozilla/widget/src/windows/nsWindow.h,v  <--  nsWindow.h
new revision: 3.198; previous revision: 3.197
done

Created attachment 177542 [details] [diff] [review]
gtk2 patch, final

roc, thanks for catching that. I actually need to always unref the pixbuf.
Attachment #176749 - Attachment is obsolete: true
bugs filed:
Bug 286303 for the CSS 3 syntax (specifying the hotspot in CSS)
Bug 286304 for implementing this on mac
Bug 286306 for OS/2
Bug 286307 for QNX
Bug 286309 for Xlib
Bug 286310 for Qt.

gtk2:
Checking in widget/src/gtk2/Makefile.in;
/cvsroot/mozilla/widget/src/gtk2/Makefile.in,v  <--  Makefile.in
new revision: 1.37; previous revision: 1.36
done
Checking in widget/src/gtk2/nsWindow.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.133; previous revision: 1.132
done
Checking in widget/src/gtk2/nsWindow.h;
/cvsroot/mozilla/widget/src/gtk2/nsWindow.h,v  <--  nsWindow.h
new revision: 1.52; previous revision: 1.51
done
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago13 years ago
Resolution: --- → FIXED

Comment 146

13 years ago
I'll try to get an XBM patch up this weekend.
Created attachment 177555 [details] [diff] [review]
bustage fix for gtk 2.0 builds (checked in)

this is the fix I had to checkin for the bustage on the egg tinderbox, which is
running gtk 2.0.6, which lacks GdkDisplay*.

Comment 148

13 years ago
Getting the following error on Win32:-

e:/mozilla/source/mozilla/widget/src/windows/nsWindow.cpp: In function `PRBool I
sCursorTranslucencySupported()':
e:/mozilla/source/mozilla/widget/src/windows/nsWindow.cpp:181: error: ISO C++ fo
rbids declaration of `didCheck' with no type
e:/mozilla/source/mozilla/widget/src/windows/nsWindow.cpp:182: error: ISO C++ fo
rbids declaration of `isSupported' with no type
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Please don't reopen bugs for build bustage unless the checkin is backed out. 
(The reopening also triggers a cascade of bugmail for the dependencies.)

Comment 150

13 years ago
Sorry, I keep forgetting about that policy change.

I'll close this bug and open a new one.

Updated

13 years ago
Blocks: 286386

Comment 151

13 years ago
New bug created. Bug #286386 to cover regression from this bug.
No longer blocks: 286386
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED

Updated

13 years ago
Blocks: 163068
So this doesn't actually work for me on GTK2 because I hit this early return in
widget/src/gtk2/nsWindow.cpp's SetCursor(imgIContainer*):

    nsCOMPtr<nsIGdkPixbufImage> pixImg(do_GetInterface(frame));
    if (!pixImg)
        return NS_ERROR_NOT_AVAILABLE;

I don't actually see how this could work.  gfxImageFrame only allows
GetInterface to nsIImage.  Or did somebody have a patch in their tree to remove
that check in gfxImageFrame.cpp?
Changing the above line:

-    nsCOMPtr<nsIGdkPixbufImage> pixImg(do_GetInterface(frame));
+    nsCOMPtr<nsIGdkPixbufImage>
pixImg(do_QueryInterface(nsCOMPtr<nsIImage>(do_GetInterface(frame))));

does fix the problem for me.
(In reply to comment #152)
> Or did somebody have a patch in their tree to remove
> that check in gfxImageFrame.cpp?

yes, I do have that in my tree... and I intended to land it as part of bug
276692, see bug 276692 comment 4 and bug 276692 comment 5; I apparently forgot
to diff gfx/shared in that patch :( I'll land that later today.

Comment 155

13 years ago
Hmmm... is Win9x platform supported?

Testcase (the 'URL') does nothing in my environment (WinMe, beast-trunk build).
Mozilla/5.0 (Windows; U; Win 9x 4.90; ja-JP; rv:1.8b2) Gecko/20050317 Firefox/1.0+


And testcase below (written in Japanese, 3rd <table>, 1st <li> is the testcase
of 'cursor:url()') also does nothing, and errors are shown in JavaScript console.
http://www5e.biglobe.ne.jp/~access_r/hp/css/css_cursor_001.html

> Error: Error in parsing value for property 'cursor'.  Declaration dropped.
> Source File: http://www5e.biglobe.ne.jp/~access_r/hp/css/css_cursor_001.html
> Line: 9

This testcase works with WinIE6.

This error message is shown when declaration is written as 'cursor: url()'.
Modify this to 'cursor: url(), ...', no errors are shown.
(In reply to comment #155)
> Hmmm... is Win9x platform supported?

should be, but I have no win9x system to test.

> Testcase (the 'URL') does nothing in my environment (WinMe, beast-trunk build).
> Mozilla/5.0 (Windows; U; Win 9x 4.90; ja-JP; rv:1.8b2) Gecko/20050317 Firefox/1.0+

:/ guess I'll need to find a win9x system and do some debugging. none of the
cursors there work? Can you file a new bug on that?

> > Error: Error in parsing value for property 'cursor'.  Declaration dropped.

That error is correct. You always need one of the cursor keywords. See the
grammar at http://www.w3.org/TR/CSS21/ui.html#cursor-props
ah, that the cursors in the testcase field don't work on win9x could be due to:
Windows 95/98/Me: The width and height of the cursor must be the values returned
by the GetSystemMetrics function for SM_CXCURSOR and SM_CYCURSOR.

the testcase there uses cursors of sizes that are, hm, not very likely to
correspond to those values ;)

Comment 158

13 years ago
(In reply to comment #156)
> (In reply to comment #155)
> > Hmmm... is Win9x platform supported?
> 
> should be, but I have no win9x system to test.
> 
> > Testcase (the 'URL') does nothing in my environment (WinMe, beast-trunk build).
> > Mozilla/5.0 (Windows; U; Win 9x 4.90; ja-JP; rv:1.8b2) Gecko/20050317
Firefox/1.0+
> 
> :/ guess I'll need to find a win9x system and do some debugging. none of the
> cursors there work? Can you file a new bug on that?
Ok. I've filed Bug 286622.


> > > Error: Error in parsing value for property 'cursor'.  Declaration dropped.
> 
> That error is correct. You always need one of the cursor keywords. See the
> grammar at http://www.w3.org/TR/CSS21/ui.html#cursor-props
And this is Bug 286624 - for IE-compatible.

Comment 159

13 years ago
One of the features that this fix allows for is specification of custom cursors
in usercontent.css for specific link types (e.g. javascript links). 
Unfortunately one cannot specify file:/// type URIs for cursors due to security
restrictions.  These restrictions limit the functionality of this fix.  Seems to
me that there should be some way to allow for file:/// uri cursors if they are
specified in usercontent.css rather than by the page content.

I realize that this issue is probably well outside the scope of this bug, but I
think it is very relevant to it.
Comment 159 should have been a new bug on file, or a reference to an existing
bug asking for usercontent.css-specified cursors.

/be
I'm sure that the same issue as for file:// cursors in userContent.css exists
for -moz-binding, list-style-image, background-image etc., and a fix should
address all of those.
(In reply to comment #156)
> > > Error: Error in parsing value for property 'cursor'.  Declaration dropped.
> 
> That error is correct. You always need one of the cursor keywords. See the
> grammar at http://www.w3.org/TR/CSS21/ui.html#cursor-props

Because there are so few sites out there that actually document this correctly
because IE sucks, I filed some Tech Evangelism bugs for some sites I found while
checking this.  Do a search for bugs containing "[invalid-css-cursor]" in the
Status Whiteboard to find them, and feel free to file bugs for sites that
incorrectly document this and add "[invalid-css-cursor]" to the Status Whiteboard.

Comment 163

13 years ago
I have a patch for XBM cursor hotspots.  However, it seems that the hotspot does
not work in the first place.  I tested this functionality with both the CUR code
as well as my XBM code and it always sets the hotspot at the top left corner. 
Should I open a bug for that?

Comment 164

13 years ago
Ignore that last comment, I didn't have a good cursor file.
(In reply to comment #152)
> So this doesn't actually work for me on GTK2

I now checked in a fix to make this work.
Target Milestone: mozilla1.8alpha5 → mozilla1.8beta2

Updated

13 years ago
Flags: blocking1.8b2?
Flags: blocking-aviary1.1?
Blocks: 296191
looks like I forgot to file the beos version of the bug before, here it is: Bug
298184

Comment 167

12 years ago
I can't get this to work in Firefox nightly 20050621 on Windows XP with SP2.
I tried the japanese testpage and I tried it on my page:
http://jlp.homelinux.org/~jlp/projlab2004/galerija.html
(a custom spyglass cursor should show up over images)

Comment 168

12 years ago
The cursor referenced on that page does not exist on the server, so of course it
doesn't work.
Status: RESOLVED → VERIFIED
that page wfm with suite nightly 2005061905. I guess the missing cursor
mentioned in comment 168 got added in the meantime...

Comment 170

12 years ago
Yup, it works just fine now. I forgot that i changed the case of the first
letter but didn't update the CSS file. I feel quite stupid for spaming because
of this. Anyways, thanks to all for making it possible!

Comment 171

12 years ago
(In reply to comment #160)
> Comment 159 should have been a new bug on file, or a reference to an existing
> bug asking for usercontent.css-specified cursors.

See bug 310165.
You need to log in before you can comment on or make changes to this bug.