Bug 12249 – FCKeditor: <hr>, <ul> and <ol> have id="undefined"

• Component: HTML Editing
• Status: RESOLVED FIXED

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

Created attachment 13556 [details]
Test Case for <hr>

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.

[Justin Garcia]

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.

[Justin Garcia]

Created attachment 13566 [details]
testcase demonstrating the bug in FF

[Justin Garcia]

Seems like we're behaving correctly, closing.

[David Kilzer (:ddkilzer)]

(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

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

Created attachment 13574 [details]
TC for id="undefined"

[Justin Garcia]

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

[Alexey Proskuryakov]

I think the problem is in incorrect execCommand definition in Document.idl, verifying a fix now.

[Alexey Proskuryakov]

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

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.

[Darin Adler]

Comment on attachment 13580 [details]
fix the test case

r=me

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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

[Sam Weinig]

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. 

[Sam Weinig]

Created attachment 13588 [details]
Testcase with null 3rd param

[Alexey Proskuryakov]

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

[Sam Weinig]

Created attachment 13614 [details]
unfinished patch

[Darin Adler]

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?

[Sam Weinig]

Created attachment 13666 [details]
updated patch

[Sam Weinig]

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

[Darin Adler]

Comment on attachment 13718 [details]
more updated patch

OK, r=me

[Justin Garcia]

Sam you should land this when you get a chance.

[Sam Weinig]

Landed in r20948.