Bug 48317 – Refactor HTMLInputElement: Move a part of HTMLInputElement::defaultEventHandler() to InputTypes

• Component: Forms
• Status: RESOLVED FIXED

[Kent Tamura]

Refactor HTMLInputElement: Move a part of HTMLInputElement::defaultEventhandler() to InputTypes

[Kent Tamura]

Created attachment 71860 [details]
Patch

[Darin Adler]

Comment on attachment 71860 [details]
Patch

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

Great work on the refactoring.

I’m going to say review- because I find the boolean results of these various functions a bit too confusing. We should choose a single meaning for the boolean result from the event function and stick to that. That probably means reversing the logic for DOMActivateEvent.

I’m not sure that the “return immediately if form is 0” behavior from DOMActivate is even important. Maybe the handleDOMActivateEvent function doesn’t need a return value. These confusing bool return values are poison!

> WebCore/ChangeLog:16
> +        Move m_xPos and m_yPos from HTMLInputElement to ImageInputType. It's
> +        safe to move them because they have valid values just after click event
> +        and form submission by the click.

Could you create a test case where we change the type in response to the click event before the form submission happens?

> WebCore/html/BaseDateAndTimeInputType.cpp:116
> +bool BaseDateAndTimeInputType::handleKeydownEvent(KeyboardEvent* event)
> +{
> +    if (handleKeydownEventForSpinButton(event))
> +        return true;
> +    return TextFieldInputType::handleKeydownEventForSpinButton(event);
> +}

This is a very surprising function. It’s not clear at all why this calls handleKeydownEventForSpinButton first, and then calls TextFieldInputType::handleKeydownEventForSpinButton! Maybe the second call was supposed to be a call to handleKeydownEvent?

> WebCore/html/FileInputType.cpp:83
> +    if (element()->renderer())
> +        toRenderFileUploadControl(element()->renderer())->click();
> +    return false;

I would use && here instead of an if statement.

> WebCore/html/HTMLInputElement.cpp:1238
> +    if (evt->isKeyboardEvent() && evt->type() == eventNames().keydownEvent
> +        && m_inputType->handleKeydownEvent(static_cast<KeyboardEvent*>(evt)))
>          return;

I hate how the formatting looks in cases like this. Maybe we could put this all on one long line?

> WebCore/html/HTMLInputElement.cpp:1256
> +    if (evt->type() == eventNames().DOMActivateEvent && !disabled()
> +        && m_inputType->handleDOMActivateEvent(evt))
> +        return;

Same thought here.

I’m not sure why the !disabled() check is being done here in the base class. It’s an unclear separation of responsibilities. Nothing in the name handleDOMActivateEvent makes it clear that disabled is already checked. It seems a surprising difference from the handleKeydownEvent function.

> WebCore/html/ImageInputType.cpp:33
> +ALWAYS_INLINE ImageInputType::ImageInputType(HTMLInputElement* element)

Isn’t a plain old inline sufficient? Why was ALWAYS_INLINE needed?

> WebCore/html/ImageInputType.cpp:72
> +bool ImageInputType::handleClickEvent(MouseEvent* mouseEvent)

You’re moving enough code here that you may have to update copyright.

> WebCore/html/ImageInputType.h:53
> +    int m_xPosition;
> +    int m_yPosition;

These names aren’t so great. How about:

    IntPoint m_clickLocation;

Or something like that?

> WebCore/html/InputType.cpp:276
> +bool InputType::handleKeydownEventForSpinButton(KeyboardEvent* event)

Why is this spin-button-specific event handling in the InputType base class? There should at least be a comment explaining this.

> WebCore/html/InputType.cpp:278
> +    String key = static_cast<KeyboardEvent*>(event)->keyIdentifier();

It would be better if keyIdentifier returned a const String& and this local variable was also const String&. There’s no need to churn the reference count just to check string equality.

> WebCore/html/InputType.cpp:285
> +    int step = 0;
> +    if (key == "Up")
> +        step = 1;
> +    else if (key == "Down")
> +        step = -1;
> +    if (!step)
> +        return false;

