REGRESSION: Setting a DOMString attribute to JS null in the JS bindings should default to converting to the empty string

• Component: DOM
• Status: RESOLVED FIXED

[Sam Weinig]

A follow up to bug 13448.  When passing a JS null value to an attribute that takes a DOMString, the default should be to convert the null value to the empty string.  This is a regression from shipping Safari.  I have a test case for all of the HTML DOM bindings attributes but I have yet to test the core DOM attributes or any of the functions.  Test case forthcoming.

[Sam Weinig]

Created attachment 14266 [details]
testcase

[Eric Seidel (no email)]

Ha!  This test case actually crashes TOT due to a recent regression:

http://bugs.webkit.org/show_bug.cgi?id=13584

[Darin Adler]

<rdar://problem/5183689>

[Alice Liu]

It looks like when a UString is constructed, it's m_rep is set to Rep::null, a static.  If the element's attribute is set, even set to JS null value, the UString is set as a string representation of "null", and not Rep::null.

[Darin Adler]

(In reply to comment #4)
> It looks like when a UString is constructed, it's m_rep is set to Rep::null, a
> static.  If the element's attribute is set, even set to JS null value, the
> UString is set as a string representation of "null", and not Rep::null.

That's mixing a couple concepts up.

It's true that new UString instances are "null strings", and there are also distinct "empty strings". But the issue here has nothing to do with null UString instances. Instead it has to do with what we do with null values when converting to strings (not KJS::UString, but WebCore::String) as part of the ObjC bindings.

By default, the JavaScript binding converts null values to the string "null". But you can specify in the .idl file a different behavior, for example using the [ConvertNullToNullString] directive. The valueToStringWithNullCheck function from kjs_binding.h has the same behavior.

To fix this bug we probably need to add some more uses of [ConvertNullToNullString] and valueToStringWithNullCheck.

We could even consider changing the default behavior, but I think that's not a good idea.

[Darin Adler]

(In reply to comment #5)
> as part of the ObjC bindings.

Oops. I mixed two things up here. I meant the JavaScript bindings!

[Sam Weinig]

I would be in favor of changing the default behavior, at least for attributes, as every attribute I have tested so far has this as its desired behavior.  If we don't change the default behavior we will have to add [ConvertNullToNullString] to a ton of attributes.

[Alice Liu]

I talked briefly with Darin about changing the default behavior.  It's probably best, for now, to just change the behavior for each attribute we want.  It's a risk to change the behavior for all elements during a time we're supposed to be stabilizing and without a truly comprehensive test.

I'm concerned about href and src attributes though.  Firefox will set href and src attributes to the parent document's location instead of empty string if the attribute is assigned to null.  Safari 2.0 and WinIE don't.  Do we want to do this as well?  i think in the case of frame and iframe, we do, but maybe not others.  I'm not sure, though. 

[Alice Liu]

Created attachment 14419 [details]
test just for href and src

[Alice Liu]

Created attachment 14420 [details]
test just for href and src

[Alice Liu]

Created attachment 14421 [details]
Patch so far.  src and href still up in the air, so not for review

[Sam Weinig]

After further thought, I agree, not changing the default behavior seems like the responsible thing to do during the stabilization period.  Regarding, the behavior of href and src, I think that it is not a bindings issue and should be addressed in another bug.  For either behavior we have to call valueToStringWithNullCheck() so that the implementation knows that it is a null value and not the string 'null'.  There are a few places you missed including attributes for Core DOM Node and Document.

[Sam Weinig]

Created attachment 14554 [details]
updated patch [WIP]

This patch updates Alice's patch to include testing for null for src and href attributes, adds Document attribute changes and adds two updated tests.

[Sam Weinig]

Created attachment 14555 [details]
element attr test

[Sam Weinig]

Created attachment 14556 [details]
doc attr test

[Alice Liu]

i think we should add [ConvertNullToNullString] to the following attributes for sake of consistency, even though just adding that doesn't do anything (a change to kjs_html.cpp was needed as well).

HTMLAppletElement.idl:                 attribute DOMString codeBase;
HTMLElement.idl:                 attribute DOMString id;
HTMLElement.idl:                 attribute DOMString lang;
HTMLElement.idl:                 attribute DOMString dir;
HTMLElement.idl:                 attribute DOMString className;
HTMLElement.idl:                 attribute DOMString innerHTML
HTMLElement.idl:                 attribute DOMString innerText
HTMLElement.idl:                 attribute DOMString outerHTML
HTMLElement.idl:                 attribute DOMString outerText
HTMLElement.idl:                 attribute DOMString contentEditable;
HTMLEmbedElement.idl:                 attribute DOMString src;
HTMLInputElement.idl:                 attribute DOMString       type;

I think that list covers them all in the html/ directory.  

r=me, regardless of that change.  

[Alice Liu]

I've discussed with Sam that I discovered using get/setAttribute versus dot notation yields different results.  There should be a separate bug for that. 

[Sam Weinig]

Landed in r21470.