286622 - URI values on "cursor" properties does not work on Win9x

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Win32, defect)

Description

[Anbo Motohiko]

User-Agent:       Mozilla/5.0 (Windows; U; Win 9x 4.90; ja-JP; rv:1.8b2) Gecko/20050317 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Win 9x 4.90; ja-JP; rv:1.8b2) Gecko/20050317 Firefox/1.0+

Since Bug 38447 is fixed, we can use URI values on cursor properties.
But testcase (the 'URL', same as Bug 38447) doesn't work in my WinMe environment.

Reproducible: Always

Steps to Reproduce:
1. open 'URL'.
2. move mouse cursor on the strings.
3.

Actual Results:  
Nothing ... sometimes black square flashes.
Mouse cursor on 'working cursor' and 'FFox png' is 'default', rest is 'pointer'.

Expected Results:  
User-defined mouse cursors are shown.

See also cbiesinger's comment on Bug 38447 comment #157.

Comment 1

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

-> me, for now; I don't currently have a win9x system though, so if anyone wants
to debug this, feel free to take ;)

Comment 2

[AKIHIRO Misaki (a.k.a Kuden)]

User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050317
Firefox/1.0+

I am using Win98SE. 
The image specified only momentarily is only displayed very unusually even if
the mouse cursor is passed on the place where cursor was specified. 

Comment 3

[ArronM (:paper)]

The answer is to not destroy the icon after ::SetCursor()
Which means we need to store the HCURSOR and destroy it later.

I have a patch, but it's intertwined with the Win2k fix for cursors.  Patch
later, unless biesi beats me to a patch.  :)

It also appears we can't display the big images as cursors on Win98.  Haven't
looked deeply into it yet, though.

Comment 4

[ArronM (:paper)]

Attachment: Fixup Cursors for Win9x/XP/2k (WIP)

This is more or less the patch.  I just have to cleanup a bit by exiting
DataToBitmap() if a 8bit depth is sent (doesn't currently happen anymore, but
should be checked for future use), and adjusting the size of reserved_space. 
Maybe a few more comments too.

But, before (or while) I do that, could I get someone to check if the cursors
still work on XP with this patch?  biesi?

Short version of what the patch does:
- Doesn't destroy icon after setcursor (Win9x problem #1)
- Force mask to always be 1 bit.  32bit RGBA images overide or combine with
mask for those platforms (2k, XP, others?) and video modes (32bit) that support
it.


On a related note, is it just me, or does the cursor get built from the image
everytime the mouse moves?  Looks like it could use some optimizing.

Comment 5

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

patch works fine for me on windows xp, both in 16 bit and 32 bpp color modes.

> On a related note, is it just me, or does the cursor get built from the image
> everytime the mouse moves?  Looks like it could use some optimizing.

that is indeed what happens; now that you cache the HCURSOR, maybe you should
also cache the imgIContainer used to build it and only rebuild if different.
(but always call SetCursor).

Comment 6

[ArronM (:paper)]

Answering some questions while I was away on IRC:

> <biesi> Paper, so... what do you need Data8BitTo1Bit for?

Win9x requires the mask to be 1 bit.

> <biesi> Paper, and what's wrong with ::GetDC(NULL) on 9x?

It messes up the 1 bit mask, for some really odd reason.

Both things were solved via trial and error.  Searching the net for cursor
documentation was very useless.

I'll add an imgIContainer cache to the next patch.

Comment 7

[ArronM (:paper)]

> > <biesi> Paper, so... what do you need Data8BitTo1Bit for?
> Win9x requires the mask to be 1 bit.

Non 32-bit modes needed the mask to be 1 bit too, at least on Win2k.

Comment 8

[ArronM (:paper)]

Attachment: Fixup Cursors for Win9x/XP/2k

Patch does the following

1) Displays standard sized icons in Win9x by removing ::DestroyIcon.  Cursor is
cached and destroyed later (when cursor is set again, or when window is
destroyed)

2) Fixes incorrect (or nonexistant) trasparency cursor problems on Win9x/2k. 
Cursor mask forced down to 1 bit, CreateCompatibleDC used instead of GetDC.

3) Cache cursor's imgIContainer. Force a SetCursor and exit early if the image
is the same.


