Closed Bug 784909 Opened 7 years ago Closed 7 years ago

Add support for app-provided HiDPI mouse pointer/cursor

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: Dolske, Assigned: Dolske)

References

(Blocks 1 open bug, )

Details

Attachments

(2 files, 4 obsolete files)

Testing with a current trunk Nightly (no patches from bug 674373 et al), most standard CSS cursors are already correctly shown at full resolution, but some are not. (More precisely, they appear to be low-res images that are being scaled to hidpi size. They look fuzzy.) See: resize (various), help, progress, wait.

Safari uses hires cursors in all cases for the linked test URL.
Went through: https://developer.mozilla.org/en-US/docs/CSS/cursor

Attached is a testcase with all the differences between Nightly/Chrome/Safari on OS X. Would probably be interesting to compare with IE, just to see if there are any other significant differences.
Assignee: nobody → dolske
A few datapoints:

These cursor types don't seem to have been added to the standard NSCursor API as of OS X 10.8 (other that the vertical text-selection one).

I've been poking through the Chromium and WebKit sources to see what they're doing; I _think_ they're both deferring to the system Webkit stuff (via wkCursor). EG, from Chromium:

chromium/src/third_party/WebKit/Source/WebCore/platform/mac/CursorMac.mm
...
    case Cursor::ZoomIn:
#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1070
        m_platformCursor = wkCursor("ZoomIn");
        if (m_platformCursor)
            break;
#endif
        m_platformCursor = createNamedCursor("zoomInCursor", 7, 7);
        break;
...


As far as I can tell, the images in the Chromium/WebKit repos are all regular size (ie, not HiDPI) -- eg http://is.gd/VoGmUF for the current Chromium listing. I see slightly different contents in the webkit.org trunk, but not a plethora of "...@2x" icons. So I'm not really sure where these are coming from.


There are PNG and/or PDF resources in /System/Library/Frameworks/ApplicationServices.framework/Versions/A/Frameworks/HiServices.framework/Versions/A/Resources/cursors.

Not sure how useful these are, though. EG. The "help" cursor is an 18x18 PNG there (really 16x16 w/padding), whereas it's clearly ~32x32 cursor on my Retina MBP. Is the system using the PDF versions and adding a shadow?


Since we're obviously not using WebKit, I think the only option here is to extract (or otherwise recreate) these icons as images (in both regular and HiDPI flavors), and add them to our app bundle.
Depends on: 789849
According to iosnoop, both Chrome and Safari are triggering loads of the PDF cursors from HiServices.framework.
A couple of questions for steven/josh, about general approach and sanity:

1) Does it seem unlikely that we can get the OS to provide these cursors for us? (Or if it is possible, it involves some deep magic far beyond my noobish Cocoa skills).

2) Ergo, we'll need to provide our own versions of these CSS3 cursors. It looks possible to extract bits via [[NSCursor currentCursor] image], so presumably a little utility to create PNGs should be possible. (Alternatively we could get pretty close by having one of our visual designers just recreate them.).

3) How should the code in nsMacCursor.mm select between them? Just a simple |if (hidpi) basename += '@2x'|? It seems some of Apple's APIs will automatically look for and use "@2x" filenames, but it also looks like we've have to make additional changes around how we do stuff to make use of those APIs. (ie [NSImage imageNamed:@"foo"] instead of [[NSImage alloc] initWithContentsOfFile:"/blah/blah/foo.tiff"]).

Or let me know if I'm just crazy. :)
Mmm, some more digging at how Chromium dealt with this...

http://code.google.com/p/chromium/issues/detail?id=133287
http://code.google.com/p/chromium/issues/detail?id=92892

I assumed all the "wkCursor" stuff was in WekKit/WebCore, but from reading those it sounds more like a syscall.
Updated testcase with adding some content-specified cursors: |cursor: url(...)|
Attachment #658738 - Attachment is obsolete: true
Attached file Working POC in XCode (obsolete) —
Hey, so, I all my Googling paid off and I'm able to load hidpi cursors from files.

Some observations:

* When loading a plain old PNG, OS X is pixel doubling it. The secret is tell it "no, really, show me the actual pixels" is [cursorImage representations] + [imageRep setSize], with a size 1/2 the actual size of the image.

