Last Comment Bug 424386 - colorDepth / pixelDepth return 24 instead of 32
: colorDepth / pixelDepth return 24 instead of 32
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: DOM: CSS Object Model (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: mozilla1.9
Assigned To: Stuart Parmenter
:
Mentors:
http://www.quirksmode.org/dom/tests/s...
Depends on: 458847
Blocks: 466669
  Show dependency treegraph
 
Reported: 2008-03-21 10:25 PDT by John Resig
Modified: 2013-01-30 01:59 PST (History)
16 users (show)
sayrer: blocking1.9+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (1.34 KB, patch)
2008-03-21 12:25 PDT, Stuart Parmenter
vladimir: review+
vladimir: approval1.9+
Details | Diff | Review

Description John Resig 2008-03-21 10:25:49 PDT
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
Comment 1 John Resig 2008-03-21 10:44:41 PDT
I forgot to mention but this is also part of the W3C CSSOM working draft:
http://www.w3.org/TR/cssom-view/
Comment 2 Stuart Parmenter 2008-03-21 12:25:19 PDT
Created attachment 311022 [details] [diff] [review]
fix
Comment 3 Vladimir Vukicevic [:vlad] [:vladv] 2008-03-21 12:33:05 PDT
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.
Comment 4 Stuart Parmenter 2008-03-21 12:52:14 PDT
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.
Comment 5 Stuart Parmenter 2008-03-21 12:58:57 PDT
dbaron/hixie: any thoughts here?
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-03-21 13:52:02 PDT
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?
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-03-21 13:54:21 PDT
(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.)
Comment 8 Vladimir Vukicevic [:vlad] [:vladv] 2008-03-21 14:31:56 PDT
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.
Comment 9 Damon Sicore (:damons) 2008-04-01 14:28:59 PDT
+'ing on this until we make a decision.  David, can you please determine what we should do to match spec if anything?
Comment 10 Damon Sicore (:damons) 2008-04-02 14:36:15 PDT
Fixing priority to P2.
Comment 11 Anne (:annevk) 2008-04-02 18:42:38 PDT
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.
Comment 12 Damon Sicore (:damons) 2008-04-08 11:48:49 PDT
David, time to make a decision here.  I want to get this off the list by the end of the day today.
Comment 13 Vladimir Vukicevic [:vlad] [:vladv] 2008-04-09 12:08:27 PDT
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.
Comment 14 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-04-09 22:15:02 PDT
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.
Comment 15 Anne (:annevk) 2008-04-10 08:20:01 PDT
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...
Comment 16 Damon Sicore (:damons) 2008-04-14 13:32:02 PDT
Did this land, Vlad?
Comment 17 Stuart Parmenter 2008-04-15 07:17:14 PDT
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.
Comment 18 Serge Gautherie (:sgautherie) 2008-04-15 09:43:41 PDT
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));
>+    }
Comment 19 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-04-15 16:01:50 PDT
(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.
Comment 20 Carsten Book [:Tomcat] 2008-05-08 14:57:28 PDT
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
Comment 21 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-11-25 08:26:12 PST
I filed bug 466669 on changing the Windows widget code (see there for more details).
Comment 22 Chris Ross 2013-01-30 01:59:44 PST
(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)

Note You need to log in before you can comment on or make changes to this bug.