Closed
Bug 163174
Opened 22 years ago
Closed 20 years ago
[FIX] Support CSS3 cursors
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
People
(Reporter: tpowellmoz, Assigned: MatsPalmgren_bugz)
References
(Depends on 2 open bugs, Blocks 1 open bug, )
Details
(Keywords: css3, testcase)
Attachments
(18 files, 17 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
image/x-mswindows-cursor
|
Details | |
(deleted),
image/x-mswindows-cursor
|
Details | |
(deleted),
image/x-mswindows-cursor
|
Details | |
(deleted),
application/octet-stream
|
Details | |
(deleted),
patch
|
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
mikepinkerton
:
review+
|
Details | Diff | Splinter Review |
(deleted),
image/x-icon
|
Details | |
(deleted),
image/x-icon
|
Details | |
(deleted),
image/x-icon
|
Details |
Although CSS3 is not finalized, it would be quite helpful to support the
additional proposed cursor styles, especially since many of them would be
helpful to use internally in Mozilla. It would also provide needed capabilities
for developers that are creating web-applications.
The current CSS3 draft spec adds the following cursors to those supported in CSS2:
progress | all-scroll | col-resize | row-resize | no-drop | not-allowed |
vertical-text | copy | alias | context-menu | cell
IE6 already supports many of these:
progress | all-scroll | col-resize | row-resize | no-drop | not-allowed |
vertical-text
See the microsoft description of its cursor support:
http://msdn.microsoft.com/workshop/author/dhtml/reference/properties/cursor.asp
and
http://webdesign.about.com/library/weekly/aa081501a.htm
Mozilla needs the col-resize and row-resize cursors for column controls and
frame resizing - see bug 18958. I suspect Mozilla already supports the progress
cursor as well as the no-drop cursor. It also supports a dragging text selection
cursor that isn't mentioned in the spec, but could be exposed.
These additional cursors would be somewhat less necessary if Mozilla implemented
handling of URI values on CSS2 cursor properties as described in bug 38447, but
they'd be more convenient and helpful. It would also be nice if web developers
could rely on the IE and Mozilla to have similar cursors available.
An earlier draft for CSS3 cursors
http://www.w3.org/TR/2000/WD-css3-userint-20000216#cursor
also mentioned the grab and grabbing cursors that are asked for in bug 121986.
Personally, I think these are much better than the move cursor commonly used in
DHTML and should be included in CSS3 and Mozilla as well.
I appears that to fix this requires changes to the nsCursor identifier:
http://lxr.mozilla.org/seamonkey/ident?i=nsCursor
Comment 1•22 years ago
|
||
Sound good to me!
Reporter | ||
Comment 2•22 years ago
|
||
An initial testcase is available:
http://www.worldtimzone.com/mozilla/testcase/css3cursors.html
Updated•22 years ago
|
Severity: normal → enhancement
Comment 3•22 years ago
|
||
Are the cursors in question actually supported natively on the mozilla platforms
(Win32, Mac, X, OS/2)? Or would we need to start painting the cursor ourselves
or something ridiculous like that?
Comment 4•22 years ago
|
||
Mass voting for advance in CSS cursor implementation...
Reporter | ||
Comment 5•21 years ago
|
||
I've updated the testcase to include the known -moz cursors and to include more
pictures. Mozilla 1.4 apparently supports some of the CSS3 cursors, if not by
their actual identifiers:
CSS3 cursor Mozilla equivalent
progress -moz-spinning
copy -moz-copy
alias -moz-alias
cell -moz-cell
context-menu -moz-context-menu (mac/unix only?)
Unfortunately only the first overlaps with the CSS3 cursors supported by IE6.
Comment 6•21 years ago
|
||
>Comment #3: Are the cursors in question actually supported natively on the
mozilla platforms (Win32, Mac, X, OS/2)?
I don't see any CSS3 cursors (currently supported and rendered by MSIE 6) in the
Windows/Cursors directory. I suspect it is the case since Mozilla uses the
default selected text dragging cursor in Win32 for dragging a bookmark and so
does MSIE 6 when dragging a favorite on the Links toolbar.
>Or would we need to start painting the cursor ourselves or something ridiculous
like that?
If Mozilla needs to create, build some CSS3 cursors for the Win32 platforms (in
.cur - bitmap format), then I can do it in 32x32 bitmaps without problems. I
already started doing so at
http://www10.brinkster.com/doctorunclear/HTMLJavascriptCSS/Cursors.html
and anyone using MSIE 6 for Windows can see how the cursors look like; others
will just see the equivalent .gif.
Comment 7•21 years ago
|
||
Since this is in Last Call now at the w3c, now would be a good time.
I can do the CSS3 parts of this, and the Mac OS implementation.
Not assigning it to myself just yet: I need to land some other cursor work first.
Comment 8•21 years ago
|
||
I've somewhat implemented the remaining CSS3 cursors since Glazman needed one of
them implementing a few days ago. I've prefixed all the keywords with "-moz-"
since the spec isn't final yet.
The patch I'm about to submit performs all the modifications necessary, adds the
extra cursors to the Gtk2 nsWindow class and implements not-allowed in the
Windows nsWindow class. Some of the Gtk2 pointers aren't entirely appropriate
for the cursor type, so we may have to come up with our own.
Comment 9•21 years ago
|
||
Comment 10•21 years ago
|
||
Cool. I'll try to review your patch this weekend.
Once I get done with another cursor related bug, I'll do the Mac & Camino
implementations of the new cursors.
Comment 11•21 years ago
|
||
CSS3 UI is last call and can become a CR anytime; can't we just implement them
without the -moz- prefix?
Comment 12•21 years ago
|
||
The CSS3 cursors that Mozilla has already implemented have the -moz prefix, so
this path would keep them consistent. However, changing the patch to not use
the prefix would be pretty trivial and I'll do it if people feel it's the right
thing to do.
Comment 13•21 years ago
|
||
Actually I'd prefer that too. Its extremely unlikely anything will change now. I
thought it was appropriate for there to be sample implementations at this stage?
Comment 14•21 years ago
|
||
Attachment #143706 -
Attachment is obsolete: true
Comment 15•21 years ago
|
||
OK, here's a new patch with the changes needed. Perhaps I should add support
for the other cursors without -moz too.
Comment 16•21 years ago
|
||
> OK, here's a new patch with the changes needed. Perhaps I should add support
> for the other cursors without -moz too.
That would be nice. That way everything is consistent. Officially we should drop
-moz- at CR stage (then they start looking for implementations and create a
suitable test case), however, this draft isn't going to change anymore.
Comment 17•21 years ago
|
||
I'll put support for the other cursors into the patch tomorrow, but I'm
reluctant to drop the -moz ones since they're used all over the place, so I'll
just make them equivalent.
Comment 18•21 years ago
|
||
After this bug has been fixed (new cursors and older cursors available without
-moz- prefix) there should be a new bug (or 2?) for cleanup:
* Removing all the extra code for the cursors with the -moz- prefix.
* Make sure that all style sheets use the syntax without the prefix (because
it will be nog longer supported with the prefix).
Comment 19•21 years ago
|
||
No. Don't remove the -moz- prefix until it is in CR. It's a simple rule.
Too many times we've been burnt by people who thought "there's no chance this
can change before it reaches CR".
If you think it'll reach CR in a few days, and don't want to check in with -moz-
prefixes, then wait a few days. Otherwise, use the -moz- prefixes. For what it's
worth, I understand Tantek _has_ made some changes to the cursors in the draft
that will be going to CR. You can never know until it is actually published.
Comment 20•21 years ago
|
||
Attachment #143709 -
Attachment is obsolete: true
Comment 21•21 years ago
|
||
As per Hixie's point, I've reverted the patch to use -moz- prefixes. I've also
added nesw_resize and nwse_resize, which I missed in the last patch.
Comment 22•21 years ago
|
||
Even after it goes into CR the -moz cursors which have been included for years
should be left in. You can take out the ones you add this week, but things like
-moz-spinning are going to be used all over the place (Mozilla Extensions and
possibly even people's web pages). No point in breaking all of those uses.
Comment 23•21 years ago
|
||
The core functionality of this patch looks good, but there are several issues to
address before it can be checked in:
* Standard format please. Typically I generate patches with
cvs diff -u -2 <filename> >> CSS3Cursors.patch
You can diff against cvs-mirror. You don't need access to the checkin server.
* You're still missing 2 constants from CSS3
ns-resize & ew-resize
* in windows/nsWindow.cpp, I see this:
+ case eCursor_not_allowed:
+ newCursor = ::LoadCursor(NULL, IDC_NO);
+ break;
+
default:
NS_ASSERTION(0, "Invalid cursor type");
break;
Do we want to hit that assertion every time? I see these lines above:
2490 case eCursor_context_menu:
2491 case eCursor_count_up:
2492 case eCursor_count_down:
2493 case eCursor_count_up_down:
2494 break;
It seems like each case you don't want to handle on Windows uses an explicit
"break" to catch it. I don't think its a serious problem, but we should be
consistent.
* I know you're OK on Mac OS because if we see a cursor we don't understand in
the Mac version of nsWindow.cpp we just set the standard left-arrow cursor.
Ditto for Camino.
Address these comments and I should be able to r= this.
Comment 24•21 years ago
|
||
> cvs diff -u -2 <filename> >> CSS3Cursors.patch
-pu8 is better -- more context, and function/method name or label on @@ lines.
/be
Updated•21 years ago
|
Comment 25•21 years ago
|
||
Attachment #143786 -
Attachment is obsolete: true
Comment 26•21 years ago
|
||
Here's a new patch that addresses lordpixel's concerns about the old one. For
the Windows cursors, I've simply added the enum values in without a cursor.
Hopefully some Windows developer can perform the appropriate mappings, since I
don't develop on Windows.
Updated•21 years ago
|
Attachment #144655 -
Flags: review+
Updated•21 years ago
|
Status: NEW → ASSIGNED
Updated•21 years ago
|
Attachment #144655 -
Flags: superreview?(dbaron)
Comment 27•21 years ago
|
||
I will take this and try to get it sr'd so I can check it in.
Assignee: dbaron → lordpixel
Status: ASSIGNED → NEW
Comment 28•21 years ago
|
||
I will take this and try to get it sr'd so I can check it in.
Darren, thanks for the patch. If you'd rather own the bug then by all means take
it... otherwise I'll try to make sure this doesn't get ignored indefinately.
Updated•21 years ago
|
Status: NEW → ASSIGNED
Comment 29•21 years ago
|
||
I think you should stub-out the new values in all widget ports, even if you
don't implement them. It also seems like you could have better approximations
for a bunch of the missing ones using existing cursors on Windows (the NWSE,
etc. sizing ones, and probably others), and perhaps on other platforms.
Also, please note that the CSS parser really shouldn't be accepting any cursors
that we don't actually support, although it's less of an issue (although still
an issue) for -moz-* cursors.
Comment 30•21 years ago
|
||
(In reply to comment #26)
> Hopefully some Windows developer can perform the appropriate mappings, since I
> don't develop on Windows.
That doesn't mean you can't read
http://msdn.microsoft.com/library/en-us/winui/winui/windowsuserinterface/resources/cursors/cursorreference/cursorfunctions/loadcursor.asp
http://developer.apple.com/documentation/Carbon/Reference/Appearance_Manager/appearance_manager/constant_14.html#//apple_ref/doc/c_ref/ThemeCursor
http://developer.gimp.org/api/2.0/gdk/gdk-cursors.html
etc., and make an attempt at a patch.
Comment 31•21 years ago
|
||
Thanks for the links.
Actually, I wrote the current cursor code for Mozilla Mac & Camino, so what I'd
planned to do is enter two more bugs for the actual implementation on those
platforms.
I feel we've got a 'good enough' on Windows & Linux. I can check all of the
other ports to see what they'll do if presented with a cursor that they don't
recognize.
I would do the extra cursors on Windows & Linux, but I don't have either around
to compile and test on, so I can't really :( I'll try if you insist, but it'd be
easier to file defects for those 2 platforms and try to get people who actually
have Windows/Linux boxes to own them.
I'd like to get the style system stuff in as it stands now, then start on the
Mac stuff. What do you think?
Comment 32•21 years ago
|
||
I'm loathe to really touch the Windows and Mac cursor code since I don't have
any developer tools for Windows and I don't have any access to a Mac. If I made
a change that broke Windows/Mac, I wouldn't know and wouldn't be able to fix it.
I'd be happy to help with the Linux implementation, but like I said, the Gtk2
cursors don't really map that well to a good number of the CSS3 cursors.
Comment 33•21 years ago
|
||
You should stub out the case statements on all widget ports. That's not negotiable.
I'd hesitate to call this "good enough" on Windows given that it implements only
one of the 10 added cursors, but whatever.
It's often easier to break other platforms by not changing their code than by
changing it in the obvious way.
Comment 34•21 years ago
|
||
I read the links you sent and I think I can add 3 or 4 couple more cursors on
Windows and maybe a couple on Linux. I'll repost the patch when I've had a
chance to work in those changes and have checked all of the other ports.
Comment 35•21 years ago
|
||
CSS3 UI has now reached CR.
The new values can be implemented without the -moz- prefix. Note that we must be
backwards compatible with existing values. These need to behave the same for
with and without -moz- the prefix; if implemented according to the CSS3 UI module.
Comment 36•21 years ago
|
||
(In reply to comment #35)
> CSS3 UI has now reached CR.
>
> The new values can be implemented without the -moz- prefix.
Excellent. I'm currently working on the Camino implementation of all of the cursors.
I'll ensure anything we previously had implemented as -moz still works.
Assignee | ||
Comment 37•20 years ago
|
||
(In reply to comment #35)
> CSS3 UI has now reached CR.
>
> The new values can be implemented without the -moz- prefix. Note that we must be
> backwards compatible with existing values. These need to behave the same for
> with and without -moz- the prefix; if implemented according to the CSS3 UI
> module.
Don't we just usually drop the old -moz- counterparts when moving to CSS3?
David?
Comment 38•20 years ago
|
||
> Don't we just usually drop the old -moz- counterparts when moving to CSS3?
> David?
Not for the cursors which have had a -moz prefix for a long time.
There will be CSS for XUL and also possibly HTML pages out there that use -moz
because some of these keywords have been in the source since 2002.
Assignee | ||
Comment 39•20 years ago
|
||
Assignee | ||
Comment 40•20 years ago
|
||
Assignee | ||
Comment 41•20 years ago
|
||
Assignee | ||
Comment 42•20 years ago
|
||
Assignee | ||
Comment 43•20 years ago
|
||
Assignee | ||
Comment 44•20 years ago
|
||
Assignee | ||
Comment 45•20 years ago
|
||
Comment 46•20 years ago
|
||
Next time you need to attach a lot, just zip or tar.gz please!
Assignee | ||
Comment 47•20 years ago
|
||
Assignee | ||
Comment 48•20 years ago
|
||
Taking bug, patch coming up...
Assignee: lordpixel → mats.palmgren
Status: ASSIGNED → NEW
Keywords: testcase
Summary: Support CSS3 cursors → [FIX] Support CSS3 cursors
Assignee | ||
Comment 49•20 years ago
|
||
This patch adds the CSS3 cursors and also cleans up the naming
to match the CSS names. I have also fixed a problem in ESM who
mapped both NS_STYLE_CURSOR_N_RESIZE/S_RESIZE to eCursor_sizeNS
which made it impossible to differentiate these in the widget
classes. The are now propagated as eCursor_n_resize/s_resize.
(Same for the w/e case). I have added custom glyphs for
gtk/gtk2/xlib from the previously attached images.
If anyone can produce .cur versions for Windows for col-resize,
row-resize and vertical-text that would be great.
I have also stubbed out all the other widget sets and added
reasonable values, but of course there are some glyphs still
missing. I have only tested on gtk2, so please test and
report problems if you can for the other platforms.
Assignee | ||
Comment 50•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #154824 -
Flags: superreview?(dbaron)
Attachment #154824 -
Flags: review?(lordpixel)
Assignee | ||
Updated•20 years ago
|
Attachment #144655 -
Attachment is obsolete: true
Attachment #144655 -
Flags: superreview?(dbaron)
Comment 51•20 years ago
|
||
Comment 52•20 years ago
|
||
Comment 53•20 years ago
|
||
Comment 54•20 years ago
|
||
Just made three MSWin cursor versions aiming to satisfy "that would be great"
request.
These cursors are painted using only two colors: transparent color and inverted
background. (That's similar to how standard Windows "I" text cursor is
rendered.) Hotspots are defined in the center of the central line segment.
(That's similar to where hotspot of standard Windows "I" text cursor is
located.) So I hope there cursors will fit into usual Windows style seamlessly.
(I've tried them in Control Panel --> Mouse --> Cursors several times while
debugging hotspots, just to be sure.)
Assignee | ||
Comment 55•20 years ago
|
||
Added support for the three custom cursors for Windows. (Thanks Sergey!)
They should be saved as col_resize.cur row_resize.cur and vertical_text.cur
(in widget/src/build/res/).
Any volounteers for testing this on Windows?
Also, if someone can provide OS/2 versions of the same (.ptr format) I'll
be glad to add those in as well.
Assignee | ||
Updated•20 years ago
|
Attachment #154824 -
Attachment is obsolete: true
Attachment #154825 -
Attachment is obsolete: true
Assignee | ||
Comment 56•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #154824 -
Flags: superreview?(dbaron)
Attachment #154824 -
Flags: review?(lordpixel)
Assignee | ||
Updated•20 years ago
|
Attachment #154850 -
Flags: superreview?(dbaron)
Attachment #154850 -
Flags: review?(lordpixel)
Updated•20 years ago
|
Attachment #154850 -
Flags: superreview?(dbaron)
Attachment #154850 -
Flags: review?(lordpixel)
Attachment #154850 -
Flags: review-
Comment 57•20 years ago
|
||
Here's my implementation for Camino. Please merge with your patch (which looks
good) and repost. Please use cvs diff -puw8 ... it'll help me review if I get
more context in the diff.
I'll implement the standard Mac version for Firefox/Mozilla shortly, but it'll
take me some hours to test because I haven't build the seamonkey tree in some
time.
I'll also be attaching an updated test case and the new .tiff files required
for Camino.
Updated•20 years ago
|
Attachment #154850 -
Attachment is obsolete: true
Attachment #154851 -
Attachment is obsolete: true
Comment 58•20 years ago
|
||
Comment 59•20 years ago
|
||
Attachment #154878 -
Attachment is obsolete: true
Assignee | ||
Comment 60•20 years ago
|
||
Merged "Patch B rev. 2" with camino/cocoa changes from lordpixel@mac.com
We now have custom made cursors for the three major platforms and
reasonable values for the others as well.
Attachment #154893 -
Attachment is obsolete: true
Assignee | ||
Comment 61•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #154918 -
Flags: review?(lordpixel)
Assignee | ||
Comment 62•20 years ago
|
||
Comment on attachment 154918 [details] [diff] [review]
Patch B rev. 3
I have now also tested this on Windows XP and it worked fine.
Assignee | ||
Updated•20 years ago
|
Attachment #154918 -
Flags: superreview?(dbaron)
Assignee | ||
Updated•20 years ago
|
Flags: blocking1.8a3?
Comment 63•20 years ago
|
||
I have almost finished by review of the attachment.
I need to do a little more work on the camino/mac implementation, however
depending on how this goes I may seperate those issues out into a seperate bug
and r+ this.
I do have one question about the gtk implementation. I see we have a gtk and a
gtk2 implementation, fair enough. But inside the gtk implementation, there seems
to be duplicate, inconsistent code:
widget/src/gtk/nsWidget.cpp & widget/src/gtk/nsWindow.cpp
Why do these two files exist, and why don't the implementations agree?
Comment 64•20 years ago
|
||
Sorry for spam, but want to keep progress updated.
Mac & Camino changes inspired by Mats comments in patch are almost done.
One slight wrinkle: we have to support building (as well as running) the Mac
Firefox/Mozilla build on Jaguar (10.2) so I need to adjust my patch with a few
#defines to make sure I don't use unavailable features.
Its midnight though, so that'll have to wait for tomorrow.
Comment 65•20 years ago
|
||
Attachment #154918 -
Attachment is obsolete: true
Attachment #154919 -
Attachment is obsolete: true
Updated•20 years ago
|
Attachment #154918 -
Flags: superreview?(dbaron)
Attachment #154918 -
Flags: review?(lordpixel)
Comment 66•20 years ago
|
||
Here's the latest mac & camino diffs and the zip containing the required cursor
images.
Merge this into the patch and I'll r+ - I got done reading the previous
version.
Steps to apply:
1. apply attachment 155998 [details] [diff] [review]
2. checkin the .tiffs from the ZIP into widget/src/cocoa/cursors
3. delete widget/src/cocoa/cursors/contextMenu.tiff from CVS - its no longer
needed
Updated•20 years ago
|
Attachment #154879 -
Attachment is obsolete: true
Assignee | ||
Comment 67•20 years ago
|
||
Merged Mac & Camino stuff (attachment 155998 [details] [diff] [review]) from lordpixel@mac.com
The changed files since "Patch B rev. 3" are:
widget/src/mac/nsMacWidget.r
widget/src/mac/nsWindow.cpp
widget/src/cocoa/nsCursorManager.mm
camino/Camino.xcode/project.pbxproj
Changes to other files are the same as they were in rev. 3
Attachment #155998 -
Attachment is obsolete: true
Assignee | ||
Comment 68•20 years ago
|
||
Assignee | ||
Comment 69•20 years ago
|
||
(In reply to comment #63)
> I do have one question about the gtk implementation. I see we have a gtk and
> a gtk2 implementation, fair enough. But inside the gtk implementation, there
> seems to be duplicate, inconsistent code:
>
> widget/src/gtk/nsWidget.cpp & widget/src/gtk/nsWindow.cpp
>
> Why do these two files exist, and why don't the implementations agree?
Fair question, to be honest - I don't know why it's designed the way
it is - or not designed perhaps ;-)
There is some code under gtk2/gtk/xlib that is *identical* or that
only differs in indentation that should be shared IMO.
(e.g. nsGtkCursors.h)
However, I would appreciate if we could ignore those issues in this
patch, since they are platform specific and redesigning that will
probably delay this bug being fixed. But, if you do see differences
that *appears to be errors*, specifically in the cursor code, then
please point them out and I will fix them.
Assignee | ||
Updated•20 years ago
|
Attachment #156225 -
Flags: review?(lordpixel)
Assignee | ||
Comment 70•20 years ago
|
||
Comment on attachment 156225 [details] [diff] [review]
Patch B rev. 4
Pinkerton, please review the Mac/Camino/Cocoa stuff in this patch, including
the .tiff images attached above.
Updated•20 years ago
|
Flags: blocking1.8a3? → blocking1.8a3-
Updated•20 years ago
|
Attachment #156225 -
Flags: review?(lordpixel) → review+
Assignee | ||
Updated•20 years ago
|
Attachment #156225 -
Flags: superreview?(dbaron)
Comment 71•20 years ago
|
||
Comment on attachment 156225 [details] [diff] [review]
Patch B rev. 4
sr=dbaron. But could somebody attach the testcase in comment 2, without the
images, to the bug, so we have a permanent copy of it? (Or if there was
another testcase you were testing with, that as well...)
Attachment #156225 -
Flags: superreview?(dbaron) → superreview+
Comment 72•20 years ago
|
||
Mats, when checking this in (since I'm not sure if you've added binary files
with cvs before -- or for that matter any files): when you cvs add a binary
file (as some images are), remember to use "cvs add -kb <file>" (and then commit
the files after they're added).
If you mess up, you can use "cvs admin -kb <file>" to change to binary and "cvs
admin -kkv <file>" to change back to the default. However, this doesn't change
what's in the repository, so if you checked ina binary file with "kv" keyword
substitution instead of "b" keyword substitution from a platform that does
newline substutions (MacCVS or a Windows cvs that converts to DOS line endings),
you'd need to copy the files aside, update, replace the uncorrupted files, and
commit again.
Assignee | ||
Updated•20 years ago
|
Attachment #156225 -
Flags: review+ → review?(pinkerton)
Comment 73•20 years ago
|
||
Since regular Mozilla bugs don't allow an arbitrary number of reviewers on a
patch, and Mats therefore had to wipe out my review status to ask pinkerton to
review, I'll add r+ lordpixel@mac.com here in this comment for the record.
Comment 74•20 years ago
|
||
+#ifdef MAC_OS_X_VERSION_10_3
+ cursor = OnPantherOrLater() ? kThemeResizeUpCursor : 135; break;
+#else
+ cursor = 135; break;
+#endif
note that we do release builds with the 10.2.X SDK so this will most certainly
be false even when running on panther. If you need the constant, you'll have to
pull it in manually. Maybe wrap the duplicate define in something so it doesn't
conflict with developers who build with the 10.3 SDK.
thanks for updating camino, btw :D
Comment 75•20 years ago
|
||
OK - so I changed the implementation to hardcode the cursor values for 10.3
when *compiling* on 10.2, and to use the resource cursors when *running* on
10.2 as before. People who compile on 10.3 will get the "real" constants.
Let me know if you have any questions.
Assignee | ||
Comment 76•20 years ago
|
||
Integrated latest widget/src/mac/nsWindow.cpp patch (no other changes to rev.
4)
Attachment #157111 -
Attachment is obsolete: true
Assignee | ||
Comment 77•20 years ago
|
||
Comment on attachment 157187 [details] [diff] [review]
Patch B rev. 5
Link to the diff against "Patch B rev. 4":
http://bugzilla.mozilla.org/attachment.cgi?oldid=156225&action=interdiff&newid=
157187&headers=1
Attachment #157187 -
Flags: review?(pinkerton)
Assignee | ||
Updated•20 years ago
|
Attachment #156225 -
Flags: review?(pinkerton)
Comment 78•20 years ago
|
||
+//19, 20, 21 from Appearance.h in 10.3
+#define PANTHER_RESIZE_UP_CURSOR 19
+#define JAGUAR_RESIZE_UP_CURSOR 135
+#define PANTHER_RESIZE_DOWN_CURSOR 20
+#define JAGUAR_RESIZE_DOWN_CURSOR 136
+#define PANTHER_RESIZE_UPDOWN_CURSOR 21
+#define JAGUAR_RESIZE_UPDOWN_CURSOR 141
how about placing the real constant name after the hardcoded value, but
commented out. also, i dislike using #defines for constants. use const short.
the compiler should do the right thing.
so it should look like, eg:
const short PANTHER_RESIZE_CURSOR = 19; // kThemeResizeUpCursor
Assignee | ||
Comment 79•20 years ago
|
||
Comment on attachment 157111 [details] [diff] [review]
Handle compiling with 10.2 SDK
>RCS file: /cvsroot/mozilla/widget/src/mac/nsWindow.cpp,v
>+#ifdef MAC_OS_X_VERSION_10_3
>+#define PANTHER_RESIZE_UP_CURSOR kThemeResizeUpCursor
>+#define JAGUAR_RESIZE_UP_CURSOR 135
>+#define PANTHER_RESIZE_DOWN_CURSOR kThemeResizeDownCursor
>+#define JAGUAR_RESIZE_DOWN_CURSOR 135
I'm assuming JAGUAR UP/DOWN should really be 135/136...
Assignee | ||
Comment 80•20 years ago
|
||
Fixed comment 78 and comment 79.
I also removed the change to OnJaguarOrLater(), leaving it as it is in CVS now
-
it has been changed by bug 223545. I made a similar change to
OnPantherOrLater().
The only changed file since rev. 5 is: widget/src/mac/nsWindow.cpp
Attachment #157187 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #157187 -
Flags: review?(pinkerton)
Assignee | ||
Updated•20 years ago
|
Attachment #157566 -
Flags: review?(pinkerton)
Assignee | ||
Updated•20 years ago
|
Flags: blocking1.8a4?
Comment 81•20 years ago
|
||
(In reply to comment #79)
> I'm assuming JAGUAR UP/DOWN should really be 135/136...
Yup. Amazing how many times one can read a piece of code and not see a typo.
The changes to onPantherOrLater()/onJaguarOrLater() look fine to me too.
Comment 82•20 years ago
|
||
Comment on attachment 157566 [details] [diff] [review]
Patch B rev. 6
mac stuff looks ok r=pink
Attachment #157566 -
Flags: review?(pinkerton) → review+
Assignee | ||
Comment 83•20 years ago
|
||
Checked in to trunk at 2004-09-11 16:24 PDT.
There was a compile problem on "MacOSX Darwin 6.8 monkey" though:
/builds/tinderbox/SeaMonkey/Darwin_6.8_Depend/mozilla/widget/src/mac/nsWindow.cpp:78:
`
kThemeResizeUpCursor' was not declared in this scope
/builds/tinderbox/SeaMonkey/Darwin_6.8_Depend/mozilla/widget/src/mac/nsWindow.cpp:79:
`
kThemeResizeDownCursor' was not declared in this scope
/builds/tinderbox/SeaMonkey/Darwin_6.8_Depend/mozilla/widget/src/mac/nsWindow.cpp:80:
`
kThemeResizeUpDownCursor' was not declared in this scope
In order to get the Tinderbox green I checked in the following "fix":
#ifdef MAC_OS_X_VERSION_10_3
-const short PANTHER_RESIZE_UP_CURSOR = kThemeResizeUpCursor;
-const short PANTHER_RESIZE_DOWN_CURSOR = kThemeResizeDownCursor;
-const short PANTHER_RESIZE_UPDOWN_CURSOR = kThemeResizeUpDownCursor;
+const short PANTHER_RESIZE_UP_CURSOR = 19;
+const short PANTHER_RESIZE_DOWN_CURSOR = 20;
+const short PANTHER_RESIZE_UPDOWN_CURSOR = 21;
#else
That is probably not correct, but I'm not familiar enough with these
different MacOSX versions to say what the correct code is.
Mike, Andrew - please advise what to do about that block.
Comment 84•20 years ago
|
||
If try the test case, I don't get another cursor than the default by the
following values: 'context-menu', '-moz-context-menu', '-moz-count-up',
'-moz-count-down', '-moz-count-up-down'. Is that correct?
If that is correct, I suggest we remove support for those 4 -moz- values, unless
they are tested incorrectly of course.
Assignee | ||
Comment 85•20 years ago
|
||
(In reply to comment #84)
> If try the test case, I don't get another cursor than the default by the
What platform and build?
The checked in code has not propagated to the nightly builds from mozilla.org
yet (at least not the Windows and Linux builds).
Comment 86•20 years ago
|
||
2004091200, WinXP. All other values worked fine, so I guessed it was checked in.
(I'm using a build that is updated every few hours by the way, not the nightly.)
Assignee | ||
Comment 87•20 years ago
|
||
OK, you're right, we have never had any glyph for -moz-context-menu on Windows.
I have spawned this off as bug 258960.
Comment 88•20 years ago
|
||
The column-resize cursor doesn't look like the version used by the windows list
view common control.
Also note that Windows style is to centre the hotspot if possible.
Comment 89•20 years ago
|
||
(in reply to comment 88)
The col-resize hotspot is at (x=10; y=8) right now, isn't it? And overall width
is 21x17, so it seems centered well enough :-)
Meanwhile, may I have a look at the version used by the windows list view common
control? Is there an URL, or may you attach that here, or whatever else?
Comment 90•20 years ago
|
||
I've played a little with several Windows applications right now.
Most of them (including Windows Explorer when navigating local filesystem in
Table view) use w-resize (e-resize) to resize columns as if that were windows.
Some applications use a simple bold cross with horizontal segment arrowed. It is
too primitive, so I have a strong feeling that attachment 154846 [details] should be
preferred, even if the bold cross is the above mentioned version used by the
windows list view common control.
Comment 91•20 years ago
|
||
This is the cursor that I see when I hover over Explorer's Detail view columns.
P.S. forget my comment about centering, I was mistaken.
Comment 92•20 years ago
|
||
I subsequently discovered the existence of this cursor, which you may prefer.
Comment 93•20 years ago
|
||
Row resize version - mime type image/x-icon used to allow Mozilla to preview.
Assignee | ||
Comment 94•20 years ago
|
||
I missed nsGlobalChromeWindow::SetCursor(). For completeness I think we
should support the new CSS3 cursor values here also. I'm also adding the
missing CSS2.1 value "progress".
Assignee | ||
Updated•20 years ago
|
Attachment #159327 -
Flags: superreview?(dbaron)
Attachment #159327 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Updated•20 years ago
|
Flags: blocking1.8a4?
Comment 95•20 years ago
|
||
Comment on attachment 159327 [details] [diff] [review]
Patch C rev. 1
If this is the same list, why not use the same code? (That probably requires
moving nsCSSParser::SearchKeywordTable into nsCSSProps, making it return the
result instead of the index (which wouldn't hurt at all), and then calling
nsCSSKeywords::LookupKeyword and nsCSSProps::SearchKeywordTable (your new one,
which might need a different name from the existing ones, which I think should
be renamed to RSearch* -- and RSearchKeywordTableInt should return an
nsCSSKeyword enum instead of an int).) But if you don't feel like doing that
now, this is fine...
Attachment #159327 -
Flags: superreview?(dbaron) → superreview+
Assignee | ||
Comment 96•20 years ago
|
||
(In reply to comment #95)
> (From update of attachment 159327 [details] [diff] [review])
> If this is the same list, why not use the same code?
The thought occured to me but the list is not the same -
'grab*'/'spinning'/'count-*' are implemented here without the -moz- prefix.
(I filed bug 260272 on cleaning up this later)
Assignee | ||
Comment 97•20 years ago
|
||
Attachment #159327 -
Attachment is obsolete: true
Attachment #159327 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 98•20 years ago
|
||
While using Mozilla 1.8a4 build 2004092404 under XP Pro SP2:
o col-resize, row-resize, not-allowed, vertical-text are correct.
o no-drop is the same as not-allowed.
o all-scroll is using the move cursor, not the autoscroll icon/cursor now in use
in Firefox.
http://www10.brinkster.com/doctorunclear/HTMLJavascriptCSS/Cursors.html#CSS3
Updated•20 years ago
|
Depends on: context-menu-cursor
Assignee | ||
Comment 99•20 years ago
|
||
I have spawned bug 275173 and bug 275174 to improve the no-drop/all-scroll
glyphs on Windows.
If there are other any other issues, please file new bugs.
I would like to thank all who contributed, Darren Winsper who did initial
work on this bug, Sergey "Mithgol the Webmaster" Sokoloff who did the Windows
cursor glyphs, Michael Kaply (IBM) who provided cursors for OS/2 and special
thanks to lordpixel@mac.com who did all the work for Mac.
Thanks to everyone else too for providing valuable feedback.
-> FIXED (remaining issues filed separately)
Comment 100•20 years ago
|
||
It was a pleasure to work with you.
And you may have already noticed my Windows glyph dropped inside the new bug 275173.
You need to log in
before you can comment on or make changes to this bug.
Description
•