567350 - getComputedStyle requires both arguments to be supplied

Status: RESOLVED FIXED

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

Description

[Andy Earnshaw]

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:1.9.2.3) 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.

https://developer-stage.mozilla.org/en/DOM/window.getComputedStyle

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 - https://developer-stage.mozilla.org/en/DOM/CSSStyleDeclaration

Comment 1

[Boris Zbarsky [:bzbarsky]]

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

[Sergei Yakovlev]

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

[Sergei Yakovlev]

Somebody with canconfirm please change the status of this bug to NEW. Thanks.

Comment 4

[Andy Earnshaw]

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

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Make getComputedStyle's second argument optional.

Comment 6

[Alex Vincent [:WeirdAl]]

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

[Boris Zbarsky [:bzbarsky]]

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

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch for checkin

Comment 9

[Dão Gottwald [:dao]]

patching file content/base/test/Makefile.in
Hunk #1 FAILED at 412
1 out of 1 hunks FAILED -- saving rejects to file content/base/test/Makefile.in.rej

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch for checkin

Fixed, thanks.

Comment 11

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/c64505896023

Comment 12

[Dão Gottwald [:dao]]

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

[Boris Zbarsky [:bzbarsky]]

I'd be fine with doing that, I think.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 483503 [details] [diff] [review]
Patch for checkin

Approved for 1.9.2.12, a=dveditz for release-drivers

Comment 15

[LegNeato]

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 16

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3bd32a77c390

Comment 17

[Mark Banner (:standard8)]

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:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5b942ca87e90

Compiler errors:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6/1288172723.1288172904.2891.gz#err0

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

[Ronny Perinke]

I guess qsgen.py needs an update to for that fix. qsgen.py does not support optional ptr arguments. -> http://mxr.mozilla.org/mozilla1.9.2/source/js/src/xpconnect/src/qsgen.py#436

Comment 19

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/DOM/window.getComputedStyle

Updated Firefox 4 for developers as well.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 483503 [details] [diff] [review]
Patch for checkin

un-approving this patch for 1.9.2.13 -- we'll need one that rolls in the bustage fix.

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.