Add InputElement abstraction

[Nikolas Zimmermann]

Add an abstract InputElement interface (just like ScriptElement) in dom/ allowing to share a lot of code between HTMLInputElement and the upcoming WMLInputElement.

In a previous step the rendering/RenderTextControl* classes have already been split up and prepared to support non-HTMLInputElement derived objects using these render classes.

[Nikolas Zimmermann]

Created attachment 26863 [details]
Initial patch

[Nikolas Zimmermann]

Forgot to mention: no regression, checked carefully since some weeks.

[Adam Roben (:aroben)]

Comment on attachment 26863 [details]
Initial patch

> +        A lot of code from HTMLInputElement now lives in InputElement, as static member functions - the
> +        InputElement class itself is an abstract virtual class, just like ScriptElement. HTML/WMLInputElement
> +        derive from InputElement, and hold a InputElementData member variable, that they pass to the
> +        static functions in InputElement. The abstraction is equal to the one chosen for HTML/SVGScriptElement.

Why do HTML/WMLInputElement hold the InputElementData, rather than InputElement itself? Is it just from a desire to keep InputElement an abstract base class?

> +        * WebCore.xcodeproj/project.pbxproj:
> +        * dom/FormControlElement.cpp: Added.
> +        (WebCore::formControlElementForElement):
> +        * dom/FormControlElement.h:
> +        * dom/InputElement.cpp: Added.
> +        (WebCore::InputElement::dispatchFocusEvent):
> +        (WebCore::InputElement::dispatchBlurEvent):
> +        (WebCore::InputElement::updatePlaceholderVisibility):
> +        (WebCore::InputElement::updateFocusAppearance):
> +        (WebCore::InputElement::updateSelectionRange):
> +        (WebCore::InputElement::aboutToUnload):
> +        (WebCore::InputElement::setValueFromRenderer):
> +        (WebCore::numCharactersInGraphemeClusters):
> +        (WebCore::InputElement::constrainValue):
> +        (WebCore::numGraphemeClusters):
> +        (WebCore::InputElement::handleBeforeTextInsertedEvent):
> +        (WebCore::InputElement::parseSizeAttribute):
> +        (WebCore::InputElement::parseMaxLengthAttribute):
> +        (WebCore::InputElement::updateValueIfNeeded):
> +        (WebCore::InputElement::notifyFormStateChanged):
> +        (WebCore::InputElementData::InputElementData):
> +        (WebCore::InputElementData::~InputElementData):
> +        (WebCore::InputElementData::name):
> +        (WebCore::inputElementForElement):
> +        * dom/InputElement.h: Added.
> +        (WebCore::InputElement::~InputElement):
> +        (WebCore::InputElement::InputElement):
> +        (WebCore::InputElementData::inputElement):
> +        (WebCore::InputElementData::element):
> +        (WebCore::InputElementData::placeholderShouldBeVisible):
> +        (WebCore::InputElementData::setPlaceholderShouldBeVisible):
> +        (WebCore::InputElementData::setName):
> +        (WebCore::InputElementData::value):
> +        (WebCore::InputElementData::setValue):
> +        (WebCore::InputElementData::size):
> +        (WebCore::InputElementData::setSize):
> +        (WebCore::InputElementData::maxLength):
> +        (WebCore::InputElementData::setMaxLength):
> +        (WebCore::InputElementData::cachedSelectionStart):
> +        (WebCore::InputElementData::setCachedSelectionStart):
> +        (WebCore::InputElementData::cachedSelectionEnd):
> +        (WebCore::InputElementData::setCachedSelectionEnd):
> +        * html/HTMLInputElement.cpp:
> +        (WebCore::HTMLInputElement::HTMLInputElement):
> +        (WebCore::HTMLInputElement::name):
> +        (WebCore::HTMLInputElement::updateFocusAppearance):
> +        (WebCore::HTMLInputElement::aboutToUnload):
> +        (WebCore::HTMLInputElement::dispatchFocusEvent):
> +        (WebCore::HTMLInputElement::dispatchBlurEvent):
> +        (WebCore::HTMLInputElement::setInputType):
> +        (WebCore::HTMLInputElement::selectionStart):
> +        (WebCore::HTMLInputElement::selectionEnd):
> +        (WebCore::HTMLInputElement::setSelectionRange):
> +        (WebCore::HTMLInputElement::parseMappedAttribute):
> +        (WebCore::HTMLInputElement::appendFormData):
> +        (WebCore::HTMLInputElement::size):
> +        (WebCore::HTMLInputElement::copyNonAttributeProperties):
> +        (WebCore::HTMLInputElement::value):
> +        (WebCore::HTMLInputElement::setValue):
> +        (WebCore::HTMLInputElement::placeholderValue):
> +        (WebCore::HTMLInputElement::searchEventsShouldBeDispatched):
> +        (WebCore::HTMLInputElement::setValueFromRenderer):
> +        (WebCore::HTMLInputElement::setFileListFromRenderer):
> +        (WebCore::HTMLInputElement::defaultEventHandler):
> +        (WebCore::HTMLInputElement::setDefaultName):
> +        (WebCore::HTMLInputElement::maxLength):
> +        (WebCore::HTMLInputElement::constrainValue):
> +        (WebCore::HTMLInputElement::cacheSelection):
> +        (WebCore::HTMLInputElement::selection):
> +        (WebCore::HTMLInputElement::placeholderShouldBeVisible):
> +        * html/HTMLInputElement.h:
> +        (WebCore::HTMLInputElement::isTextField):
> +        (WebCore::HTMLInputElement::isSearchField):
> +        (WebCore::HTMLInputElement::isAutofilled):
> +        * html/HTMLIsIndexElement.cpp:
> +        (WebCore::HTMLIsIndexElement::HTMLIsIndexElement):
> +        * rendering/RenderTextControl.cpp:
> +        (WebCore::RenderTextControl::formControlElement):
> +        * rendering/RenderTextControlSingleLine.cpp:
> +        (WebCore::RenderTextControlSingleLine::placeholderShouldBeVisible):
> +        (WebCore::RenderTextControlSingleLine::addSearchResult):
> +        (WebCore::RenderTextControlSingleLine::stopSearchEventTimer):
> +        (WebCore::RenderTextControlSingleLine::showPopup):
> +        (WebCore::RenderTextControlSingleLine::hidePopup):
> +        (WebCore::RenderTextControlSingleLine::subtreeHasChanged):
> +        (WebCore::RenderTextControlSingleLine::capsLockStateMayHaveChanged):
> +        (WebCore::RenderTextControlSingleLine::preferredContentWidth):
> +        (WebCore::RenderTextControlSingleLine::createSubtreeIfNeeded):
> +        (WebCore::RenderTextControlSingleLine::updateFromElement):
> +        (WebCore::RenderTextControlSingleLine::cacheSelection):
> +        (WebCore::RenderTextControlSingleLine::createInnerBlockStyle):
> +        (WebCore::RenderTextControlSingleLine::createResultsButtonStyle):
> +        (WebCore::RenderTextControlSingleLine::createCancelButtonStyle):
> +        (WebCore::RenderTextControlSingleLine::updateCancelButtonVisibility):
> +        (WebCore::RenderTextControlSingleLine::startSearchEventTimer):
> +        (WebCore::RenderTextControlSingleLine::searchEventTimerFired):
> +        (WebCore::RenderTextControlSingleLine::valueChanged):
> +        (WebCore::RenderTextControlSingleLine::setTextFromItem):
> +        (WebCore::RenderTextControlSingleLine::inputElement):
> +        * rendering/RenderTextControlSingleLine.h:

