Bug 11888

Summary: REGRESSION (r18320): Web Inspector panes broken
Product: WebKit Reporter: Matt Lilek (pewtermoose) <mlilek@apple.com>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned@lists.webkit.org>
Status: RESOLVED FIXED    
Severity: Major CC: ap@webkit.org, aroben@webkit.org, ddkilzer@webkit.org, mitz@webkit.org, mrowe@apple.com
Priority: P1    
Version: 420+   
Hardware: Macintosh   
OS: Mac OS X 10.4   
Attachments:
Description Flags
Patch
mrowe: review-
Patch - fix Inspector and Drosera to do the right thing ggaren: review+

Description From 2006-12-20 01:46:36 PST
The web inspector panes no longer display.  Seem to have been broken by http://trac.webkit.org/projects/webkit/changeset/18320
------- Comment #1 From 2006-12-20 02:59:26 PST -------
Whenever the Inspector is brought up or closed, this error is printed to the console:

/absolute/path/to/WebCore/bindings/objc/WebScriptObject.mm:198:[5896]  JavaScript exception:  SYNTAX_ERR: DOM Exception 12

Whenever "Node", "Style", "Metrics" or "Properties" is clicked, this error is printed to the console:

(event handler):SYNTAX_ERR: DOM Exception 12

Using a locally-built debug build of WebKit r18334 with Safari 2.0.4 (419.3) on Mac OS X 10.4.8 (8L127).
------- Comment #2 From 2006-12-20 20:46:59 PST -------
This happens because several places in the JavaScript for both Drosera and the Web Inspector code does myElement.style.display = null;, which now throws an exception.  A little investigation shows that Firefox doesn't throw an exception on this construct, while IE 7 does.

We expect "myElement.style.display = null" to remove the value of the display property, effectively resetting it to its default value.
------- Comment #3 From 2006-12-20 20:54:06 PST -------
Seems kind of pointless to throw an exception in this case... sounds like we're setting ourselves up for compatibility problems if we throw and Firefox doesn't, since we'll often share the "standards" code path.
------- Comment #4 From 2006-12-20 21:23:21 PST -------
Created an attachment (id=11943) [details]
Patch
------- Comment #5 From 2006-12-20 21:35:42 PST -------
(From update of attachment 11943 [details])
Could we wait just a bit more before reverting a behavior that's (a) standard-compliant AND (b) matches WinIE? So far, there were no reports of Web compatibility problems (except for www.apple.com/getamac, where we share IE behavior now).

To correctly remove a property, one should just set it to empty: "myElement.style.display = ''".

I believe this Inspector JS bug needs to be fixed even if we decide to disable exceptions in this case, so r-.
------- Comment #6 From 2006-12-20 21:41:01 PST -------
(From update of attachment 11943 [details])
+        Don't throw an exception when setting the value of a SSStyleDeclaration.

Typo: _C_SSStyleDeclaration

+        if (value->isNull() || frame->settings()->shouldUseDashboardBackwardCompatibilityMode()) {

It would be great to have a comment here about why we're allowing setting properties to null.

r=me
------- Comment #7 From 2006-12-20 22:47:00 PST -------
(From update of attachment 11943 [details])
I discussed this with Alexey in #webkit, and we decided that it's best to leave the JS bindings as-is unless we come across websites relying on null resetting a property to its initial value.  I'll follow up with a patch that changes the inspector and debugger's erroneous use to the correct method of using removeProperty.
------- Comment #8 From 2006-12-21 00:10:24 PST -------
Created an attachment (id=11944) [details]
Patch - fix Inspector and Drosera to do the right thing
------- Comment #9 From 2006-12-21 00:12:52 PST -------
I'm pretty nervous about this, but I guess we can wait and see.  In general, the more exceptions you throw, the more likely you are to have really horrible site errors (because of buggy JS that doesn't think any exception is going to happen).
------- Comment #10 From 2006-12-21 00:39:21 PST -------
Landed in r18374.
------- Comment #11 From 2006-12-21 01:14:46 PST -------
(In reply to comment #7)
> I discussed this with Alexey in #webkit, and we decided that it's best to leave
> the JS bindings as-is unless we come across websites relying on null resetting
> a property to its initial value. 

I guess we convinced each other - I now think that it would be the right thing to support setting the value to null :-). What I didn't consider earlier was that Firefox not only doesn't throw - it actually handles setting the value to null the same way as setting it to "". I thought that FF was just hiding the error, as it does for invalid values.

However, this should be fixed in setProperty(), not in the JS binding code for assignment.