* But if you use a PNG with a pHYs chunk to specify the dpi, OSX will magically make it work as-is. The irony is that this is still a bit of a lie, apparently a rMBP screen is actually ~220 dpi, but the image needs to be 144dpi. (At 220 dpi it's actually scaled down too far and is blurry again!).

* I _think_ you can make OS X "just do the right thing" by using a NSImage with 2 representations -- one at 72dpi, the other at 144dpi. Untested just yet, though.

* We probably need to have a discussion about if we want to take advantage of the pHYs stuff when it comes to web content. It has no effect on today's web content on other devices, and if it suddenly becomes functional that may result in some oddities for people with hidpi displays. I suspect we'll actually want to explicitly make this not-work, in favor of some other mechanism that is more explicit?
Test images at http://dolske.net/mozilla/tests/hidpi/

[And I guess my last point in comment 7 isn't a significant concern; we do our own PNG decoding and so OS X never sees the dpi info a web-based PNG may have.]
Justin, you've already gone beyond what I know about this stuff :-)

As for the wkCursor stuff, though, I suspect those methods are thin wrappers around undocumented system calls.  I've encountered this kind of thing before in WebKit.
So, nice, this trick works for displaying the appropriate cursor image when a system has both a retina display and a regular external display...

    NSString* pathToImageLo = @"/Users/dolske/Desktop/testicon32.png";
    NSString* pathToImageHi = @"/Users/dolske/Desktop/testicon64.png";

    NSImage *images  = [[NSImage alloc] initWithContentsOfFile:pathToImageLo];
    NSImage *imageHi = [[NSImage alloc] initWithContentsOfFile:pathToImageHi];
    
    // Make the hires image hidpi
    NSImageRep *imageRep = [[imageHi representations] objectAtIndex:0];
    [imageRep setSize: (NSMakeSize(32.0, 32.0))];
    
    // XXX there's a note in the docs that NSImages can't share reps,
    // does that mean the receiver is copying?
    [images addRepresentation: [[imageHi representations] objectAtIndex:0]];
    
    NSCursor *customCursor = [[NSCursor alloc] initWithImage:images hotSpot:NSMakePoint(1.0, 1.0)];
    [customCursor set];

I think I can glue enough together to write this patch now!

I'm going to skip the wkCursor stuff, because I'm pretty sure it's not available in 10.6, and so I'd have to write fallback code anyway.
Attached patch Patch v.0 (WIP) (obsolete) — Splinter Review
(Note: based on top of bug 789849's patch)

Muahaha! It works! Even shows the right cursor when I drag the window across to my lowdpi monitor.

I've only added one actual hidpi cursor (and it's jsut a test image full of dots). But it seems to work... Does it seem like I'm doing anything fundamentally terrible here? I should point out that I don't know ObjC very well at all!
Attachment #660335 - Attachment is obsolete: true
Attachment #660675 - Flags: feedback?(smichaud)
Comment on attachment 660675 [details] [diff] [review]
Patch v.0 (WIP)

Moving feedback, since Steven noted he's swamped with HiDPI plugin stuff.
Attachment #660675 - Flags: feedback?(smichaud) → feedback?(joshmoz)
Depends on: 792592
Note to self: do we need to adjust the "hotspot" coords for hidpi, or does the system do that automatically? (Presumably there's an assumption that the hidpi and lodpi cursors will have the same relative hotspot).
Comment on attachment 660675 [details] [diff] [review]
Patch v.0 (WIP)

Review of attachment 660675 [details] [diff] [review]:
-----------------------------------------------------------------

This approach seems fine to me.
Attachment #660675 - Flags: feedback?(joshmoz) → feedback+
Adjusting summary: this bug adds the support, bug 789849 will add the actual hidpi cursor images. For testing this you can just drop in the @2x cursor example from the v.0 patch. (I don't think there's any way to write an automated test for this.)
No longer depends on: 674373
Summary: Some CSS cursors look low-res on a HiDPI display → Add support for app-provided HiDPI mouse pointer/cursor
(In reply to Justin Dolske [:Dolske] from comment #14)
> Note to self: do we need to adjust the "hotspot" coords for hidpi, or does
> the system do that automatically? (Presumably there's an assumption that the
> hidpi and lodpi cursors will have the same relative hotspot).

Reply to self: yes, this works fine. The zoomin cursor is set with "hotSpot:NSMakePoint(6,6)"... When hovering over a 1x1px div with that cursor specified, I see the hotspot at what seems like the right spot (~12x12ish).
Attached patch Patch v.1 (obsolete) — Splinter Review
Attachment #660675 - Attachment is obsolete: true
Attachment #665212 - Flags: review?(joshmoz)
Comment on attachment 665212 [details] [diff] [review]
Patch v.1

Review of attachment 665212 [details] [diff] [review]:
-----------------------------------------------------------------

It seems to me that this code might be doing main-thread i/o, which isn't good, and if that is true then your code will do even more of it. I don't think that should stop us from taking this right now, but we should file a bug to investigate. One way to test for this might be to create an HTML page with a number of rectangular divs in a vertical row. Make each one show a different cursor that is loaded from disk. Run the Gecko Profiler while quickly mousing over all of them in one swipe.

::: widget/cocoa/nsMacCursor.mm
@@ +149,5 @@
>  
>    nsCOMPtr<nsIFile> resDir;
>    nsAutoCString resPath;
> +  NSString* pathToImage, *pathToHiDpiImage;
> +  NSImage* cursorImage, *hiDpiImage;

The NSStrings are named in such a way that the second one just has "HiDpi" inserted. Each makes sense on its own. The images are not - the second name does not contain the string "cursor". Would be nice to be consistent for readability.
Attachment #665212 - Flags: review?(joshmoz) → review+
Attached patch Patch v.2Splinter Review
Changed var name per review.
Attachment #665212 - Attachment is obsolete: true
Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/85a545c8cd49

Will take a look at file IO and follow a followup if needed... ISTR we cache the cursors. The end solution would be to do so, and/or do the file io on the gecko side (async), and just pass bitmaps to this API. That would be nice so that we could also load the images from a theme JAR file.
Comment on attachment 665825 [details] [diff] [review]
Patch v.2

Flagging for post-landing review. Steven wanted to try these out in a live environment. Given the low impact of breaking this stuff, let's just do that in Nightly.
Attachment #665825 - Flags: ui-review?(shorlander)
Comment on attachment 665825 [details] [diff] [review]
Patch v.2

[Oops, disregard last comment. Meant to add that + flag to bug 792592]
Attachment #665825 - Flags: ui-review?(shorlander)
https://hg.mozilla.org/mozilla-central/rev/85a545c8cd49
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.