38447 - Implement Handling of URI Values on CSS "cursor" Properties

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P4)

Description

[Michael Lowe]

This bug has been broken off from bug #1916, which is now a meta bug about 
CSS2/3 cursor property support.

Comment 1

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

Hmmm... what cursor formats should be accepted?  Are there any good XP cursor
formats?

Comment 2

[Hixie (not reading bugmail)]

SVG?

Comment 3

[Pierre Saslawsky]

Resolved as Later. It raises about the same problems as downloadable fonts and 
has nowhere near the same usefulness.

Comment 4

[Hixie (not reading bugmail)]

By which of course, you meant "Milestone: Future". Right?

Comment 5

[Pierre Saslawsky]

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.

Comment 6

[Hixie (not reading bugmail)]

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

Comment 7

[Hixie (not reading bugmail)]

Despite comments above, LATER is deprecated, so reopening and moving to Future.

Comment 8

[Skewer]

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.

Comment 9

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

Recognizing a platform-specific cursor format for web pages (which should be
platform-independent) is a Bad Thing (TM).

Comment 10

[Skewer]

dbaron: What platform-independent cursor format are you thinking of, then?

Comment 11

[Hixie (not reading bugmail)]

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

[Skewer]

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

[Tim Powell]

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

[Skewer]

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

[Skewer]

Needs a dev relnote.

Comment 16

[Greg K.]

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

[Greg K.]

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

[Greg K.]

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

Comment 19

[Skewer]

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

Comment 20

[David Hyatt]

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

Comment 21

[Pierre Saslawsky]

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/

Comment 22

[Alfonso Martinez]

*** Bug 114938 has been marked as a duplicate of this bug. ***

Comment 23

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

Assigning pierre's remaining Style System-related bugs to myself.

Comment 24

[Chris Casciano]

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

[Stanimir Stamenkov]

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

[Chris Casciano]

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

[Skewer]

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.

Comment 28

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** Bug 177193 has been marked as a duplicate of this bug. ***

Comment 29

[Zack]

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 30

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

comment 29 is the wrong syntax, but the right idea.  See bug 77974.

Comment 31

[Xanthor Sccas]

Why don't we use formats already supported by Mozilla, like for the favicon
(where .ico, PNG, GIF, MNG... are accepted) ?

Comment 32

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

Because Mozilla doesn't draw the cursor itself -- the OS draws the cursor.

Comment 33

[Martin Heidegger]

stupid question:

hiding os cursor and painting own cursor ?

Comment 34

[Bob]

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

[chris]

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.

Comment 36

[chris]

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.

Comment 37

[Sergey Sokoloff]

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

[chris]

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

[Anne (:annevk)]

> 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

[chris]

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

[Anne (:annevk)]

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

Comment 42

[Jacek Piskozub]

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

[chris]

