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.
Created attachment 14266 [details]
Ha! This test case actually crashes TOT due to a recent regression:
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.
(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.
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.
(In reply to comment #5)
> as part of the ObjC bindings.
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.
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.
Created attachment 14419 [details]
test just for href and src
Created attachment 14420 [details]
test just for href and src
Created attachment 14421 [details]
Patch so far. src and href still up in the air, so not for review
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.
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.
Created attachment 14555 [details]
element attr test
Created attachment 14556 [details]
doc attr test
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.
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.
Landed in r21470.