Bug 12249 - FCKeditor: <hr>, <ul> and <ol> have id="undefined"
Summary: FCKeditor: <hr>, <ul> and <ol> have id="undefined"
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL: http://www.fckeditor.net/nightly/brow...
Keywords: NeedsReduction
Depends on:
Blocks: 9915
  Show dependency treegraph
 
Reported: 2007-01-13 03:23 PST by webkit
Modified: 2007-04-19 06:29 PDT (History)
2 users (show)

See Also:


Attachments
Test Case for <hr> (668 bytes, text/html)
2007-03-09 04:17 PST, webkit
no flags Details
testcase demonstrating the bug in FF (374 bytes, text/html)
2007-03-09 13:55 PST, Justin Garcia
no flags Details
TC for id="undefined" (654 bytes, text/html)
2007-03-10 02:48 PST, webkit
no flags Details
fix the test case (4.20 KB, patch)
2007-03-10 14:00 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
Testcase with null 3rd param (692 bytes, text/html)
2007-03-11 14:26 PDT, Sam Weinig
no flags Details
unfinished patch (4.45 KB, patch)
2007-03-13 12:25 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
updated patch (14.39 KB, patch)
2007-03-16 08:23 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
more updated patch (15.19 KB, patch)
2007-03-20 11:17 PDT, Sam Weinig
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description webkit 2007-01-13 03:23:52 PST
Open the URL and create, by using the toolbar buttons, bulleted/numbered lists or include horizontal rules.

Switch to source. You will see that the <hr>, <ul> and <ol> have id="undefined".
Comment 1 webkit 2007-03-09 04:17:16 PST
Created attachment 13556 [details]
Test Case for <hr>
Comment 2 webkit 2007-03-09 04:21:52 PST
While working on the Test Case, I found out that actually the id value is "false" and not "undefined". Probably FCKeditor is transform the boolean false in a "undefined" string in some way.

