getComputedStyle requires both arguments to be supplied

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
DOM: CSS Object Model
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Andy Earnshaw, Assigned: Ms2ger)

Tracking

({dev-doc-complete})

Trunk
mozilla2.0b7
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(status1.9.2 ?)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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
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.
Whiteboard: DUPEME

Comment 2

7 years ago
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

7 years ago
Somebody with canconfirm please change the status of this bug to NEW. Thanks.
(Reporter)

Comment 4

7 years ago
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.
(Assignee)

Updated

7 years ago
Assignee: nobody → Ms2ger
Version: unspecified → Trunk
(Assignee)

Comment 5

7 years ago
Created attachment 480188 [details] [diff] [review]
Make getComputedStyle's second argument optional.
(Assignee)

Updated

7 years ago
Attachment #480188 - Flags: review?(bzbarsky)
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 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.  ;)
Attachment #480188 - Flags: review?(bzbarsky) → review+
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

7 years ago
Attachment #480188 - Flags: approval2.0?
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED
QA Contact: general → style-system

Updated

7 years ago
Attachment #480188 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 8

7 years ago
Created attachment 483287 [details] [diff] [review]
Patch for checkin
Attachment #480188 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
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
(Assignee)

Comment 10

7 years ago
Created attachment 483503 [details] [diff] [review]
Patch for checkin

Fixed, thanks.
Attachment #483287 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/c64505896023
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: DUPEME
Target Milestone: --- → mozilla2.0b8
Keywords: dev-doc-needed
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...
I'd be fine with doing that, I think.

Updated

7 years ago
Attachment #483503 - Flags: approval1.9.2.12?
Comment on attachment 483503 [details] [diff] [review]
Patch for checkin

Approved for 1.9.2.12, a=dveditz for release-drivers
Attachment #483503 - Flags: approval1.9.2.12? → approval1.9.2.12+

Comment 15

7 years ago
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.

Updated

7 years ago
Keywords: checkin-needed
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3bd32a77c390
status1.9.2: --- → .13-fixed
Keywords: checkin-needed
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'
status1.9.2: .13-fixed → ?

Comment 18

7 years ago
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
Target Milestone: mozilla2.0b8 → mozilla2.0b7
Documentation updated:

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

Updated Firefox 4 for developers as well.
Keywords: dev-doc-needed → dev-doc-complete
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.
Attachment #483503 - Flags: approval1.9.2.13+ → approval1.9.2.13-
(Assignee)

Comment 21

7 years ago
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.
You need to log in before you can comment on or make changes to this bug.