Closed
Bug 1315443
Opened 8 years ago
Closed 8 years ago
Some customized cursors could not work on Windows Firefox
Categories
(Core :: Graphics: ImageLib, defect)
Tracking
()
RESOLVED
FIXED
mozilla53
| Tracking | Status | |
|---|---|---|
| firefox53 | --- | fixed |
People
(Reporter: ttt43ttt, Assigned: aosmond)
Details
(Keywords: testcase)
Attachments
(3 files, 3 obsolete files)
test.cur
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.71 Safari/537.36 Steps to reproduce: I set the customized cursors on a div using CSS "cursor:url(xxx.cur), default". Actual results: Some cursors work well, but some of them don't work, and they fallback to the default. All of these cursors can work on Chrome, IE, Safari, and Mac Firefox. I find that if I change the hotspot of the broken *.cur file, they can work under some values. So this issue may relate to the hotspot of the cursor file. Expected results: The cursors should work.
Status: UNCONFIRMED → NEW
Has STR: --- → yes
Ever confirmed: true
Keywords: testcase
OS: Unspecified → Windows 10
Hardware: Unspecified → All
|
||
Updated•8 years ago
|
Component: Untriaged → DOM
Product: Firefox → Core
|
||
Updated•8 years ago
|
Component: DOM → CSS Parsing and Computation
|
||
Comment 2•8 years ago
|
||
The testcase in comment 1 doesn't really show the issue because <body> doesn't have height by default in Firefox (unlike in Chrome and Safari). This testcase sets the height of <body> to be viewport, and also inlines the cur file and uses pointer as fallback so that it is clearer that fallback happens (rather than falling to the default value).
|
|
||
Comment 3•8 years ago
|
||
The issue is that, in nsFrame::FillCursorInformationFromStyle, image request has STATUS_ERROR bit set, which indicates it is an issue in the image decoder which fails to decode this cur file.
Component: CSS Parsing and Computation → ImageLib
|
|
||
Comment 4•8 years ago
|
||
This is a testcase which simply uses <img> tag to display the cur file. It shows the image correctly on Chrome and Safari, but shows a broken mark on Firefox.
Attachment #8807848 -
Attachment is obsolete: true
Attachment #8810164 -
Attachment is obsolete: true
|
Assignee | |
Updated•8 years ago
|
Assignee: nobody → aosmond
|
|
Assignee | |
Comment 5•8 years ago
|
||
Looks like we are tripping up on the image data offset in the BMP header being the same as the start of the color table: http://searchfox.org/mozilla-central/rev/886d5186f0598ab2f8a9953eb5a4dab9750ef834/image/decoders/nsBMPDecoder.cpp#724
|
|
Assignee | |
Comment 6•8 years ago
|
||
Further analysis required before review/landing (ideally dig up some documentation somewhere saying this is the right thing to do...). It appears that if we ignore the data offset in the BMP header, and assume the data follows the color table directly if inside an ICO, we can decode the image just fine. This is what Chrome does.
|
|
Assignee | |
Comment 7•8 years ago
|
||
Consistently use the BMP's BPP value instead of the ICO's when it becomes available, as we already acknowledge it can be wrong. In this case it affected our offset calculation. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f0212241a2e241f11e8de90fb28aaaf82f9364d
Attachment #8810537 -
Attachment is obsolete: true
Attachment #8811255 -
Flags: review?(tnikkel)
|
||
Comment 8•8 years ago
|
||
Comment on attachment 8811255 [details] [diff] [review] Prefer BMP BPP over ICO BPP, v1 I had this exact patch sitting in my patch queue forgotten! I'm confident it's correct because this is the comment I had on the patch: "Use the bits per pixel value we read from the bmp header inside icos, not the value in the ico directory entry. If they disagree we want to use the bmp header value. Further mBitCount is a union with the cursor hot spot is this is a CUR file, so it doesn't even represent BPP in CUR files." On cursors mBitCount isn't valid because it represents the hotspot of the cursor. There might be more bugs where we use fields of this union incorrectly, we should have a followup for auditing that.
Attachment #8811255 -
Flags: review?(tnikkel) → review+
Pushed by aosmond@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9876606891a7 Use the embedded BMPs BPP over the ICOs BPP when available. r=tnikkel
|
|
Assignee | |
Comment 10•8 years ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #8) > Comment on attachment 8811255 [details] [diff] [review] > Prefer BMP BPP over ICO BPP, v1 > > I had this exact patch sitting in my patch queue forgotten! > > I'm confident it's correct because this is the comment I had on the patch: > > "Use the bits per pixel value we read from the bmp header inside icos, not > the value in the ico directory entry. > > If they disagree we want to use the bmp header value. Further mBitCount is a > union with the cursor hot spot is this is a CUR file, so it doesn't even > represent BPP in CUR files." > > On cursors mBitCount isn't valid because it represents the hotspot of the > cursor. > > There might be more bugs where we use fields of this union incorrectly, we > should have a followup for auditing that. The only questionable use case of mBitCount/mPlanes/mXHotspot/mYhotspot (which isn't protected by checking if it is a cursor) is here: http://searchfox.org/mozilla-central/rev/efcb1644e49c36445e75d89b434fdf4c84832c84/image/decoders/nsICODecoder.cpp#230 If there are multiple cursors in the ICO, we won't necessarily select the biggest one because we are checking the "BPP." We shouldn't check it in that case.
|
||
Comment 11•8 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/9876606891a7
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•