Extra note on #1. Win9x/ME/2k may or may not display icons that aren't the
proper "icon size".  At least on 2k, it's up to the video driver (In 32 bit
mode, my nVidia card displays the the full mozilla banner as an icon, but crops
the sun image to 64x64, while keeping it's 8 bit alpha data. In 16 bit mode, it
displays both images fully, but only in 1 bit alpha).  It might be wise in the
future to resize the icon to the "standard size" on Win9x, providing
CreateCursorIndirect returns a fail state (I haven't tested if it does).  That
would be another bug, IMO. Something like "Win9x doesn't display icons of
non-standard sizes", marked as an enhancement.

Comment 9

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

Comment on attachment 178212 [details] [diff] [review]
Fixup Cursors for Win9x/XP/2k

(could you diff with -p in the future?)

looks good. I wish it was documented that people need not DestroyCursor when
they have called SetCursor.

+    if (NS_FAILED(SetCursor(eCursor_standard)))
+      ::DestroyIcon(mHCursor);

So... even if the new SetCursor fails... won't windows eventually destroy the
old cursor that was set? clearly, _something_ will set a new cursor. why will
this not double-destroy it?

What about here:
     HCURSOR oldCursor = ::SetCursor(newCursor);
+    
+    if (mHCursor) {
+      mCursorImgContainer = nsnull;
+      ::DestroyIcon(mHCursor);

didn't you write SetCursor will destroy previously-set cursors?

+  PRUint8 alphaPixels = 0;
+  PRUint8 offset = 7;

why not move those inside the outer for-loop? You are resetting it anyway on
each iteration...

+  if (aDepth == 8 || aDepth == 4) {
+    NS_WARNING("nsWindow::DataToBitmap can't handle 4 or 8 bit images");

can it handle 16 bit images?

+  HBITMAP tBitmap = ::CreateBitmap(1,1,planes,bpp,NULL);
+  HBITMAP oldbits = (HBITMAP)::SelectObject(dc,tBitmap);

code style seems to be a space after the comma (for example in the lines you
added above these two?)

However, what's wrong with CreateCompatibleBitmap?

-  head.biClrUsed = (aDepth == 1) ? 2 : 0;
+  head.biClrUsed = 0;

this works?

+  // XXX future, when we have a GetIsImageComplete(). (nsIImage has one, it
just
+  // needs to be propogated to gfxIImageFrame)

I note that the only current caller will not pass incomplete images here. but I
guess other callers could be added at some point.
(If the image is incomplete, shouldn't we set only a part of it as cursor?)

+  if (mHCursor != NULL)
+    ::DestroyIcon(mHCursor);

again, not sure this is correct...

+   * @param aWidth	       Width of the alpha data, in pxxels

"pixels"?

Comment 10

[ArronM (:paper)]

Attachment: Fixup Cursors for Win9x/XP/2k

(In reply to comment #9)
> (could you diff with -p in the future?)
An awesome feature. I'll definitely be using it from now on.

> So... even if the new SetCursor fails... won't windows eventually destroy the

> old cursor that was set? clearly, _something_ will set a new cursor. why will

> this not double-destroy it?

> didn't you write SetCursor will destroy previously-set cursors?

Windows/SetCursor doesn't destroy the previous cursor.	If you are referring to
Comment #8, point 1, I'm saying that the patch's code destroys the cursor when
a new cursor is set or the nsWindow is destroyed.


> +  PRUint8 alphaPixels = 0;
> +  PRUint8 offset = 7;
> 
> why not move those inside the outer for-loop? You are resetting it anyway on
> each iteration...

Changed.  I was initially thinking it would save a few cycles on images that
are divisable by 8.


> can it handle 16 bit images?

Yes, in theory.  16 bit images don't use a palette.  None of our decoders
return a 16 bit image, so I can't really test it without way too much work.


> +  HBITMAP tBitmap = ::CreateBitmap(1,1,planes,bpp,NULL);
> +  HBITMAP oldbits = (HBITMAP)::SelectObject(dc,tBitmap);
> However, what's wrong with CreateCompatibleBitmap?

CreateBitmap is used to force the DC to the correct bpp. 
CreateCompatibleBitmap doesn't force anything.	On Win98, if I
CreateCompatibleDC, I get a monochrome DC.  CreateCompatibleBitmap would create
a monochrome bitmap, where CreateBitmap changes the DC.  Since I don't trust
the original state of the DC, I force it to change to monochrome too.


> +  head.biClrUsed = 0;
> this works?

Yes.  If biClrUsed is 0, all colors are used based on biBitCount.

> +  // XXX future, when we have a GetIsImageComplete(). (nsIImage has one, it
> I note that the only current caller will not pass incomplete images here. but
I
> guess other callers could be added at some point.
> (If the image is incomplete, shouldn't we set only a part of it as cursor?)

A good question that I don't dare answer.  I can remove the comment if you
want.
It's more of a reference for me (or whoever) that there is in fact a way to
determine completeness, it's just not propogated yet.

Fixed spelling and lack of comma spacing.

Comment 11

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

Comment on attachment 178554 [details] [diff] [review]
Fixup Cursors for Win9x/XP/2k

>If you are referring to
Comment #8, point 1, I'm saying that the patch's code destroys the cursor when
>a new cursor is set or the nsWindow is destroyed.

Oh, ok. I clearly misunderstood that comment.

>Yes, in theory.  16 bit images don't use a palette.  None of our decoders
>return a 16 bit image, so I can't really test it without way too much work.

ok, you don't have to set the format to BI_BITFIELDS for that to work?

>I can remove the comment if you want.

nah, leave it there

Comment 12

[ArronM (:paper)]

(In reply to comment #11)
> >Yes, in theory.  16 bit images don't use a palette.  None of our decoders
> >return a 16 bit image, so I can't really test it without way too much work.
> 
> ok, you don't have to set the format to BI_BITFIELDS for that to work?

Close. You set the format to BI_BITFIELDS when you want to use a pallette,
otherwise, if it's BI_RGB, the data is in 5 bit RGB (padded to 16 bits)

Comment 13

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

How do you know that mCursorImgContainer isn't going to be destroyed while
you're holding a pointer to it?

+  (::GetSystemMetrics).  We try anyway, because some video drivers handle it 
+  ok, although results vary from displaying the whole image, scaling the image,
+  displaying part of the image, to not displaying the image at all.

Is this the right thing to do? I think it would be better for Web developers for
cursors to work as reliably as possible even if it means forcing the cursor to
be scaled down on pre-WinXP OSes.

Comment 14

[ArronM (:paper)]

(In reply to comment #13)
> How do you know that mCursorImgContainer isn't going to be destroyed while
> you're holding a pointer to it?

It doesn't matter if it's destroyed.  mCursorImgContainer isn't used to access
the imgContainer, just to check if our current mHCursor was built from it.

> +  (::GetSystemMetrics).  We try anyway, because some video drivers handle it 
> +  ok, although results vary from displaying the whole image, scaling the image,
> +  displaying part of the image, to not displaying the image at all.
> 
> Is this the right thing to do? I think it would be better for Web developers for
> cursors to work as reliably as possible even if it means forcing the cursor to
> be scaled down on pre-WinXP OSes.

I don't think scaling down is the answer.  The answer, according to the CSS 2.1
spec, would be "if the user agent cannot handle the first cursor of a list of
cursors, it should attempt to handle the second, etc." (
http://www.w3.org/TR/CSS21/ui.html#cursor-props )

What we really should be doing is moving to the next item on the list when
SetCursor returns NS_ERROR_FAILURE.  Currently, we don't.  I don't think
implementing cursor lists is in the scope of this bug.

The problem with some video drivers not displaying the cursor correctly is a bug
on the part of the video driver.  It should be returning null when we call
CreateIconIndirect, so we can safely exit with NS_ERROR_FAILURE.  This happens
correctly on Win98 when the icon isn't 32x32 (GetSystemMetrics).

From the web developer's perspective, if they plan on changing to the cursor to
a non-standard size, they should be providing an alternate cursor image that is
a standard size.  Perhaps even multiple ones, from largest to smallest (ie.
200x80 "funky original", 64x64, 48x48, 32x32, 16x16, and finally 'pointer' as
default)

The comments you quoted from the code are slightly misleading.  I'm actually
tempted to remove them.  Or maybe replace them with the paragraph starting with
"The problem"?  Depends on whether you agree with my arguement, I suppose. ;)

Comment 15

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

What if the container is destroyed and then a new one is created at the same
address?

You may be right about moving to the next frame in the list, but it's arguable.
But as you say, moving to a cursor list is beyond the scope of this bug. So I
suggest it's easier to just scale when we know we might need to.

> The problem with some video drivers not displaying the cursor correctly is a
> bug on the part of the video driver.  It should be returning null when we call
> CreateIconIndirect, so we can safely exit with NS_ERROR_FAILURE.

But do they? Because if we can't be sure they really do this right, then we need
to be proactive and scale or move onto the next cursor.

Comment 16

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

>How do you know that mCursorImgContainer isn't going to be destroyed while
>you're holding a pointer to it?

oops. for some reason I was thinking it was a nsCOMPtr while reviewing the
patch. it seems to be safer to actually make it one.

Comment 17

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

Also I think it's more developer friendly to scale the cursors if necessary than
to force them to provide one variant for every size a platform might require.

Comment 18

[ArronM (:paper)]

Our scaling will look like crap compared to anyone with a paint program with
bilinear resizing.  Also, the web developer that *does* want to create seperate
icon sizes will be unable too.  They will complain that Gecko doesn't adhear to
CSS 2.1 because the browsers scales their first image instead of picking one
that it actually supports.

Comment 19

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

Then what we should do is scan the list twice, once without scaling, and then if
no suitable cursor is found, we should rescan allowing scaling.

I'll relent on this scaling issue for now, but we will need to address it to
make this useful. We should still not be trying to set a cursor with a size that
we know is unreliable on popular video cards/drivers (unless we are sure we can
reliably detect the failure). And there's still the problem with
mCursorImgContainer.

Comment 20

[ArronM (:paper)]

(In reply to comment #19)
> Then what we should do is scan the list twice, once without scaling, and then if
> no suitable cursor is found, we should rescan allowing scaling.

I totally agree with this solution.

I'll have a patch later today (in a few hours).  Thanks to roc, bz, and stuart
for helping me on IRC with the mCursorImgContainer issue.

Comment 21

[ArronM (:paper)]

Attachment: Fixup Cursors for Win9x/XP/2k

It was discussed on IRC that we use a global cursor cache, as opposed to
nsCOMPtr, which could leak, or adding nsISupportsWeakReference to imgContainer.


Here's hoping I got the global cursor caching with add/releaseref right.

Comment 22

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

Comment on attachment 180460 [details] [diff] [review]
Fixup Cursors for Win9x/XP/2k

In nsWindow::~nsWindow(), you need to clear gCursorImgContainer after releasing
it.

+  For Win9x/ME/2k (Others?), API specs say the image size must be 
+  SM_CXCURSOR, SM_CYCURSOR (::GetSystemMetrics)

+ On WinME/2k, some video
+  drivers don't return null when they can not correctly display the cursor.
+  This is a driver bug, and in theory would be fixed in future driver
versions.

If these are popular drivers/cards, then we should check to see if the size is
SM_CXCURSOR/SM_CYCURSOR and if not, don't even try to use it.

Comment 23

[ArronM (:paper)]

Attachment: Fixup Cursors for Win9x/XP/2k

NS_IF_RELEASE clears gCursorImgContainer, so no troubles there.

My in-code comments were wrong (again).  Only Windows 9x/ME (not 2k) require
the width & height to "be the values returned by the ::GetSystemMetrics
function for SM_CXCURSOR and SM_CYCURSOR".  See 
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/winui/winui/windowsuserinterface/resources/cursors/cursorreference/cursorfunctions/setcursor.asp

Since Win9x/ME does sucessfully fail at ::GetIconIndirect when the size is not
valid, checking GetSystemMetrics is redundant.

That leaves Win2k, where there's an nVidia driver bug that crops 8bit alpha
images that are > 64x64.  roc has told me to write a case for that, and I've
added it to the patch.

roc, please correct me if I've misinterpreted anything.

Comment 24

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

Comment on attachment 180467 [details] [diff] [review]
Fixup Cursors for Win9x/XP/2k

hrm, the code duplication between IsCursorTranslucencySupported and IsWin2k is
not nice...  can you find a way around that?

it kinda sucks that even non-nVidia users get to suffer, but oh well...

Comment 25

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

Comment on attachment 180467 [details] [diff] [review]
Fixup Cursors for Win9x/XP/2k

looks good.

Comment 26

[ArronM (:paper)]

Comment on attachment 180467 [details] [diff] [review]
Fixup Cursors for Win9x/XP/2k

Seeking 1.8b2 approval.  It would be nice for Win9x/ME/2k users to be able to
see cursors properly.  As it stands, the cursor blinks on for a nanosecond
everytime the cursor is moved.	Also, there's a possible leak or two fixed up
(delete [] bgra8data, and DestroyIcon may not destroy it if it's selected on
older OSes)

Comment 27

[ArronM (:paper)]

(In reply to comment #24)
> hrm, the code duplication between IsCursorTranslucencySupported and IsWin2k is
> not nice...  can you find a way around that?

Bug 290112 filed to address this (and any other places that use the same
GetVersionEx call)

Comment 28

[Asa Dotzler [:asa]]

Comment on attachment 180467 [details] [diff] [review]
Fixup Cursors for Win9x/XP/2k

a=asa

Comment 29

[ArronM (:paper)]

Checking in nsWindow.h;
/cvsroot/mozilla/widget/src/windows/nsWindow.h,v  <--  nsWindow.h
new revision: 3.199; previous revision: 3.198
done
Checking in nsWindow.cpp;
/cvsroot/mozilla/widget/src/windows/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 3.547; previous revision: 3.546
done

Comment 30

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050413
Firefox/1.0+
The last icon for the textcase (png w/ alpha chan) worked with the official
20050412 nightly, but with a beast build from 20050413 after this checkin I no
longer see the mouseicon (the semi-transparent sun-like one).
Is this expected behaviour? Win2ksp4

Comment 31

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

unfortunately that is expected, yes - the image is larger than 64x64, and it has
an alpha channel; and the code here will not try to display such cursors on
win2k due to a bug in an nvidia driver...

Comment 32

[Steve England [:stevee]]

ok, I thought I'd point it out anyway incase.. FWIW I have a nVidia card
(GeForce4 Ti4400 with v76.41 drivers, 1280x1024x32@85Hz) and the png testcase
worked fine for me.

Comment 33

[ArronM (:paper)]

(In reply to comment #32)
> ok, I thought I'd point it out anyway incase.. FWIW I have a nVidia card
> (GeForce4 Ti4400 with v76.41 drivers, 1280x1024x32@85Hz) and the png testcase
> worked fine for me.

Unfortunately, v76.41 isn't an official nVidia driver release.  On v71.84, the
latest official driver, the bug biesi mentioned exists.  If nVidia has a driver
release before Gecko 1.8 is released, I'll definitely push for the code to be
removed.  Good to know that nVidia will have addressed this problem in their
next revision (one would hope!).

Comment 34

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

(In reply to comment #33)
> Unfortunately, v76.41 isn't an official nVidia driver release.  On v71.84, the
> latest official driver, the bug biesi mentioned exists.  If nVidia has a driver
> release before Gecko 1.8 is released, I'll definitely push for the code to be
> removed.  Good to know that nVidia will have addressed this problem in their
> next revision (one would hope!).

I'd push back unless you can detect the buggy driver specifically. Lots of users
will have the buggy driver for the forseeable future.

Comment 35

[AKIHIRO Misaki (a.k.a Kuden)]

http://d.hatena.ne.jp/nyama/
(written in Japanese)

It is not displayed on this page. 
The cause seems it is because this cursor image is not 32�~32. 
When the size of the image is adjusted to 32x32, it is displayed. 

User-Agent:Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050413
Firefox/1.0+

Comment 36

[AKIHIRO Misaki (a.k.a Kuden)]

testcase
http://maguroban.s41.xrea.com/cursor_test.html

Only 32x32 is displayed. 

Comment 37

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

yeah, win98 only supports cursors of a specific size (guess it's 32x32). we
should scale them as needed; someone should file a bug on that ;)

Comment 38

[ArronM (:paper)]

(In reply to comment #37)
> yeah, win98 only supports cursors of a specific size (guess it's 32x32). we
> should scale them as needed; someone should file a bug on that ;)

Actually, for cursors smaller than 32x32 (or required Win9x size) we should just
paint them onto a transparent 32x32 surface and use that as the cursor.  But
yes, for anything larger, and without an alternate custom cursor specified, we
should resize.   That was out of the scope of the patch.