It would be useful to have function-level comments explaining how the code was moved around.

> +void InputElement::dispatchFocusEvent(InputElementData& data, Document* document)
> +{
> +    if (!data.inputElement()->isTextField())
> +        return;
> +
> +    InputElement::updatePlaceholderVisibility(data, document);

I don't think "InputElement::" is needed here or in the other member functions in this file.

> +void InputElement::dispatchBlurEvent(InputElementData& data, Document* document)
> +{
> +    if (!data.inputElement()->isTextField())
> +        return;
> +
> +    if (Frame* frame = document->frame()) {

You could make this an early return to remove some nesting.

> +void InputElement::updatePlaceholderVisibility(InputElementData& data, Document* document, bool placeholderValueChanged)
> +{
> +    ASSERT(data.inputElement()->isTextField());
> +
> +    bool oldPlaceholderShouldBeVisible = data.placeholderShouldBeVisible();
> +    Element* element = data.element();
> +
> +    data.setPlaceholderShouldBeVisible(data.inputElement()->value().isEmpty() 
> +                                       && document->focusedNode() != element
> +                                       && !data.inputElement()->placeholderValue().isEmpty());
> +
> +    if ((oldPlaceholderShouldBeVisible != data.placeholderShouldBeVisible() || placeholderValueChanged) && element->renderer())
> +        static_cast<RenderTextControlSingleLine*>(element->renderer())->updatePlaceholderVisibility();

What guarantees that element->renderer() will be a RenderTextControlSingleLine?

> +void InputElement::updateSelectionRange(InputElementData& data, int start, int end)
> +{
> +    if (!data.inputElement()->isTextField())
> +        return;
> +
> +    if (RenderTextControl* renderer = static_cast<RenderTextControl*>(data.element()->renderer()))

What guarantees that data.element()->renderer() is a RenderTextControl?

> +void InputElement::setValueFromRenderer(InputElementData& data, Document* document, const String& value)
> +{
> +    // Renderer and our event handler are responsible for constraining values.
> +    ASSERT(value == data.inputElement()->constrainValue(value) || data.inputElement()->constrainValue(value).isEmpty());
> +
> +    if (data.inputElement()->isTextField())
> +        InputElement::updatePlaceholderVisibility(data, document);
> +
> +    // Workaround for bug where trailing \n is included in the result of textContent.
> +    // The assert macro above may also be simplified to:  value == constrainValue(value)
> +    // http://bugs.webkit.org/show_bug.cgi?id=9661
> +    if (value == "\n")
> +        data.setValue("");
> +    else
> +        data.setValue(value);
> +
> +    Element* element = data.element();
> +    formControlElementForElement(element)->setValueMatchesRenderer();

Should we assert that formControlElementForElement returns a non-null value?

> +String InputElement::constrainValue(const InputElementData& data, const String& proposedValue, int maxLength)
> +{
> +    String string = proposedValue;
> +    if (!data.inputElement()->isTextField())
> +        return string;
> +        
> +    string.replace("\r\n", " ");
> +    string.replace('\r', ' ');
> +    string.replace('\n', ' ');
> +    
> +    StringImpl* s = string.impl();
> +    int newLength = numCharactersInGraphemeClusters(s, maxLength);
> +    for (int i = 0; i < newLength; ++i) {
> +        const UChar& current = (*s)[i];
> +        if (current < ' ' && current != '\t') {
> +            newLength = i;
> +            break;
> +        }
> +    }
> +
> +    if (newLength < static_cast<int>(string.length()))
> +        return string.substring(0, newLength);

I believe String::left could be used here, but I know this isn't new code.

> +void InputElement::parseMaxLengthAttribute(InputElementData& data, MappedAttribute* attribute)
> +{
> +    int maxLength = attribute->isNull() ? InputElement::s_maximumLength : attribute->value().toInt();
> +    if (maxLength <= 0 || maxLength > InputElement::s_maximumLength)
> +        maxLength = InputElement::s_maximumLength;

Often we do this kind of clamping using max() and min(): maxLength = max(0, min(maxLength, InputElement::s_maximumLength))

> @@ -348,9 +353,6 @@ void RenderTextControlSingleLine::styleD
>  
>  void RenderTextControlSingleLine::capsLockStateMayHaveChanged()
>  {
> -    if (!node() || !document())
> -        return;
> -

Why remove these null-checks?

r=me, but please consider these comments.

[Nikolas Zimmermann]

> Why do HTML/WMLInputElement hold the InputElementData, rather than InputElement
> itself? Is it just from a desire to keep InputElement an abstract base class?
Yes exactly. We had this debate before when introducing ScriptElement(Data). Maciej strongly suggested to keep an ABC an ABC :-)
 
> It would be useful to have function-level comments explaining how the code was
> moved around.
Okay, I can try commenting better.

> 
> > +void InputElement::dispatchFocusEvent(InputElementData& data, Document* document)
> > +{
> > +    if (!data.inputElement()->isTextField())
> > +        return;
> > +
> > +    InputElement::updatePlaceholderVisibility(data, document);
> 
> I don't think "InputElement::" is needed here or in the other member functions
> in this file.
True - I use this style to explicitely show I'm calling another static function. But I can easily change it.
 
> > +void InputElement::dispatchBlurEvent(InputElementData& data, Document* document)
> > +{
> > +    if (!data.inputElement()->isTextField())
> > +        return;
> > +
> > +    if (Frame* frame = document->frame()) {
> 
> You could make this an early return to remove some nesting.
Will fix.

> 
> > +void InputElement::updatePlaceholderVisibility(InputElementData& data, Document* document, bool placeholderValueChanged)
> > +{
> > +    ASSERT(data.inputElement()->isTextField());
> > +
> > +    bool oldPlaceholderShouldBeVisible = data.placeholderShouldBeVisible();
> > +    Element* element = data.element();
> > +
> > +    data.setPlaceholderShouldBeVisible(data.inputElement()->value().isEmpty() 
> > +                                       && document->focusedNode() != element
> > +                                       && !data.inputElement()->placeholderValue().isEmpty());
> > +
> > +    if ((oldPlaceholderShouldBeVisible != data.placeholderShouldBeVisible() || placeholderValueChanged) && element->renderer())
> > +        static_cast<RenderTextControlSingleLine*>(element->renderer())->updatePlaceholderVisibility();
> 
> What guarantees that element->renderer() will be a RenderTextControlSingleLine?
The ASSERT above relating to isTextField(). See HTMLInputElement::createRenderer, and HTMLInputElement::isTextField().

> 
> > +void InputElement::updateSelectionRange(InputElementData& data, int start, int end)
> > +{
> > +    if (!data.inputElement()->isTextField())
> > +        return;
> > +
> > +    if (RenderTextControl* renderer = static_cast<RenderTextControl*>(data.element()->renderer()))
> 
> What guarantees that data.element()->renderer() is a RenderTextControl?
Same reason as above.

> 
> > +void InputElement::setValueFromRenderer(InputElementData& data, Document* document, const String& value)
> > +{
> > +    // Renderer and our event handler are responsible for constraining values.
> > +    ASSERT(value == data.inputElement()->constrainValue(value) || data.inputElement()->constrainValue(value).isEmpty());
> > +
> > +    if (data.inputElement()->isTextField())
> > +        InputElement::updatePlaceholderVisibility(data, document);
> > +
> > +    // Workaround for bug where trailing \n is included in the result of textContent.
> > +    // The assert macro above may also be simplified to:  value == constrainValue(value)
> > +    // http://bugs.webkit.org/show_bug.cgi?id=9661
> > +    if (value == "\n")
> > +        data.setValue("");
> > +    else
> > +        data.setValue(value);
> > +
> > +    Element* element = data.element();
> > +    formControlElementForElement(element)->setValueMatchesRenderer();
> 
> Should we assert that formControlElementForElement returns a non-null value?
That's an implicit assertion here, if an 'Element' class derives from InputElement, it derived from 'FormControlElement' as well. Do you prefer an assertion?
 
> > +String InputElement::constrainValue(const InputElementData& data, const String& proposedValue, int maxLength)
> > +{
> > +    String string = proposedValue;
> > +    if (!data.inputElement()->isTextField())
> > +        return string;
> > +        
> > +    string.replace("\r\n", " ");
> > +    string.replace('\r', ' ');
> > +    string.replace('\n', ' ');
> > +    
> > +    StringImpl* s = string.impl();
> > +    int newLength = numCharactersInGraphemeClusters(s, maxLength);
> > +    for (int i = 0; i < newLength; ++i) {
> > +        const UChar& current = (*s)[i];
> > +        if (current < ' ' && current != '\t') {
> > +            newLength = i;
> > +            break;
> > +        }
> > +    }
> > +
> > +    if (newLength < static_cast<int>(string.length()))
> > +        return string.substring(0, newLength);
> 
> I believe String::left could be used here, but I know this isn't new code.
Okay, will fix anyway.

 
> > +void InputElement::parseMaxLengthAttribute(InputElementData& data, MappedAttribute* attribute)
> > +{
> > +    int maxLength = attribute->isNull() ? InputElement::s_maximumLength : attribute->value().toInt();
> > +    if (maxLength <= 0 || maxLength > InputElement::s_maximumLength)
> > +        maxLength = InputElement::s_maximumLength;
> 
> Often we do this kind of clamping using max() and min(): maxLength = max(0,
> min(maxLength, InputElement::s_maximumLength))
Right, will fix.
 
> > @@ -348,9 +353,6 @@ void RenderTextControlSingleLine::styleD
> >  
> >  void RenderTextControlSingleLine::capsLockStateMayHaveChanged()
> >  {
> > -    if (!node() || !document())
> > -        return;
> > -
> 
> Why remove these null-checks?
Oops, leftover from testing, to see if any code hits those conditions. It doesn't, but i'll readd them to be on the safe side.

Thanks for the quick review, going to post another patch soon.

[Nikolas Zimmermann]

Created attachment 26866 [details]
Updated patch

Fixed Adams issues.

[Nikolas Zimmermann]

Oops, patch contains a typo, missing semicolon in one line maxLength = max(0, ....);. Please ignore.

[Adam Roben (:aroben)]

(In reply to comment #4)
> > > +void InputElement::updatePlaceholderVisibility(InputElementData& data, Document* document, bool placeholderValueChanged)
> > > +{
> > > +    ASSERT(data.inputElement()->isTextField());
> > > +
> > > +    bool oldPlaceholderShouldBeVisible = data.placeholderShouldBeVisible();
> > > +    Element* element = data.element();
> > > +
> > > +    data.setPlaceholderShouldBeVisible(data.inputElement()->value().isEmpty() 
> > > +                                       && document->focusedNode() != element
> > > +                                       && !data.inputElement()->placeholderValue().isEmpty());
> > > +
> > > +    if ((oldPlaceholderShouldBeVisible != data.placeholderShouldBeVisible() || placeholderValueChanged) && element->renderer())
> > > +        static_cast<RenderTextControlSingleLine*>(element->renderer())->updatePlaceholderVisibility();
> > 
> > What guarantees that element->renderer() will be a RenderTextControlSingleLine?
> The ASSERT above relating to isTextField(). See
> HTMLInputElement::createRenderer, and HTMLInputElement::isTextField().


What if we have markup such as:

<input style="-webkit-appearance: button">

Will we still get a RenderTextControlSingleLine?

[Adam Roben (:aroben)]

(In reply to comment #4)
> > > +void InputElement::setValueFromRenderer(InputElementData& data, Document* document, const String& value)
> > > +{
> > > +    // Renderer and our event handler are responsible for constraining values.
> > > +    ASSERT(value == data.inputElement()->constrainValue(value) || data.inputElement()->constrainValue(value).isEmpty());
> > > +
> > > +    if (data.inputElement()->isTextField())
> > > +        InputElement::updatePlaceholderVisibility(data, document);
> > > +
> > > +    // Workaround for bug where trailing \n is included in the result of textContent.
> > > +    // The assert macro above may also be simplified to:  value == constrainValue(value)
> > > +    // http://bugs.webkit.org/show_bug.cgi?id=9661
> > > +    if (value == "\n")
> > > +        data.setValue("");
> > > +    else
> > > +        data.setValue(value);
> > > +
> > > +    Element* element = data.element();
> > > +    formControlElementForElement(element)->setValueMatchesRenderer();
> > 
> > Should we assert that formControlElementForElement returns a non-null value?
> That's an implicit assertion here, if an 'Element' class derives from
> InputElement, it derived from 'FormControlElement' as well. Do you prefer an
> assertion?

Nothing in the code above says to me "element's derived type derives from InputElement". All I see is "element is an Element, and we'd like the corresponding FormControlElement for it". So I do think an assertion would be helpful.

[Adam Roben (:aroben)]

Comment on attachment 26866 [details]
Updated patch

> +void InputElement::notifyFormStateChanged(InputElementData& data, Document* document)
> +{
> +    Frame* frame = document->frame();
> +    if (!frame)
> +        return;
> +
> +    frame->page()->chrome()->client()->formStateDidChange(data.element());

I still think we should be null-checking frame->page() here.

> +class InputElementData {
> +public:
> +    InputElementData(InputElement*, Element*);

When will these two parameters not have the same value? Maybe a comment here in the header explaining their purpose would help.

r=me

[Nikolas Zimmermann]

(In reply to comment #9)
> (From update of attachment 26866 [details] [review])
> > +void InputElement::notifyFormStateChanged(InputElementData& data, Document* document)
> > +{
> > +    Frame* frame = document->frame();
> > +    if (!frame)
> > +        return;
> > +
> > +    frame->page()->chrome()->client()->formStateDidChange(data.element());
> 
> I still think we should be null-checking frame->page() here.
Fixed.

> 
> > +class InputElementData {
> > +public:
> > +    InputElementData(InputElement*, Element*);
> 
> When will these two parameters not have the same value? Maybe a comment here in
> the header explaining their purpose would help.
They always have the same value. But as InputElement doesn't inherit from Element, it's not possible to just pass one value here (just like it's done in ScriptElementData).

Thanks for r+.

[Nikolas Zimmermann]

(In reply to comment #7)
> > > 
> > > What guarantees that element->renderer() will be a RenderTextControlSingleLine?
> > The ASSERT above relating to isTextField(). See
> > HTMLInputElement::createRenderer, and HTMLInputElement::isTextField().
> 
> 
> What if we have markup such as:
> 
> <input style="-webkit-appearance: button">
> 
> Will we still get a RenderTextControlSingleLine?
The renderer creation depends only on the 'type' attribute, so this won't affect it.

Landed patch in r40065.