Last Comment Bug 567350 - getComputedStyle requires both arguments to be supplied
: getComputedStyle requires both arguments to be supplied
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: CSS Object Model (show other bugs)
: Trunk
: All All
-- normal with 2 votes (vote)
: mozilla2.0b7
Assigned To: :Ms2ger (⌚ UTC+1/+2)
: Jet Villegas (:jet)
Depends on:
  Show dependency treegraph
Reported: 2010-05-21 07:09 PDT by Andy Earnshaw
Modified: 2010-11-19 01:33 PST (History)
10 users (show)
dao+bmo: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Make getComputedStyle's second argument optional. (2.64 KB, patch)
2010-10-01 11:46 PDT, :Ms2ger (⌚ UTC+1/+2)
bzbarsky: review+
jst: approval2.0+
Details | Diff | Splinter Review
Patch for checkin (2.50 KB, patch)
2010-10-14 14:41 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Patch for checkin (2.52 KB, patch)
2010-10-15 09:35 PDT, :Ms2ger (⌚ UTC+1/+2)
dveditz: approval1.9.2.13-
Details | Diff | Splinter Review

Description User image Andy Earnshaw 2010-05-21 07:09:10 PDT
User-Agent:       Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; WOW64; Trident/4.0; Sky Broadband; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; InfoPath.3; .NET4.0C; .NET4.0E)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv: Gecko/20100401 Firefox/3.6.3

In Google Chrome's V8, Safari, Opera and even in Internet Explorer 9 Platform Preview, the pseudoElt argument is optional for getComputedStyle.  In Mozilla Firefox, it is a required argument even though the requirement is that null be passed for standard elements.

Not specifying the pseudoElt argument to getComputedStyle results in an error only in Firefox.  I believe this is largely due to the wording in the specification, which defines the parameter as

   "The pseudo element or null if none"

In JavaScript, it is uncommon to require a null value to be passed as an argument and leaving the argument undefined makes sense in this case as it is the last argument of the function.

Reproducible: Always

Steps to Reproduce:
1. Open console
2. Type window.getComputedStyle(document.body)
3. Press enter
Actual Results:  
An error occurs:

Error: uncaught exception: [Exception... "Not enough arguments"  nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)"  location: "JS frame :: javascript:%20window.getComputedStyle(document.body) :: <TOP_LEVEL> :: line 1"  data: no]

Expected Results:  
Method should return CSSStyleDeclaration object -
Comment 1 User image Boris Zbarsky [:bz] (still a bit busy) 2010-05-21 09:18:09 PDT
This is a general issue with ECMAScript bindings in Gecko.  Unless the argument is explicitly marked as "optional", it's required.  I believe this is correct behavior per existing specs (though so is the IE/Safari/Opera one).

It's not clear to me what behavior WebIDL specifies here; that part of it was in flux last I checked.

Current early drafts of CSSOM propose having a one-argument overload for getComputedStyle; once that stabilizes somewhat we would probably implement it.  It, too, is in flux enough that wasting time implementing it right now only to have to rewrite in the near future isn't worth it.
Comment 2 User image Sergei Yakovlev 2010-09-30 11:43:03 PDT
Boris, do you realize that this currently forces all web developers
to write `window.getComputedStyle(element, null)` *just* for
the Firefox? Every other browser takes one argument just fine.
Is it so hard to fix, really? I am perplexed by this.

P.S.  Sorry if I sound too angry/grumbling.
Comment 3 User image Sergei Yakovlev 2010-09-30 11:45:04 PDT
Somebody with canconfirm please change the status of this bug to NEW. Thanks.
Comment 4 User image Andy Earnshaw 2010-10-01 01:22:01 PDT
I wholeheartedly agree with Sergei.  I know of no other DOM methods that require you pass *null* for the final argument and I think it is just plain common sense to make it optional.  

It would be ideal for this issue to be fixed in Firefox 4.
Comment 5 User image :Ms2ger (⌚ UTC+1/+2) 2010-10-01 11:46:28 PDT
Created attachment 480188 [details] [diff] [review]
Make getComputedStyle's second argument optional.
Comment 6 User image Alex Vincent [:WeirdAl] 2010-10-01 12:14:02 PDT
Comment on attachment 480188 [details] [diff] [review]
Make getComputedStyle's second argument optional.

> [scriptable, uuid(0b9341f3-95d4-4fa4-adcd-e119e0db2889)]
> interface nsIDOMViewCSS : nsIDOMAbstractView
>-                                       in DOMString pseudoElt);
>+                                             [optional] in DOMString pseudoElt);

Update the IID?

