Bug 11888 – REGRESSION (r18320): Web Inspector panes broken

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Matt Lilek]

The web inspector panes no longer display.  Seem to have been broken by http://trac.webkit.org/projects/webkit/changeset/18320

[David Kilzer (:ddkilzer)]

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

[Mark Rowe (bdash)]

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.

[Dave Hyatt]

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.

[Mark Rowe (bdash)]

Created attachment 11943 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 11943 [details]
Patch

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

[Adam Roben (:aroben)]

Comment on attachment 11943 [details]
Patch

+        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

[Mark Rowe (bdash)]

Comment on attachment 11943 [details]
Patch

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.

[Mark Rowe (bdash)]

Created attachment 11944 [details]
Patch - fix Inspector and Drosera to do the right thing

[Dave Hyatt]

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

[Mark Rowe (bdash)]

Landed in r18374.

[Alexey Proskuryakov]

(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.