I think "else return false" would be clearer.

> WebCore/html/InputType.h:105
> +    virtual bool handleClickEvent(MouseEvent*);
> +    virtual bool handleDOMActivateEvent(Event*);
> +    virtual bool handleKeydownEvent(KeyboardEvent*);

The meanings of these bool return values are unclear to me. What does true and false mean?

I also think that we should provide a basic handleEvent function here, and put the dispatching to the individual handleEvent functions inside that, but maybe you can do that later. Eventually I’d hope these individual functions could become protected or even private.

> WebCore/html/RangeInputType.cpp:131
> +bool RangeInputType::handleKeydownEvent(KeyboardEvent* event)

You’re moving enough code here that you may have to update copyright.

> WebCore/html/RangeInputType.cpp:141
> +        // FIXME: Is 1/100 reasonable?

This is a mysterious comment. Rewrite for clarity?

> WebCore/html/ResetInputType.cpp:60
> +bool ResetInputType::handleDOMActivateEvent(Event*)
> +{
> +    if (!element()->form())
> +        return true;
> +    element()->form()->reset();
> +    return false;
> +}

This seems backwards. What does the boolean return value from this function means? It seems to be returning true when the function does nothing and false when the function does something. Maybe we need a different return type if the boolean result is unclear.

> WebCore/html/TextFieldInputType.cpp:59
> +    Document* document = element()->document();
> +    if (!document->frame())
> +        return false;
> +    if (!document->frame()->editor()->doTextFieldCommandFromEvent(element(), event))

The only way document is ever used is to call document->frame() so clearly the local variable should be the Frame* instead.

> WebCore/html/TextFieldInputType.cpp:62
> +    event->setDefaultHandled();
> +    return true;

Why do some of these functions call setDefaultedHandled, but not others. I think there is something wrong with the idiom here. I know you are only refactoring, but this refactoring seems to be exposing inconsistencies that may be bugs, so we should at least add FIXME.

[Kent Tamura]

Comment on attachment 71860 [details]
Patch

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

>> WebCore/ChangeLog:16
>> +        and form submission by the click.
> 
> Could you create a test case where we change the type in response to the click event before the form submission happens?

I added a test in Bug#49302, and found this patch was wrong :-)
HTMLFormElement::prepareSubmit() dispatches 'submit' event and JavaScript code may change the input type.  So we should not access data members of  InputType after prepareSubmit(). I fixed it in the second patch.

>> WebCore/html/BaseDateAndTimeInputType.cpp:116
>> +}
> 
> This is a very surprising function. It’s not clear at all why this calls handleKeydownEventForSpinButton first, and then calls TextFieldInputType::handleKeydownEventForSpinButton! Maybe the second call was supposed to be a call to handleKeydownEvent?

Oh, you're right.
But this mistake didn't make behavior change.

>> WebCore/html/FileInputType.cpp:83
>> +    return false;
> 
> I would use && here instead of an if statement.

done.

>> WebCore/html/HTMLInputElement.cpp:1238
>>          return;
> 
> I hate how the formatting looks in cases like this. Maybe we could put this all on one long line?

done.

>> WebCore/html/HTMLInputElement.cpp:1256
>> +        return;
> 
> Same thought here.
> 
> I’m not sure why the !disabled() check is being done here in the base class. It’s an unclear separation of responsibilities. Nothing in the name handleDOMActivateEvent makes it clear that disabled is already checked. It seems a surprising difference from the handleKeydownEvent function.

done.
I moved the disabled() check to handleDOMActivateEvent() implementations though I wonder if the disabled() check is required.

>> WebCore/html/ImageInputType.cpp:33
>> +ALWAYS_INLINE ImageInputType::ImageInputType(HTMLInputElement* element)
> 
> Isn’t a plain old inline sufficient? Why was ALWAYS_INLINE needed?

I'm not sure.
Anyway, I reverted this change because m_xPosition and m_yPosition is merged to m_clickLocation and we don't need to initialize it explicitly.

>> WebCore/html/ImageInputType.cpp:72
>> +bool ImageInputType::handleClickEvent(MouseEvent* mouseEvent)
> 
> You’re moving enough code here that you may have to update copyright.

I checked revision logs, and confirmed the current copyright statement is enough.  This file already have non-Google copyright.

> WebCore/html/ImageInputType.cpp:77
> +    // FIXME: We could just call offsetX() and offsetY() on the event,
> +    // but that's currently broken, so for now do the computation here.

This comment was added in r12153, and we have many improvements on offsetX() and offsetY() after r12153. I think we should use offsetX() and offsetY().  The second patch uses them.

> WebCore/html/ImageInputType.cpp:105
> +    if (!element()->form()->prepareSubmit(event)) {
> +        m_xPosition = 0;
> +        m_yPosition = 0;
> +    }
> +    element()->setActivatedSubmit(false);

We shouldn't call element().  "this" might be deleted by 'submit' event.

>> WebCore/html/ImageInputType.h:53
>> +    int m_yPosition;
> 
> These names aren’t so great. How about:
> 
>     IntPoint m_clickLocation;
> 
> Or something like that?

Done.

>> WebCore/html/InputType.cpp:276
>> +bool InputType::handleKeydownEventForSpinButton(KeyboardEvent* event)
> 
> Why is this spin-button-specific event handling in the InputType base class? There should at least be a comment explaining this.

I moved it to TextFieldInputType.

>> WebCore/html/InputType.cpp:278
>> +    String key = static_cast<KeyboardEvent*>(event)->keyIdentifier();
> 
> It would be better if keyIdentifier returned a const String& and this local variable was also const String&. There’s no need to churn the reference count just to check string equality.

I'll handle it in Bug#49426.

>> WebCore/html/InputType.cpp:285
>> +        return false;
> 
> I think "else return false" would be clearer.

Done.

>> WebCore/html/InputType.h:105
>> +    virtual bool handleKeydownEvent(KeyboardEvent*);
> 
> The meanings of these bool return values are unclear to me. What does true and false mean?
> 
> I also think that we should provide a basic handleEvent function here, and put the dispatching to the individual handleEvent functions inside that, but maybe you can do that later. Eventually I’d hope these individual functions could become protected or even private.

I added a comment on the bools in the second patch.

    //   The return value means "should return from the caller event handler immediately."

Does it make sense?  It is almost identical to Event::setDefaultHandled().  However the current HTMLInputElement::defaultEventHandler() does early-return without setDefaultHandled() in some cases. So we need the flag.
Should we introduce new enum for this?

Yes, the implementation of HTMLInputElement::defaultEventHandler() should be just "m_inputType->handleEvent(evt)" in the future.

>> WebCore/html/RangeInputType.cpp:131
>> +bool RangeInputType::handleKeydownEvent(KeyboardEvent* event)
> 
> You’re moving enough code here that you may have to update copyright.

I checked revision logs, and confirmed the current copyright statement is enough.

>> WebCore/html/RangeInputType.cpp:141
>> +        // FIXME: Is 1/100 reasonable?
> 
> This is a mysterious comment. Rewrite for clarity?

Done.

>> WebCore/html/ResetInputType.cpp:60
>> +}
> 
> This seems backwards. What does the boolean return value from this function means? It seems to be returning true when the function does nothing and false when the function does something. Maybe we need a different return type if the boolean result is unclear.

The behavior of this code is same as the original code.  But I suspect the original code has a bug.  This should be the following?
  element()->form()->reset();
  event->setDefaultHandled();
  return true;

> WebCore/html/SubmitInputType.cpp:69
> +    element()->form()->prepareSubmit(event);
> +    element()->setActivatedSubmit(false);

We shouldn't call element().  "this" might be deleted by 'submit' event.

>> WebCore/html/TextFieldInputType.cpp:59
>> +    if (!document->frame()->editor()->doTextFieldCommandFromEvent(element(), event))
> 
> The only way document is ever used is to call document->frame() so clearly the local variable should be the Frame* instead.

Done.

[Kent Tamura]

Created attachment 73703 [details]
Patch 2

[Kent Tamura]

Created attachment 73875 [details]
Patch 3 (rebased for keyIdentifier())

[Darin Adler]

(In reply to comment #3)
> > I’m not sure why the !disabled() check is being done here in the base class. It’s an unclear separation of responsibilities. Nothing in the name handleDOMActivateEvent makes it clear that disabled is already checked. It seems a surprising difference from the handleKeydownEvent function.
> 
> done.
> I moved the disabled() check to handleDOMActivateEvent() implementations though I wonder if the disabled() check is required.

I believe it is required, but we can create a test to explore that if yo like.

> > This seems backwards. What does the boolean return value from this function means? It seems to be returning true when the function does nothing and false when the function does something. Maybe we need a different return type if the boolean result is unclear.
> 
> The behavior of this code is same as the original code.  But I suspect the original code has a bug.  This should be the following?
>   element()->form()->reset();
>   event->setDefaultHandled();
>   return true;

I honestly don’t know. We have to figure out how to create a test that can detect the difference.

[Darin Adler]

Comment on attachment 73875 [details]
Patch 3 (rebased for keyIdentifier())

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

r=me, but there are still some tricky bits that need a little more thought

> WebCore/html/FileInputType.cpp:84
> +bool FileInputType::handleDOMActivateEvent(Event*)
> +{
> +    if (!element()->disabled() && element()->renderer())
> +        toRenderFileUploadControl(element()->renderer())->click();
> +    return false;
> +}

Is it correct to return false when the DOMActivate event is handled? We should construct a test for this.

> WebCore/html/ImageInputType.cpp:71
> +    RefPtr<HTMLInputElement> input(element());

I prefer the assignment style to constructor style for this sort of thing, and I also suggest we put this at the top of the function rather than after the checks of disabled and form. Or, to make it more clear exactly where the issue lies, we could put it just before the call to prepareSubmit.

Also, another possible name for the local variable is "element", which might be clearer.

    RefPtr<HTMLInputElement> element = this->element();

Or we could come up with a name that makes it clear it’s about surviving even after arbitrary code runs.

Since the only reason we need this is for the call to setActivatedSubmit(false), we might instead make an object that calls setActivatedSubmit(true) and then setActivatedSubmit(false) when it falls out of scope, and that object can use a RefPtr. The RAII idiom.

> WebCore/html/ImageInputType.cpp:78
> +    input->form()->prepareSubmit(event); // JavaScript code might run.

It might be be clearer to say that “event handlers” can run rather than JavaScript. The same issue exists regardless of programming language.

> WebCore/html/ImageInputType.cpp:80
> +    return false;

I think returning false here is truly strange as I mentioned earlier, and I suggest we create test cases to see what is going on.

In fact, because we return false, I suspect the caller will use a possibly-bad "this" pointer if the event handler ran, so I think this is more than just a minor behavioral issue. It’s possible the caller has to be prepared for event handlers to be run even if false is returned. We should take a look at the various functions to see if that ever happens.

> WebCore/html/InputType.h:104
> +    // Event handlers
> +    //   The return value means "should return from the caller event handler immediately."

Strange formatting here. I know I asked for a comment, but I still don’t find this comment entirely clear. I think what the return value means is more like "if true, do no further default event handling in the default event handler and return true from that function". The reason I was unclear before is that some of the functions seem to do the opposite.

> WebCore/html/RangeInputType.cpp:158
> +            setValueAsNumber(newValue, ec);
> +            element()->dispatchFormControlChangeEvent();

Can we really get at element() after calling setValueAsNumber? Can’t event handlers run?

> WebCore/html/RangeInputType.cpp:165
> +        element()->stepUp(stepMagnification, ec);
> +        if (lastStringValue != element()->value())
> +            element()->dispatchFormControlChangeEvent();

Can we really get at element() after calling stepUp? Can’t event handlers run?

> WebCore/html/ResetInputType.cpp:61
> +    return false;

Same crazy "return false" here as in the other DOMActivate event handlers. This should really get explored and fixed.

> WebCore/html/SubmitInputType.cpp:73
> +    return false;

Again, same issue.

[Kent Tamura]

Comment on attachment 73875 [details]
Patch 3 (rebased for keyIdentifier())

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

Thank you for reviewing.  I'll commit the patch with some small changes.

>> WebCore/html/FileInputType.cpp:84
>> +}
> 
> Is it correct to return false when the DOMActivate event is handled? We should construct a test for this.

We should call setDefaultHandled() and return true.
However, AFAIK, calling setDefaultHandled() won't make any behavior change with the current WebKit code.  So, I'll add setDefaultHandled() here, but won't add tests.

>> WebCore/html/ImageInputType.cpp:71
>> +    RefPtr<HTMLInputElement> input(element());
> 
> I prefer the assignment style to constructor style for this sort of thing, and I also suggest we put this at the top of the function rather than after the checks of disabled and form. Or, to make it more clear exactly where the issue lies, we could put it just before the call to prepareSubmit.
> 
> Also, another possible name for the local variable is "element", which might be clearer.
> 
>     RefPtr<HTMLInputElement> element = this->element();
> 
> Or we could come up with a name that makes it clear it’s about surviving even after arbitrary code runs.
> 
> Since the only reason we need this is for the call to setActivatedSubmit(false), we might instead make an object that calls setActivatedSubmit(true) and then setActivatedSubmit(false) when it falls out of scope, and that object can use a RefPtr. The RAII idiom.

I changed so that "element = this->element()" is at the top of the function.
RAII is cool, but I feel it's overkill because we can use RefPtr.

>> WebCore/html/ImageInputType.cpp:78
>> +    input->form()->prepareSubmit(event); // JavaScript code might run.
> 
> It might be be clearer to say that “event handlers” can run rather than JavaScript. The same issue exists regardless of programming language.

Fixed.

>> WebCore/html/ImageInputType.cpp:80
>> +    return false;
> 
> I think returning false here is truly strange as I mentioned earlier, and I suggest we create test cases to see what is going on.
> 
> In fact, because we return false, I suspect the caller will use a possibly-bad "this" pointer if the event handler ran, so I think this is more than just a minor behavioral issue. It’s possible the caller has to be prepared for event handlers to be run even if false is returned. We should take a look at the various functions to see if that ever happens.

Fixed same as FileInputType.cpp.

>> WebCore/html/InputType.h:104
>> +    //   The return value means "should return from the caller event handler immediately."
> 
> Strange formatting here. I know I asked for a comment, but I still don’t find this comment entirely clear. I think what the return value means is more like "if true, do no further default event handling in the default event handler and return true from that function". The reason I was unclear before is that some of the functions seem to do the opposite.

Fixed the comment.

>> WebCore/html/RangeInputType.cpp:158
>> +            element()->dispatchFormControlChangeEvent();
> 
> Can we really get at element() after calling setValueAsNumber? Can’t event handlers run?

setValueAsNumber() doesn't dispatch events.

>> WebCore/html/RangeInputType.cpp:165
>> +            element()->dispatchFormControlChangeEvent();
> 
> Can we really get at element() after calling stepUp? Can’t event handlers run?

stepUp() doesn't dispatch events.

>> WebCore/html/ResetInputType.cpp:61
>> +    return false;
> 
> Same crazy "return false" here as in the other DOMActivate event handlers. This should really get explored and fixed.

Fixed same as FileInputType.cpp.

>> WebCore/html/SubmitInputType.cpp:73
>> +    return false;
> 
> Again, same issue.

Fixed same as FileInputType.cpp.

[Kent Tamura]

Landed: http://trac.webkit.org/changeset/72059