(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

[Prognathous]

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.

Comment 45

[Christian :Biesinger (don't email me, ping me on IRC)]

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?

Comment 46

[Brendan Eich [:brendan]]

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

[Anne (:annevk)]

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

Comment 48

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

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.

Comment 49

[Christian :Biesinger (don't email me, ping me on IRC)]

(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

[chris]

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

[chris]

"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

[chris]

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.

Comment 53

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 54

[Brendan Eich [:brendan]]

[e-mail], 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

Comment 55

[Uwe Schröder]

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

Comment 56

[Sergey Sokoloff]

Attachment: 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! ;-)

Comment 57

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 58

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: imagelib part

Comment 59

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: xp widget part (checked in)

interface changes for widget/ + stub impl in nsBaseWidget

Comment 60

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: 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.

Comment 61

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 62

[Stuart Parmenter]

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?

Comment 63

[Sergey Sokoloff]

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

[Aaron Kaluszka]

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

Comment 65

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 66

[Christian :Biesinger (don't email me, ping me on IRC)]

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

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

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.

Comment 68

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 69

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: content/layout changes, v2

- unrelated changes removed
- using const nsPoint& in the GetCursor signature
- make sure never to add null requests to nsStyleUserInterface::mCursorArray

Comment 70

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: windows part

Comment 71

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 155390 [details] [diff] [review]
windows part

oops, this mishandles images w/ 1-bit transparency (.gif)

Comment 72

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: windows patch, v2

now supports opaque cursors too, and gifs as cursors. also, checks allocations
for failure.

Comment 73

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: 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...

Comment 74

[Aaron Kaluszka]

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.

Comment 75

[Christian :Biesinger (don't email me, ping me on IRC)]

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

[Boris Zbarsky [:bzbarsky]]

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

Comment 77

[Boris Zbarsky [:bzbarsky]]

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

Comment 78

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 79

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: content/layout changes, v3

Comment 80

[Sergey Sokoloff]

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

Comment 81

[Christian :Biesinger (don't email me, ping me on IRC)]

That is correct. Animated gifs are not supported either.

Comment 82

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

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 83

[Christian :Biesinger (don't email me, ping me on IRC)]

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)

Comment 84

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: 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.

Comment 85

[Boris Zbarsky [:bzbarsky]]

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

Looks good.  r+sr=bzbarsky

Comment 86

[Ere Maijala (slow)]

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?

Comment 87

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: windows patch, v3

fixes ere's comments. also, updated to trunk (previous patch doesn't apply
cleanly)

Comment 88

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

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

Comment 89

[Anne (:annevk)]

This patch will only implement the CSS 2.1 version as mentioned in comment 60,
correct?

Comment 90

[Christian :Biesinger (don't email me, ping me on IRC)]

yes, that is correct.

Comment 91

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 92

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 93

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: 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.

Comment 94

[Stuart Parmenter]

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

Comment 95

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: 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.

Comment 96

[Christian :Biesinger (don't email me, ping me on IRC)]

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 97

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

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?

Comment 98

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 99

[Worcester12345]

Should the target milestone be moved since it has passed?

Comment 100

[Christian :Biesinger (don't email me, ping me on IRC)]

how does that matter. but ok, doing it just for you.

Comment 101

[Worcester12345]

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

Comment 102

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 103

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: 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.

Comment 104

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: imagelib patch, v3

not going to attach an updated gtk patch just yet, the change is trivial anyway

Comment 105

[Brian Ryner (not reading)]

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 106

[Brian Ryner (not reading)]

Comment on attachment 160412 [details] [diff] [review]
gtk2 patch, v2

I guess this is ok, see my previous comment about code sharing though.

Comment 107

[Jure Repinc (JLP)]

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?

Comment 108

[Sergey Sokoloff]

Not supported. The bug is not yet fixed.

Comment 109

[Sergey Sokoloff]

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.

Comment 110

[Christian :Biesinger (don't email me, ping me on IRC)]

>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

[Uwe Schröder]

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

Comment 112

[Sergey Sokoloff]

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

Comment 113

[Sergey Sokoloff]

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

Comment 114

[Sergey Sokoloff]

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.

Comment 115

[Christian :Biesinger (don't email me, ping me on IRC)]

people, we are not using LoadImage. we wouldn't use it for ANI either (at least,
not if I am to implement it).

Comment 116

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: content/layout patch, v5 (checked in)

updated per file move + bug 263671

Comment 117

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: 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.

Comment 118

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 119

[Uwe Schröder]

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?

Comment 120

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 121

[Simon]

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?

Comment 122

[Christian :Biesinger (don't email me, ping me on IRC)]

>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

[Niels Reedijk]

(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?)

Comment 124

[Niels Reedijk]

Sorry for this mess of reassigning... I hate these laptop touchpad mice (left
mine at home).

Comment 125

[tor]

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

The CID should probably be updated.

Comment 126

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: imagelib patch, v4 (checked in)

with the updated CID, and updated to trunk

Comment 127

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: gtk2 patch, v3

this patch depends on bug 276692. re-requesting review since the patch now
looks quite a bit different.

Comment 128

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: 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.

Comment 129

[Ere Maijala (slow)]

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?

Comment 130

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 131

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: windows patch, v7

now with static member functions and better variable names. I left
IsCursorTranslucencySupported as a global static for consistency with
IsAlphaTranslucencySupported.

Comment 132

[Ere Maijala (slow)]

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.

Comment 133

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: windows patch, v8

ok, made those two changes

Comment 134

[Sergey Sokoloff]

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

Comment 135

[Ere Maijala (slow)]

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

r=emaijala

Comment 136

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

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

Comment 137

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: windows patch, final

Comment 138

[Sergey Sokoloff]

When will it be checked in? I am eager to try this improvement, even if in a
nightly build!

Comment 139

[Christian :Biesinger (don't email me, ping me on IRC)]

"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 140

[Brian Ryner (not reading)]

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.

Comment 141

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: gtk2 patch, v4

transferring r=bryner

Comment 142

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

Comment on attachment 176749 [details] [diff] [review]
gtk2 patch, v4

+    if (cursor) {

If this fails, shouldn't you unref the pixbuf?

Comment 143

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 144

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: gtk2 patch, final

roc, thanks for catching that. I actually need to always unref the pixbuf.

Comment 145

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 146

[Aaron Kaluszka]

I'll try to get an XBM patch up this weekend.

Comment 147

[Christian :Biesinger (don't email me, ping me on IRC)]

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

[David G King]

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

Comment 149

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

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

[David G King]

Sorry, I keep forgetting about that policy change.

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

Comment 151

[David G King]

New bug created. Bug #286386 to cover regression from this bug.

Comment 152

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

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?

Comment 153

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

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.

Comment 154

[Christian :Biesinger (don't email me, ping me on IRC)]

(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

[Anbo Motohiko]

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.

Comment 156

[Christian :Biesinger (don't email me, ping me on IRC)]

(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

Comment 157

[Christian :Biesinger (don't email me, ping me on IRC)]

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

[Anbo Motohiko]

(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

[Dan]

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 160

[Brendan Eich [:brendan]]

Comment 159 should have been a new bug on file, or a reference to an existing
bug asking for usercontent.css-specified cursors.

/be

Comment 161

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 162

[Jeff Walden [:Waldo]]

(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

[Aaron Kaluszka]

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

[Aaron Kaluszka]

Ignore that last comment, I didn't have a good cursor file.

Comment 165

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 166

[Christian :Biesinger (don't email me, ping me on IRC)]

looks like I forgot to file the beos version of the bug before, here it is: Bug
298184

Comment 167

[Jure Repinc (JLP)]

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

[Aaron Kaluszka]

The cursor referenced on that page does not exist on the server, so of course it
doesn't work.

Comment 169

[Christian :Biesinger (don't email me, ping me on IRC)]

that page wfm with suite nightly 2005061905. I guess the missing cursor
mentioned in comment 168 got added in the meantime...

Comment 170

[Jure Repinc (JLP)]

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

[zug_treno]

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