That's the only thing I see off here.
Comment 7 User image Boris Zbarsky [:bz] (still a bit busy) 2010-10-01 13:34:17 PDT
Comment on attachment 480188 [details] [diff] [review]
Make getComputedStyle's second argument optional.

r=me.  Please make sure to NOT change the iid here when landing, no matter what Alex says.  This is a binary-compatible change, and a backwards-compatible change on the JS end, so does not need an iid change.  Which is good, because this couldn't land for 2.0 at this stage if it needed to change the iid.  ;)
Comment 8 User image :Ms2ger (⌚ UTC+1/+2) 2010-10-14 14:41:10 PDT
Created attachment 483287 [details] [diff] [review]
Patch for checkin
Comment 9 User image Dão Gottwald [:dao] 2010-10-15 08:25:42 PDT
patching file content/base/test/
Hunk #1 FAILED at 412
1 out of 1 hunks FAILED -- saving rejects to file content/base/test/
Comment 10 User image :Ms2ger (⌚ UTC+1/+2) 2010-10-15 09:35:00 PDT
Created attachment 483503 [details] [diff] [review]
Patch for checkin

Fixed, thanks.
Comment 11 User image Dão Gottwald [:dao] 2010-10-16 04:56:51 PDT
Comment 12 User image Dão Gottwald [:dao] 2010-10-22 00:49:40 PDT
Should this land on 1.9.2? Firefox 3.6 will live on a bit after 4.0 has been released, but web developers won't necessarily test their stuff there...
Comment 13 User image Boris Zbarsky [:bz] (still a bit busy) 2010-10-22 08:04:58 PDT
I'd be fine with doing that, I think.
Comment 14 User image Daniel Veditz [:dveditz] 2010-10-25 10:45:36 PDT
Comment on attachment 483503 [details] [diff] [review]
Patch for checkin

Approved for, a=dveditz for release-drivers
Comment 15 User image christian 2010-10-25 10:48:44 PDT
It would probably be worth it taking it all the way back to 1.9.1 before we cut it loose, correct? Would that not be worth the risk? This looks to be virtually no risk with only benefit.
Comment 17 User image Mark Banner (:standard8) 2010-10-27 03:09:38 PDT
I backed this out as it had busted all of the 1.9.2 builds and there wasn't any sign of anyone to ask on irc, nor any indication of a clobber being required:

Compiler errors:

In file included from /builds/slave/mozilla-1.9.2-linux/build/js/src/nanojit/nanojit.h:267,
                 from /builds/slave/mozilla-1.9.2-linux/build/js/src/jsbuiltins.h:45,
                 from dom_quickstubs.cpp:12:
/builds/slave/mozilla-1.9.2-linux/build/js/src/nanojit/Native.h:53:27: warning: anonymous variadic macros were introduced in C99
/builds/slave/mozilla-1.9.2-linux/build/js/src/nanojit/Native.h:54:23: warning: anonymous variadic macros were introduced in C99
/builds/slave/mozilla-1.9.2-linux/build/js/src/nanojit/Native.h:135:28: warning: anonymous variadic macros were introduced in C99
/builds/slave/mozilla-1.9.2-linux/build/js/src/jsfun.h: In function 'js_ArgsPrivateNative* js_GetArgsPrivateNative(JSObject*)':
/builds/slave/mozilla-1.9.2-linux/build/js/src/jsfun.h:233: warning: converting to non-pointer type 'unsigned int' from NULL
dom_quickstubs.cpp: In function 'JSBool nsIDOMViewCSS_GetComputedStyle(JSContext*, uintN, jsval*)':
dom_quickstubs.cpp:7559: error: expected primary-expression before ')' token
make[5]: *** [dom_quickstubs.o] Error 1
make[5]: Leaving directory `/builds/slave/mozilla-1.9.2-linux/build/obj-firefox/js/src/xpconnect/src'
Comment 18 User image Ronny Perinke 2010-10-27 07:26:15 PDT
I guess needs an update to for that fix. does not support optional ptr arguments. ->
Comment 19 User image Eric Shepherd [:sheppy] 2010-11-03 12:39:47 PDT
Documentation updated:

Updated Firefox 4 for developers as well.
Comment 20 User image Daniel Veditz [:dveditz] 2010-11-18 19:40:03 PST
Comment on attachment 483503 [details] [diff] [review]
Patch for checkin

un-approving this patch for -- we'll need one that rolls in the bustage fix.
Comment 21 User image :Ms2ger (⌚ UTC+1/+2) 2010-11-19 01:33:03 PST
FTR, I'm not planning to get 1.9.2 set up, if someone feels strongly that this should be on branch, please feel free to write a patch.

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