In any case, the id="false" is not desirable.
Comment 3 Justin Garcia 2007-03-09 13:54:06 PST
The execCommands to insert these types of elements expect a string as the third argument execCommand((commandID, userInterface, value)), and use that argument to set the id of the inserted element.  At first I thought that FCKEditor might be omitting the value argument (which we support) since we turn omitted string arguments into "undefined", but then I found that FCKEditor is *actually* passing "undefined" for value.  The reason you don't see id="undefined" in FireFox is because they ignore the value parameter and don't use it to set the id of inserted elements, as per the spec.
Comment 4 Justin Garcia 2007-03-09 13:55:07 PST
Created attachment 13566 [details]
testcase demonstrating the bug in FF
Comment 5 Justin Garcia 2007-03-09 13:56:06 PST
Seems like we're behaving correctly, closing.
Comment 6 David Kilzer (:ddkilzer) 2007-03-09 16:21:40 PST
(In reply to comment #3)
> The reason you don't see id="undefined" in
> FireFox is because they ignore the value parameter and don't use it to set the
> id of inserted elements, as per the spec.

Filed this b.m.o. bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=373406

Comment 7 webkit 2007-03-10 02:45:39 PST
(In reply to comment #5)
> Seems like we're behaving correctly, closing.

According to the only "standards" we have regarding it, only the first parameter is required. The rest is optional. Check this out:
http://msdn.microsoft.com/workshop/author/dhtml/reference/constants/inserthorizontalrule.asp

So it is correct to call document.execCommand('InsertHorizontalRule'). Actually this is its most common usage, as it is quite uncommon to require an id for those elements (developers would be forced to implement a unique id provider logic just to avoid id duplications).

Just to correct my last comment, the TC is giving id="false" because it is actually passing "false" as the third parameter, instead of null. I'll be attaching a new TC which gives id="undefined" instead, by just omitting that parameter.
Comment 8 webkit 2007-03-10 02:48:03 PST
Created attachment 13574 [details]
TC for id="undefined"
Comment 9 Justin Garcia 2007-03-10 13:00:23 PST
> So it is correct to call document.execCommand('InsertHorizontalRule'). Actually
> this is its most common usage, as it is quite uncommon to require an id for
> those elements (developers would be forced to implement a unique id provider
> logic just to avoid id duplications).

You're right, if you omit the third parameter you see a bug, but on our end, it appears that FCKEditor is *actually passing* a second and third parameter, *not omitting them*.  Can you confirm this?  If this is the case then we're behaving correctly, setting the id to what we're given for the third parameter.
Comment 10 Alexey Proskuryakov 2007-03-10 13:35:43 PST
I think the problem is in incorrect execCommand definition in Document.idl, verifying a fix now.
Comment 11 Alexey Proskuryakov 2007-03-10 14:00:54 PST
Created attachment 13580 [details]
fix the test case

This fixes the test case, but not the original issue. As Justin says, it seems that "undefined" is actually given as a parameter to execCommand().

Please also note that Firefox does not pass this test case (and I haven't tested with IE).
Comment 12 webkit 2007-03-10 15:12:51 PST
The FCKeditor implementation could be reduced to something like this:

----

function ExecCommand( command, commandParam )
{
    document.execCommand( command, false, commandParam ) ;
}

ExecCommand('InsertHorizontalRule') ;

----

Obviously the implementation is much more complex than that, but the essence is that we have a generic ExecCommand that is called for all execCommand needs.

In the above case, commandParam is "undefined". By ECMA definition, "undefined == null". Calling back the execCommand definition for InsertHorizontalRule, the second and third parameters may be omitted or *set to null*.

I believe this is the way IE understands it, as it doesn't produce a id="undefined", and I think it would be the right approach to not behave differently.
Comment 13 Darin Adler 2007-03-10 20:14:11 PST
Comment on attachment 13580 [details]
fix the test case

r=me
Comment 14 Alexey Proskuryakov 2007-03-11 00:25:23 PST
Comment on attachment 13580 [details]
fix the test case

I have now verified that the test passes in WinIE; committed revision 20101.

Clearing review flag from the landed fix.
Comment 15 Alexey Proskuryakov 2007-03-11 10:58:06 PDT
(In reply to comment #12)
> By ECMA definition, "undefined == null". Calling back the execCommand definition for
> InsertHorizontalRule, the second and third parameters may be omitted or *set to null*.

While it is true that "undefined == null", this doesn't mean that "undefined" and "null" are the same. For example, they give different results when converted to string.

Looks like IE has a non-documented quirk when converting to what they call a "variant" (see <http://msdn.microsoft.com/workshop/author/dhtml/reference/methods/execcommand.asp>) - they treat undefined as null in this particular case. I'm not sure if it can be expressed in IDL, CC'ing Sam Weinig to comment on this.
Comment 16 Sam Weinig 2007-03-11 14:24:23 PDT
Disregarding the issue of conversion from undefined to null in the IDL for a moment for a moment, it seems we still don't have the correct behavior for null.  As far as I can tell, according to MS's "spec" calling

  document.execCommand('InsertHorizontalRule', false, null);

should be the same as calling

  document.execCommand('InsertHorizontalRule');

due to the text "Optional. String that specifies an id attribute for the horizontal line. May be set to null or omitted."

Now, regarding the conversion issue, I don't think we have an extended attribute (those things we enclose in square brackets ie. [ConvertNullToNullString]) but adding one is not that difficult.  I am going to add a test case that illustrate the first point in moment. 
Comment 17 Sam Weinig 2007-03-11 14:26:56 PDT
Created attachment 13588 [details]
Testcase with null 3rd param
Comment 18 Alexey Proskuryakov 2007-03-11 22:39:44 PDT
After looking at the MS documentation a bit more, I'm now wondering if execCommand just needs a custom implementation.

Different commands have different types for the third parameter, and converting null JS strings to null Strings won't help much if the expected parameter type is a number. Also, for many commands, all the parameters are documented as required (although that might be a lie - I've tried BackColor, and IE didn't complain about missing parameters).
Comment 19 Sam Weinig 2007-03-13 12:25:18 PDT
Created attachment 13614 [details]
unfinished patch
Comment 20 Darin Adler 2007-03-13 12:41:53 PDT
Comment on attachment 13614 [details]
unfinished patch

It's usually not good to base things on argCount. Instead it's better to look at whether args are undefined. Passing undefined should normally have the same behavior as not passing additional arguments at all. Since undefined toBoolean becomes false, and undefined to valueToStringWithUndefinedOrNullCheck becomes a null strings I don't think we need the if statements at all in JSDocument::execCommand. If we need them, why?
Comment 21 Sam Weinig 2007-03-16 08:23:29 PDT
Created attachment 13666 [details]
updated patch
Comment 22 Sam Weinig 2007-03-20 11:17:15 PDT
Created attachment 13718 [details]
more updated patch

I've further updated the patch to support optional arguments for Objective-C (in the hack-a-licious way that we do it for the time being) and changed the test case so that it doesn't rely on a WebKit quirk of selecting the contents of an editable context on initial focus().
Comment 23 Darin Adler 2007-03-21 09:34:05 PDT
Comment on attachment 13718 [details]
more updated patch

OK, r=me
Comment 24 Justin Garcia 2007-04-18 12:53:14 PDT
Sam you should land this when you get a chance.
Comment 25 Sam Weinig 2007-04-19 06:29:14 PDT
Landed in r20948.