Bug 10934 – REGRESSION: prototype.js logs error (HTMLFormElement) on webkit builds

• Component: DOM
• Status: RESOLVED FIXED

[James Berry]

Prototype v 1.5.0_rc1 from http://script.aculo.us/downloads logs the following message as soon as it is included:

 "ReferenceError: Can't find variable: HTMLFormElement"

This can be seen by simply visiting the scrapaculous page at http://script.aculo.us/, which apparently includes that prototype.js file.

This error does not occur with production Safari versions.

r16421.

[James Berry]

God, did I really write "scrapaculous"? ;)

[Alexey Proskuryakov]

This script assumes that WebKit would add window.HTMLElement and window.HTMLFormElement at the same time, but this didn't happen - we now have the former, but not the latter (same issue with HTMLInputElement, HTMLTextAreaElement, HTMLSelectElement).

Still, this is technically a regression, so upgrading to P1.

[Alexey Proskuryakov]

This library is used at apple.com, so opening www.apple.com results in the same error message, too (in prototype.js and effects.js).

[Stephanie Lewis]

radar 4823135

[John Sullivan]

Are there any user symptoms of this, or just the log message?

[David Kilzer (:ddkilzer)]

www.apple.com doesn't seem to generate these errors anymore, but script.aculo.us does:

http://script.aculo.us/

[7173] http://script.aculo.us/prototype.js line 1318: ReferenceError: Can't find variable: HTMLFormElement
[7173] http://script.aculo.us/effects.js line 1318: ReferenceError: Can't find variable: HTMLFormElement

The line of code in question (in both files) is:

    copy(Form.Methods, HTMLFormElement.prototype);

I assume this "doesn't matter" until Form.Methods is used by an action that the user performs.  Looks like WebKit is missing a JavaScript object/prototype.

[David Kilzer (:ddkilzer)]

Here's the JavaScript that builds HTMLFormElement:

if (!window.HTMLElement && /Konqueror|Safari|KHTML/.test(navigator.userAgent)) {
  /* Emulate HTMLElement, HTMLFormElement, HTMLInputElement, HTMLTextAreaElement,
     and HTMLSelectElement in Safari */
  ['', 'Form', 'Input', 'TextArea', 'Select'].each(function(tag) {
    var klass = window['HTML' + tag + 'Element'] = {};
    klass.prototype = document.createElement(tag ? tag.toLowerCase() : 'div').__proto__;
  });
}

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> Here's the JavaScript that builds HTMLFormElement:
> 
> if (!window.HTMLElement && /Konqueror|Safari|KHTML/.test(navigator.userAgent))

In WebKit ToT, window.HTMLElement is defined, so this code no longer runs, which may be the source of the issue.  Note that commenting out the if statement produces a different error, though in the [].each() expression:

TypeError: Value undefined (result of expression ["","Form","Input","TextArea","Select"].each) is not object.

[Alexey Proskuryakov]

Created attachment 12579 [details]
proposed fix

There is still (another) exception logged when opening script.aculo.us, but it also occurs in Firefox, so I guess it's not our bug.

[David Kilzer (:ddkilzer)]

