WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
12249
FCKeditor: <hr>, <ul> and <ol> have id="undefined"
https://bugs.webkit.org/show_bug.cgi?id=12249
Summary
FCKeditor: <hr>, <ul> and <ol> have id="undefined"
webkit
Reported
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".
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
webkit
Comment 1
2007-03-09 04:17:16 PST
Created
attachment 13556
[details]
Test Case for <hr>
webkit
Comment 2
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.
Justin Garcia
Comment 3
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.
Justin Garcia
Comment 4
2007-03-09 13:55:07 PST
Created
attachment 13566
[details]
testcase demonstrating the bug in FF
Justin Garcia
Comment 5
2007-03-09 13:56:06 PST
Seems like we're behaving correctly, closing.
David Kilzer (:ddkilzer)
Comment 6
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
webkit
Comment 7
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.
webkit
Comment 8
2007-03-10 02:48:03 PST
Created
attachment 13574
[details]
TC for id="undefined"
Justin Garcia
Comment 9
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.
Alexey Proskuryakov
Comment 10
2007-03-10 13:35:43 PST
I think the problem is in incorrect execCommand definition in Document.idl, verifying a fix now.
Alexey Proskuryakov
Comment 11
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).
webkit
Comment 12
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.
Darin Adler
Comment 13
2007-03-10 20:14:11 PST
Comment on
attachment 13580
[details]
fix the test case r=me
Alexey Proskuryakov
Comment 14
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.
Alexey Proskuryakov
Comment 15
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.
Sam Weinig
Comment 16
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.
Sam Weinig
Comment 17
2007-03-11 14:26:56 PDT
Created
attachment 13588
[details]
Testcase with null 3rd param
Alexey Proskuryakov
Comment 18
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).
Sam Weinig
Comment 19
2007-03-13 12:25:18 PDT
Created
attachment 13614
[details]
unfinished patch
Darin Adler
Comment 20
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?
Sam Weinig
Comment 21
2007-03-16 08:23:29 PDT
Created
attachment 13666
[details]
updated patch
Sam Weinig
Comment 22
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().
Darin Adler
Comment 23
2007-03-21 09:34:05 PDT
Comment on
attachment 13718
[details]
more updated patch OK, r=me
Justin Garcia
Comment 24
2007-04-18 12:53:14 PDT
Sam you should land this when you get a chance.
Sam Weinig
Comment 25
2007-04-19 06:29:14 PDT
Landed in
r20948
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug