Bug 23808 – Add SelectElement abstraction

• Component: DOM
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

Allow RenderMenuList/RenderListBox to be used from non-HTML clients, like WML.

A new abstract base class, SelectElement, should be created, just like it has already been done for InputElement/Option(Group)Element. It should serve as base class for WML/HTMLSelectElement, and is supposed to share code between those classes (as static helper methods), and also as abstract interface to RenderMenuList/RenderListBox, equally designed as Input/Option(Group)Element.

[Nikolas Zimmermann]

Created attachment 27433 [details]
Initial patch

Woho, celebrate the last needed abstract base class to complete WMLs' needs!

[Nikolas Zimmermann]

As the memory regression (FormControlElement...) is fixed in trunk since two days, it makes sense to work on new abstractions again :-)

As the summary says, OptionElement/OptionGroupElement has already been created, and the last missing piece to implement WMLSelectElement is a SelectElement base-class, sharing code with HTMLSelectElement.

I'm going to attach a patch soon which just adds SelectElement, and moves away the rendering code from using HTMLSelectElement directly. This patch does NOT add static helper functions in SelectElement to share code between HTMLSelectElement and the upcoming WMLSelectElement, this will be done in a follow-up patch. This has been the process for InputElement and all the other base classes that have been introduced, and it's working nicely this way.

Fortunately that's really the last needed piece for WML, no further abstraction overhead for HTML other than this class. But I think it's okay, since the ambigouties with FormControlElement/FormControlElementWithState & HTMLFormControlElement have been resolved.

CC'ing Dave as he knows the memory regression as well as the rendering code related to HTMLSelectElement - it would be great if you could check the patch once it's up here.

[Nikolas Zimmermann]

Created attachment 30150 [details]
Updated patch

[Maciej Stachowiak]

Won't the multiple inheritance in this patch cause a memory regression (however slight) for HTML? Is there any way to do this without introducing multiple inheritance?

[Nikolas Zimmermann]

(In reply to comment #4)
> Won't the multiple inheritance in this patch cause a memory regression (however
> slight) for HTML? Is there any way to do this without introducing multiple
> inheritance?
Yes the same drawback then adding StyleElement/ScriptElement/InputElement etc. had.

As discussed with Hyatt before, I do not plan any further addition of classes, adding SelectElement would fulfil all of WMLs needs. A follow-up patch will also add much more code to SelectElement, used to share between HTML/WMLSelectElement.

I'd really argue to ignore this slight mem regression. My follow-up patches are waiting for a long time now, and I'd finally love to get this in.

Remember, that all other MI issues (FormControlElememnt(WithState) addition) are gone now, so the main concerns Hyatt had are gone now. I really hope we can stay with this design for now.

[Nikolas Zimmermann]

Created attachment 30619 [details]
Updated patch v2

Build against ToT. (Accessibility* files have been moved from page/ to accessibility/).
All tests pass. No regressions.

[Darin Adler]

Comment on attachment 30619 [details]
Updated patch v2

Didn't we discover a way to do this without introducing multiple inheritance? I thought I saw patches where we were undoing some virtualization we did before based on feedback from Dave Hyatt.

> -        AccessibilityObject* listOption = listBoxOptionAccessibilityObject(listItems[i]);
> +        AccessibilityObject* listOption = listBoxOptionAccessibilityObject(static_cast<HTMLElement*>(listItems[i]));

This cast needs a comment explaining why it is safe. Casts like these are dangerous and we've had many bugs in the past where we did an illegal downcast. I believe the answer is that this is guaranteed to be an HTML element unless you are compiling with WML enabled, and that the code in this file is not compatible with WML.

> -            return listBoxOptionAccessibilityObject(listItems[i]);
> +            return listBoxOptionAccessibilityObject(static_cast<HTMLElement*>(listItems[i]));

Same thought here.

>      if (m_renderer->isMenuList())
> -        return static_cast<RenderMenuList*>(m_renderer)->selectElement();
> -    
> +        return static_cast<Element*>(m_renderer->node());

Same comment again. Why is this cast to Element safe? We need to at least have a comment saying why, and ideally come up with idioms that don't require unchecked type casts, unless the check is just before the cast. You'll note that in the old code, the cast to RenderMenuList was checked by the code in the if statement.

> +    virtual void dispatchFormControlChangeEvent() { }

Do we really need this in the Element class? Our goal is to go in the opposite direction, and cut down the size of our massive base classes.

This is really just a helper function to call dispatchEvent with the changeEvent event name. I don't think it's really form-control-specific. Perhaps it would be cleaner move this into a separate helper function and have it not be a member function at all. Having this be a virtual function isn't helpful unless we actually plan to call this in cases where we do not want a change event dispatched. We could also have this just be in the EventTarget class.

> +        if (element->hasTagName(HTMLNames::selectTag))
> +            return static_cast<HTMLSelectElement*>(element);
> +        else if (element->hasTagName(HTMLNames::keygenTag))
> +            return static_cast<HTMLKeygenElement*>(element);

We normally do not do else after return.

> +    virtual bool multiple() const = 0;

Not a great name for a function -- you probably didn't create it. Typically getters should have a noun or adjective that applies to the object. But "a select element's multiple" makes no sense. 

> +protected:
> +    SelectElement() { }

This explicit declaration and definition of the constructor is unneeded. If you don't declare or define it, we'll get the same thing automatically. So the only effect this has is to make the constructor protected. Since the class has pure virtual functions, objects of this class already can't be constructed, so it's best to just leave this out.

>          if (current->hasTagName(optionTag)) {
> -            m_listItems.append(static_cast<HTMLElement*>(current));
> +            m_listItems.append(static_cast<Element*>(current));

I think this should be a cast to HTMLOptionElement*, not to Element*, since that matches the test that was done. I know that m_listItems can hold any Element*, but that need not be the deciding factor.

> -        // Save the selection so it can be compared to the new selection when we call onChange during dispatchBlurEvent.
> +        // Save the selection so it can be compared to the new selection when we call dispatchFormControlChangeEvent during dispatchBlurEvent.

I think this comment is probably referring to the change event, not the onChange function. If so, we should call it "send a change event" or "call onchange", and not use the function name.

> -    // We only need to fire onChange here for menu lists, because we fire onChange for list boxes whenever the selection change is actually made.
> +    // We only need to fire dispatchFormControlChangeEvent here for menu lists, because we fire onChange for list boxes whenever the selection change is actually made.

Same thought here, but even more so. When you say "fire onchange" I think you're referring to the event, not the function.

In general, I don't think the renames inside the comments are all improvements, especially since the new function name is so long.

> +void WMLFormControlElement::dispatchFormControlChangeEvent()
> +{
> +    // no-op
> +}

Why have this since it's the same as the base class?

I'm going to say r=me. This seems a little bit messy, but no significant problems.

[Nikolas Zimmermann]

(In reply to comment #7)
> (From update of attachment 30619 [details] [review])
> Didn't we discover a way to do this without introducing multiple inheritance? I
> thought I saw patches where we were undoing some virtualization we did before
> based on feedback from Dave Hyatt.
The main concern were my abstract FormControlElement / FormControlElementWithState classe. Example for HTMLInputElement:

HTMLInputElement -> HTMLFormControlElementWithState, InputElement
HTMLFormControlElementWithState -> HTMLFormControlElement and FormControlElementWithState
HTMLFormControlElement -> HTMLElement and FormControlElement

These classes introduced an odd class structure. Since they are gone in trunk, it's just:

HTMLInputElement -> HTMLFormControlElementWithState, InputElement
HTMLFormControlElementWithState -> HTMLFormControlElement
HTMLFormControlElement -> HTMLElement

I've chosen the same approach for HTMLSelectElement. And we already had a few classes using this scheme (HTMLInput/Option/OptGroup/ScriptElement).

> 
> > -        AccessibilityObject* listOption = listBoxOptionAccessibilityObject(listItems[i]);
> > +        AccessibilityObject* listOption = listBoxOptionAccessibilityObject(static_cast<HTMLElement*>(listItems[i]));
> 
> This cast needs a comment explaining why it is safe. Casts like these are
> dangerous and we've had many bugs in the past where we did an illegal downcast.
> I believe the answer is that this is guaranteed to be an HTML element unless
> you are compiling with WML enabled, and that the code in this file is not
> compatible with WML.
Exactly. Fixed.

> 
> > -            return listBoxOptionAccessibilityObject(listItems[i]);
> > +            return listBoxOptionAccessibilityObject(static_cast<HTMLElement*>(listItems[i]));
> 
> Same thought here.
Ditto.

> 
> >      if (m_renderer->isMenuList())
> > -        return static_cast<RenderMenuList*>(m_renderer)->selectElement();
> > -    
> > +        return static_cast<Element*>(m_renderer->node());
> 
> Same comment again. Why is this cast to Element safe? We need to at least have
> a comment saying why, and ideally come up with idioms that don't require
> unchecked type casts, unless the check is just before the cast. You'll note
> that in the old code, the cast to RenderMenuList was checked by the code in the
> if statement.
Oh, all the other cases above this line are using the same cast. Does that need a specific comment?

<quote>
    if (isFileUploadButton())
        return static_cast<Element*>(m_renderer->node());

    if (AccessibilityObject::isARIAInput(ariaRoleAttribute()))
        return static_cast<Element*>(m_renderer->node());

    if (isImageButton())
        return static_cast<Element*>(m_renderer->node());

    if (m_renderer->isMenuList())
        return static_cast<Element*>(m_renderer->node());
</quote>

> 
> > +    virtual void dispatchFormControlChangeEvent() { }
> 
> Do we really need this in the Element class? Our goal is to go in the opposite
> direction, and cut down the size of our massive base classes.
> 
> This is really just a helper function to call dispatchEvent with the
> changeEvent event name. I don't think it's really form-control-specific.
> Perhaps it would be cleaner move this into a separate helper function and have
> it not be a member function at all. Having this be a virtual function isn't
> helpful unless we actually plan to call this in cases where we do not want a
> change event dispatched. We could also have this just be in the EventTarget
> class.
That's exactly the reason why I added it, WML doesn't want these events to be fired.
On the other hand, nothing in WML could intercept these events, so we could just fire it and save that function. Though all other places in WML are patched that no events are dispatched. I think I'll leave it in for now. We may change it in a follow-up patch.

> 
> > +        if (element->hasTagName(HTMLNames::selectTag))
> > +            return static_cast<HTMLSelectElement*>(element);
> > +        else if (element->hasTagName(HTMLNames::keygenTag))
> > +            return static_cast<HTMLKeygenElement*>(element);
> 
> We normally do not do else after return.
Fixed.

> 
> > +    virtual bool multiple() const = 0;
> 
> Not a great name for a function -- you probably didn't create it. Typically
> getters should have a noun or adjective that applies to the object. But "a
> select element's multiple" makes no sense. 
True, the name corresponds to the HTML "multiple" attribute. I guess it's named this way for historical reasons.
I can rename the function in a follow-up patch, but it rather not clutter this commit with it, as I'm just moving this method.

How about "canSelectMultipleItems()" / "allowsMultiSelection"? Doesn't sound really nice, maybe you have a better suggestion.

> 
> > +protected:
> > +    SelectElement() { }
> 
> This explicit declaration and definition of the constructor is unneeded. If you
> don't declare or define it, we'll get the same thing automatically. So the only
> effect this has is to make the constructor protected. Since the class has pure
> virtual functions, objects of this class already can't be constructed, so it's
> best to just leave this out.
Right, fixed.

> 
> >          if (current->hasTagName(optionTag)) {
> > -            m_listItems.append(static_cast<HTMLElement*>(current));
> > +            m_listItems.append(static_cast<Element*>(current));
> 
> I think this should be a cast to HTMLOptionElement*, not to Element*, since
> that matches the test that was done. I know that m_listItems can hold any
> Element*, but that need not be the deciding factor.
I've changed it back to HTMLElement as it was, there are other places in this file using the same scheme.

> 
> > -        // Save the selection so it can be compared to the new selection when we call onChange during dispatchBlurEvent.
> > +        // Save the selection so it can be compared to the new selection when we call dispatchFormControlChangeEvent during dispatchBlurEvent.
> 
> I think this comment is probably referring to the change event, not the
> onChange function. If so, we should call it "send a change event" or "call
> onchange", and not use the function name.
Okay, rephrased all comments.
 
> > -    // We only need to fire onChange here for menu lists, because we fire onChange for list boxes whenever the selection change is actually made.
> > +    // We only need to fire dispatchFormControlChangeEvent here for menu lists, because we fire onChange for list boxes whenever the selection change is actually made.
> 
> Same thought here, but even more so. When you say "fire onchange" I think
> you're referring to the event, not the function.
Yes, fixed.
 
> In general, I don't think the renames inside the comments are all improvements,
> especially since the new function name is so long.
> 
> > +void WMLFormControlElement::dispatchFormControlChangeEvent()
> > +{
> > +    // no-op
> > +}
> 
> Why have this since it's the same as the base class?
Oh fixed.
 
> I'm going to say r=me. This seems a little bit messy, but no significant
> problems.

Thanks a lot, going to land soon.

[Darin Adler]

> Oh, all the other cases above this line are using the same cast. Does that need
> a specific comment?
> 
> <quote>
>     if (isFileUploadButton())
>         return static_cast<Element*>(m_renderer->node());
> 
>     if (AccessibilityObject::isARIAInput(ariaRoleAttribute()))
>         return static_cast<Element*>(m_renderer->node());
> 
>     if (isImageButton())
>         return static_cast<Element*>(m_renderer->node());
> 
>     if (m_renderer->isMenuList())
>         return static_cast<Element*>(m_renderer->node());
> </quote>

I guess not. Seems a bit weak.

> How about "canSelectMultipleItems()" / "allowsMultiSelection"? Doesn't sound
> really nice, maybe you have a better suggestion.

I realize now that multiple() is the name in the DOM, so maybe we should just leave the name alone even after this patch.

[Nikolas Zimmermann]

(In reply to comment #9)
> 
> > How about "canSelectMultipleItems()" / "allowsMultiSelection"? Doesn't sound
> > really nice, maybe you have a better suggestion.
> 
> I realize now that multiple() is the name in the DOM, so maybe we should just
> leave the name alone even after this patch.

Okay, going to land now that all issues are resolved.

[Nikolas Zimmermann]

Landed in r44100. Contained an unrelated patch, fixed in commit r44101.

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.