(In reply to comment #9)
> Created an attachment (id=12579) [edit]
> proposed fix

A couple items I noticed in the patch:

--- WebCore/bindings/scripts/CodeGeneratorJS.pm	(revision 19001)
+++ WebCore/bindings/scripts/CodeGeneratorJS.pm	(working copy)
-    die "Don't know whether a JS value can fail conversion to type $type."
+    die "Don't know whether a JS value ${type} can fail conversion to type $type."

In this code, ${type} is the same value as $type.  Was there a different expression that you wanted to use other than "${type}"?

--- WebCore/html/HTMLSelectElement.idl	(revision 19001)
+++ WebCore/html/HTMLSelectElement.idl	(working copy)
         // Modified in DOM Level 2:
-        // FIXME: the version of this attribute according to the
-        // dom spec is commented out below it.
+#if defined(LANGUAGE_OBJECTIVE_C)
         readonly attribute long            length;
-              /* attribute unsigned long   length
-                    setter raises (DOMException); */
+#else
+                 attribute unsigned long   length
+                    setter raises (DOMException);
+#endif

Why not "fix" the ObjC binding here to match the JavaScript binding?  Aren't we cleaning up the ObjC bindings by generating them anyway?

+#if defined(LANGUAGE_JAVASCRIPT)
+        // In JS, we support both options index and options object parameters - this cannot be autogenerated now.
+        [Custom] void      remove(/* 1 */);
+#else
         void               remove(in long index);
+#endif

It's been a while since I worked on generated bindings, but can the above use the "[Optional]" keyword to provide an overloaded method call?  (See DOMWindow.idl, Element.idl or HTMLOptionsCollection.idl for examples.  I see [Optional] is not implemented for generated ObjC, though.)

         void               remove(in [Optional] long index);

[Darin Adler]

Comment on attachment 12579 [details]
proposed fix

+        select.remove(((HTMLOptionElement*)element)->index());
+    else
+        select.remove(int(args[0]->toNumber(exec)));

I suggest static_cast syntax on these two lines.

+JSValue* toJS(ExecState* exec, HTMLOptionsCollection* c)

This seems to belong in kjs_html.h/cpp not kjs_dom.h/cpp.

+    for (unsigned i = 0; i < items.size(); ++i) {

If you're rewriting this function anyway, then please consider putting the items.size() call outside the loop. The compiler might optimize it anyway, but it might not.

Why do you need to make this change? Won't the bindings handle int for the length? It's not like we can have lengths that are greater than 31 bits.

+        [Custom] void      remove(/* 1 */);

What does the commented out 1 mean here? I find that comment confusing.

+//        attribute HTMLFrameElementConstructor HTMLFrameElement;
+//        attribute HTMLFrameSetElementConstructor HTMLFrameSetElement;
+//        attribute HTMLIFrameElementConstructor HTMLIFrameElement;
+//        attribute HTMLObjectElementConstructor HTMLObjectElement;
+//        attribute HTMLTableCaptionElementConstructor HTMLTableCaptionElement;
+//        attribute HTMLTableCellElementConstructor HTMLTableCellElement;
+//        attribute HTMLTableColElementConstructor HTMLTableColElement;
+//        attribute HTMLTableElementConstructor HTMLTableElement;
+//        attribute HTMLTableRowElementConstructor HTMLTableRowElement;
+//        attribute HTMLTableSectionElementConstructor HTMLTableSectionElement;

These appear in the list, commented out without any indicate of why they're there or why they're commented out. Please either omit them or make it clearer what these commented-out lines mean.

The patch generally looks great to me, but I made enough comments about that I'm going to say review- this round.

[Alexey Proskuryakov]

(In reply to comment #10)

> In this code, ${type} is the same value as $type.  Was there a different
> expression that you wanted to use other than "${type}"?

  Um, thanks - turns out I actually misunderstood how this method worked. Reverted this change.

> Why not "fix" the ObjC binding here to match the JavaScript binding?  Aren't we
> cleaning up the ObjC bindings by generating them anyway?

  This accessor is in public interface, changing it isn't possible (an error is generated by the script if I try to).

> It's been a while since I worked on generated bindings, but can the above use
> the "[Optional]" keyword to provide an overloaded method call?

  [Optional] is used to overload on the number of arguments, while here it's the type that can be different.

[Alexey Proskuryakov]

(In reply to comment #11)
> I suggest static_cast syntax on these two lines.

  Done (this code was just moved from kjs_html.cpp).

> +JSValue* toJS(ExecState* exec, HTMLOptionsCollection* c)
> This seems to belong in kjs_html.h/cpp not kjs_dom.h/cpp.

  Indeed; done.

> +    for (unsigned i = 0; i < items.size(); ++i) {
> 
> If you're rewriting this function anyway, then please consider putting the
> items.size() call outside the loop. The compiler might optimize it anyway, but
> it might not.

  Unless there is a specific reason to do so, I'd like to keep it as is. This change would trade a little readability for a little performance - not something I normally do without profiling or at least knowing that the code in question is hot.

  There are many similar loops in this file, and none seem to take size computation outside the loop.

> Why do you need to make this change? Won't the bindings handle int for the
> length? It's not like we can have lengths that are greater than 31 bits.

  The auto-generated code produced a warning about comparing signed and unsigned values. I guess I could change the IDL, but changing DOM interface seemed cleaner.

> +        [Custom] void      remove(/* 1 */);
> What does the commented out 1 mean here? I find that comment confusing.

  This comment means that the function takes one parameter - this syntax is used in other IDL files that have custom functions (see, for example, CanvasRenderingcontext2d.idl). I think it's purely informational and is ignored by the code generator, but I haven't checked.

> These appear in the list, commented out without any indicate of why they're
> there or why they're commented out. Please either omit them or make it clearer
> what these commented-out lines mean.

  Done, added a comment.

[Alexey Proskuryakov]

Created attachment 12588 [details]
proposed fix

[Sam Weinig]

(In reply to comment #12)
> > Why not "fix" the ObjC binding here to match the JavaScript binding?  Aren't we
> > cleaning up the ObjC bindings by generating them anyway?
> 
>   This accessor is in public interface, changing it isn't possible (an error is
> generated by the script if I try to).

To make a change like this to the Objective-C DOM interfaces you also have to edit PublicDOMInterfaces.h file.  We have the error so inadvertent changes aren't made, but when a change is needed, editing the file is ok.  I think in this case changing it would good.

[Darin Adler]

Comment on attachment 12588 [details]
proposed fix

+        select.remove((static_cast<HTMLOptionElement*>(element))->index());

There's an extra set of parentheses on this line that is not needed.

+    if (element && element->hasTagName(optionTag))
+        select.remove((static_cast<HTMLOptionElement*>(element))->index());
+    else
+        select.remove(static_cast<int>(args[0]->toNumber(exec)));

I think it would be a bit more elegant to do the HTMLElement part of this by overloading HTMLSelectElement::remove to have a version that takes an HTMLElement*. The HTMLSelectElement::add function already takes an HTMLElement, so I think it would be consistent.

Then we could also fix the bug where you can call remove on select A with an HTMLOptionElement from select B and it will remove one of the options from select A!

I think that eventually we should fix -[HTMLSelectElement length], even though it's good to preserve the strange version of it for this patch.

Fine to land as-is, r=me.

[David Kilzer (:ddkilzer)]

(In reply to comment #13)
> (In reply to comment #11)
> > +    for (unsigned i = 0; i < items.size(); ++i) {
> > 
> > If you're rewriting this function anyway, then please consider putting the
> > items.size() call outside the loop. The compiler might optimize it anyway, but
> > it might not.
> 
>   Unless there is a specific reason to do so, I'd like to keep it as is. This
> change would trade a little readability for a little performance - not
> something I normally do without profiling or at least knowing that the code in
> question is hot.

    for (unsigned i = 0, max = items.size(); i < max; ++i) {

[Alexey Proskuryakov]

Committed revision 19024.

(In reply to comment #17)
>     for (unsigned i = 0, max = items.size(); i < max; ++i) {

This does look somewhat unusual and confusing, although maybe it's just me.