Bug 13541 - REGRESSION: Setting a DOMString attribute to JS null in the JS bindings should default to converting to the empty string
Summary: REGRESSION: Setting a DOMString attribute to JS null in the JS bindings shoul...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh OS X 10.4
: P1 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar, Regression
Depends on: 13584
Blocks:
  Show dependency treegraph
 
Reported: 2007-04-29 07:55 PDT by Sam Weinig
Modified: 2007-05-14 18:56 PDT (History)
1 user (show)

See Also:


Attachments
testcase (17.05 KB, text/html)
2007-04-29 08:00 PDT, Sam Weinig
no flags Details
test just for href and src (2.27 KB, text/html)
2007-05-08 20:11 PDT, Alice Liu
no flags Details
test just for href and src (2.27 KB, text/html)
2007-05-08 20:11 PDT, Alice Liu
no flags Details
Patch so far. src and href still up in the air, so not for review (58.59 KB, patch)
2007-05-08 20:14 PDT, Alice Liu
no flags Details | Formatted Diff | Diff
updated patch [WIP] (126.64 KB, patch)
2007-05-14 16:20 PDT, Sam Weinig
alice.barraclough: review+
Details | Formatted Diff | Diff
element attr test (25.53 KB, text/html)
2007-05-14 16:45 PDT, Sam Weinig
no flags Details
doc attr test (3.50 KB, text/html)
2007-05-14 16:46 PDT, Sam Weinig
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2007-04-29 07:55:04 PDT
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.
Comment 1 Sam Weinig 2007-04-29 08:00:34 PDT
Created attachment 14266 [details]
testcase
Comment 2 Eric Seidel (no email) 2007-05-04 00:35:29 PDT
Ha!  This test case actually crashes TOT due to a recent regression:

http://bugs.webkit.org/show_bug.cgi?id=13584
Comment 3 Darin Adler 2007-05-04 22:18:51 PDT
<rdar://problem/5183689>
Comment 4 Alice Liu 2007-05-07 20:31:18 PDT
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.
Comment 5 Darin Adler 2007-05-08 08:11:47 PDT
(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.
Comment 6 Darin Adler 2007-05-08 09:46:18 PDT
(In reply to comment #5)
> as part of the ObjC bindings.

Oops. I mixed two things up here. I meant the JavaScript bindings!
Comment 7 Sam Weinig 2007-05-08 14:34:00 PDT
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.
Comment 8 Alice Liu 2007-05-08 20:09:11 PDT
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. 
Comment 9 Alice Liu 2007-05-08 20:11:49 PDT
Created attachment 14419 [details]
test just for href and src
Comment 10 Alice Liu 2007-05-08 20:11:57 PDT
Created attachment 14420 [details]
test just for href and src
Comment 11 Alice Liu 2007-05-08 20:14:08 PDT
Created attachment 14421 [details]
Patch so far.  src and href still up in the air, so not for review
Comment 12 Sam Weinig 2007-05-12 14:46:31 PDT
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.
Comment 13 Sam Weinig 2007-05-14 16:20:22 PDT
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.
Comment 14 Sam Weinig 2007-05-14 16:45:46 PDT
Created attachment 14555 [details]
element attr test
Comment 15 Sam Weinig 2007-05-14 16:46:26 PDT
Created attachment 14556 [details]
doc attr test
Comment 16 Alice Liu 2007-05-14 17:36:53 PDT
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.  

Comment 17 Alice Liu 2007-05-14 17:57:29 PDT
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. 
Comment 18 Sam Weinig 2007-05-14 18:56:36 PDT
Landed in r21470.