Bug 121586 – Fix the HTMLSelectElement.prototype.remove() method

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

Consider merging https://chromium.googlesource.com/chromium/blink/+/f851277598b06bc6b06543782396d9546e47751f

Fix the HTMLSelectElement.prototype.remove() method so that it behaves like
Element.remove() if no argument is passed (from ChildNode). This behavior
is consistent with Firefox.

Also get rid of custom code for HTMLSelectElement.remove() and
HTMLOptionsCollection.remove() by leveraging operation overloading in Web
IDL.

[Chris Dumez]

I am out of office and in the process on moving. I am happy to port this over to WebKit as soon as I get some coding time though.

If you don't wish to wait for me, it is also fine of course.

[Chris Dumez]

Created attachment 213291 [details]
Patch

[Darin Adler]

Comment on attachment 213291 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213291&action=review

Good thing to fix, but not done quite the right way.

> Source/WebCore/bindings/js/JSHTMLOptionsCollectionCustom.cpp:93
> -    HTMLOptionsCollection* imp = impl();
> -    JSHTMLSelectElement* base = jsCast<JSHTMLSelectElement*>(asObject(toJS(exec, globalObject(), imp->ownerNode())));
> -    return base->remove(exec);
> +    HTMLOptionsCollection& imp = *impl();
> +
> +    // The argument can be an HTMLOptionElement or an index.
> +    if (HTMLOptionElement* option = toHTMLOptionElement(exec->argument(0))) {
> +        HTMLSelectElement& select = *static_cast<HTMLSelectElement*>(imp.ownerNode());
> +        select.remove(option);
> +    } else
> +        imp.remove(exec->argument(0).toInt32(exec));
> +    return jsUndefined();

Overloading should be reflected in the real class, not just the binding. The job of the binding is to get the types converted from JavaScript and get over to the DOM. So this should be something like this:

    JSValue argument0 = exec->argument(0);
    if (HTMLOptionElement* option = toHTMLOptionElement(argument0))
        impl()->remove(option);
    else
        impl()->remove(argument.toInt32(exec));

The rest of the logic should be in the functions in HTMLOptionsCollection itself. No accident that JSHTMLSelectElement does it this way.

> Source/WebCore/bindings/js/JSHTMLSelectElementCustom.cpp:44
> +        static_cast<Element&>(select).remove(ec);

This is the wrong way to do it. We should not cast to Element&. Instead we should use “using” to make the inherited remove visible in the HTMLSelectElement class like this:

    using HTMLElement::remove;
    // other remove overloads go here

[Chris Dumez]

Comment on attachment 213291 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213291&action=review

>> Source/WebCore/bindings/js/JSHTMLOptionsCollectionCustom.cpp:93
>> +    return jsUndefined();
> 
> Overloading should be reflected in the real class, not just the binding. The job of the binding is to get the types converted from JavaScript and get over to the DOM. So this should be something like this:
> 
>     JSValue argument0 = exec->argument(0);
>     if (HTMLOptionElement* option = toHTMLOptionElement(argument0))
>         impl()->remove(option);
>     else
>         impl()->remove(argument.toInt32(exec));
> 
> The rest of the logic should be in the functions in HTMLOptionsCollection itself. No accident that JSHTMLSelectElement does it this way.

Makes sense. I will add a remove(HTMLOptionsElement*) overload to HTMLOptionsCollection class then.

>> Source/WebCore/bindings/js/JSHTMLSelectElementCustom.cpp:44
>> +        static_cast<Element&>(select).remove(ec);
> 
> This is the wrong way to do it. We should not cast to Element&. Instead we should use “using” to make the inherited remove visible in the HTMLSelectElement class like this:
> 
>     using HTMLElement::remove;
>     // other remove overloads go here

Yes, this is what I did on Blink side actually. The issue on WebKit side is that Node::remove() takes a ExceptionCode& in argument and ExceptionCode is a typedef to int. Therefore, using Node::remove in HTMLSelectElement.h does not compile as calls to remove(int) become ambiguous (as HTMLSelectElement::remove(int) already exists).

How do you suggest to proceed?

[Darin Adler]

(In reply to comment #4)
> The issue on WebKit side is that Node::remove() takes a ExceptionCode& in argument and ExceptionCode is a typedef to int. Therefore, using Node::remove in HTMLSelectElement.h does not compile as calls to remove(int) become ambiguous (as HTMLSelectElement::remove(int) already exists).
> 
> How do you suggest to proceed?

Short term I would suggest renaming HTMLSelectElement::remove(int) to eliminate the ambiguity. Maybe removeByIndex. With a comment explaining the ambiguity.

Longer term there may be some better solution. Not sure we want to change ExceptionCode to be something other than an integer as Blink have, but maybe something like that. It could just be a class wrapping an integer.

[Chris Dumez]

Created attachment 213297 [details]
Patch

[Darin Adler]

Comment on attachment 213297 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213297&action=review

Looks fine as long as we fix GTK.

> Source/WebCore/html/HTMLSelectElement.h:63
> +    void removeByIndex(int);

This change broke GTK’s binding: webkit_dom_html_select_element_remove. I guess we have to fix that.

[Chris Dumez]

Created attachment 213303 [details]
Attempt  to fix ews

[WebKit Commit Bot]

Comment on attachment 213303 [details]
Attempt  to fix ews

Clearing flags on attachment: 213303

Committed r156869: <http://trac.webkit.org/changeset/156869>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.