Closed Bug 424386 Opened 16 years ago Closed 16 years ago

colorDepth / pixelDepth return 24 instead of 32

Categories

(Core :: DOM: CSS Object Model, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9

People

(Reporter: jeresig, Assigned: pavlov)

References

()

Details

Attachments

(1 file)

According to PPK, in Gecko 1.9, the colorDepth and pixelDepth properties no longer return a correct value:
http://www.quirksmode.org/dom/w3c_cssom.html#screenview

A simple test case is provided here (must be using a 32 bit display):
http://www.quirksmode.org/dom/tests/screenview.html#color
I forgot to mention but this is also part of the W3C CSSOM working draft:
http://www.w3.org/TR/cssom-view/
Attached patch fixSplinter Review
Assignee: nobody → pavlov
Status: NEW → ASSIGNED
Attachment #311022 - Flags: review?(vladimir)
The current code is certainly wrong, as it always returns 24.  We're not consistent here across platforms, though -- on Linux, we've returned 24, and on MacOS X, we return 24.  Safari on OSX returns 24.

The spec specifies that it's the "lowest number of bits from the color components of the output device".  That's 24 -- 32 bits includes 8 bits of padding, or alpha, depending on their usage, but it's not a color component.  On 8bpp and 16bpp displays, we return 8 and 16.  I would assume that if a high-depth 48bpp display were to come around (16 bits per color channel), we should return 48, and not 64.

So, I think we -should- be returning 24 here and not 32, for that particular case.
Every single browser on Windows (IE, Safari, Opera, Firefox 2) returns 32.

While I'm not convinced that many people are using this property, I don't see a good reason for us to change it away from what most people should be expecting.

If we want to support 48bpp displays at some point, we should probably return 48 instead of 64.
dbaron/hixie: any thoughts here?
Well, the spec in question was designed to match what implementations do, I think, so if it doesn't it should probably be fixed.

What do implementations do?
(That said, it doesn't make any sense to me to return 32; 24 seems correct.  Having 8-bits for alpha-channel doesn't have anything to do with the device.)
On Linux and OSX, we return 24.  Safari does as well.  On Win32, everyone returned 32, presumaby because that's what the GetDeviceCaps call that everyone was using (with BITSPIXEL) was doing... but it's inconsistent with 8bpp and 16bpp displays, and would be inconsistent with any future high-bit-depth displays.
Flags: blocking1.9?
+'ing on this until we make a decision.  David, can you please determine what we should do to match spec if anything?
Assignee: pavlov → dbaron
Status: ASSIGNED → NEW
Flags: blocking1.9? → blocking1.9+
Fixing priority to P2.
Priority: -- → P2
http://dev.w3.org/csswg/cssom-view/#screen-colordepth is indeed wrong. Sorry about that. It actually suggests 8 for a 24bit system (assuming each color has an identical amount of bits). I think 24 would make the most sense if the other 8 bits are not used for colors. Let me know what you decide and I can update the draft.
David, time to make a decision here.  I want to get this off the list by the end of the day today.
Comment on attachment 311022 [details] [diff] [review]
fix

Either way, we need to take this patch, so let's do this.

David and I both think that we should add an  if (depth == 32) return 24;  check, though, but we need this patch either way.
Attachment #311022 - Flags: review?(vladimir)
Attachment #311022 - Flags: review+
Attachment #311022 - Flags: approval1.9+
I agree that 24 makes more sense than 32.

For what it's worth, media queries also gives the bits-per-color-component answer:
http://dev.w3.org/csswg/css3-mediaqueries/#color
...which seems like an annoying answer for 16-bit color, and especially for distinguishing 15-bit vs. 16-bit.
I updated the CSSOM View specification to say it is about the total amount of color bits, rather than per component. This suggests that the attributes should return 24 rather than 32.

Changing media queries might be possible if it's proposed quickly...
Keywords: checkin-needed
Did this land, Vlad?
i checked this in.  if we want to change the behavior of the dom code to return 32 instead of 24, we should file a new bug and change the dom code.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9
Version: unspecified → Trunk
Comment on attachment 311022 [details] [diff] [review]
fix

>+    nsCOMPtr<nsIScreen> primaryScreen;
>+    if (mDepth == 0) {

Nit: any reason not to write it:
    if (mDepth == 0) {
      nsCOMPtr<nsIScreen> primaryScreen;

>+        mScreenManager->GetPrimaryScreen(getter_AddRefs(primaryScreen));
>+        primaryScreen->GetColorDepth(reinterpret_cast<PRInt32 *>(&mDepth));
>+    }
(In reply to comment #17)
> i checked this in.  if we want to change the behavior of the dom code to return
> 32 instead of 24, we should file a new bug and change the dom code.

Are you sure there isn't a Windows API involved that returns 32 when the real value is 24?  See comment 8.  It seems like the gfx layer ought to return the correct result, regardless of Windows API quirks.
verified fixed using the testurl from comment #0 and and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050614 Minefield/3.0pre
Status: RESOLVED → VERIFIED
Depends on: 458847
I filed bug 466669 on changing the Windows widget code (see there for more details).
(In reply to Stuart Parmenter from comment #4)
> Every single browser on Windows (IE, Safari, Opera, Firefox 2) returns 32.
> 
> While I'm not convinced that many people are using this property, I don't
> see a good reason for us to change it away from what most people should be
> expecting.

I quite agree.
My pages check screen resolution and color depth before diplaying large photos - and WARN THE USER if they are less than recommended. So it was a user who reported this problem to me as he knew his screen was set at 32 bits but he was being told 24 bits - but ONLY in SeaMonkey, other browsers showing the same value as Windows.
So it is USERS who get confused by this illogical decision to behave differently from everyone else...

(Duplicate of Bug 835914)
See Also: → 1875345
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: