Bug 67742 – Push more code from HTMLInputElement::setValue to TextFieldInputType::setValue

• Component: Forms
• Status: RESOLVED FIXED

[Ryosuke Niwa]

We have too much code for TextFieldInputType in HTMLInputElement::setValue. We should those into TextFieldInputType::setValue.

[Ryosuke Niwa]

Created attachment 106663 [details]
Patch

[Ryosuke Niwa]

I feel like I'm doing too much at once in this patch still but I'll be happy nontheless if someone feels brave enough to review it.

[Darin Adler]

I’m planning on reviewing soon.

I’m more concerned about the confusing idiom of the bool& for sendChangeEvent than I am about the pure "too much in one patch" issue.

[Ryosuke Niwa]

(In reply to comment #3)
> I’m planning on reviewing soon.
> 
> I’m more concerned about the confusing idiom of the bool& for sendChangeEvent than I am about the pure "too much in one patch" issue.

Thanks! The problem is that dispatching change/input event needs to happen at the every end for text fields. Because InputType::setValue is called at the beginning of setValue methods in the derived classes, it prevents us from dispatching the event there.  On the other hand, we can't always dispatch change events because text fields needs to dispatch input events in some cases.

[Kent Tamura]

Comment on attachment 106663 [details]
Patch

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

> Source/WebCore/html/HTMLInputElement.cpp:1092
> -            dispatchFormControlInputEvent();
> -        else
> -            dispatchFormControlChangeEvent();
> -    }
>  
> +    // FIXME: why do we do this when !sendChangeEvent?
>      if (isTextField() && (!focused() || !sendChangeEvent))
>          setTextAsOfLastFormControlChangeEvent(value);

This code is dangerous. 'change' / 'input' event handlers can delete 'this' though it's not your failure.

> Source/WebCore/html/HTMLInputElement.h:239
> +    class OffsetForCacheSelection {

What's the purpose of this class?
Passing an int to cacheSelectionInResponseToSetValue() looks enough.

> Source/WebCore/html/TextFieldInputType.cpp:97
> +    if (sendChangeEvent) {
> +        // If the user is still editing this field, dispatch an input event rather than a change event.
> +        // The change event will be dispatched when editing finishes.
> +        if (element()->focused())
> +            element()->dispatchFormControlInputEvent();
> +        else
> +            element()->dispatchFormControlChangeEvent();

The original code dispatches a 'change' event for non text fields. Moving this code to here looks wrong.
Have you run layout tests?

> Source/WebCore/html/TextFieldInputType.cpp:98
> +        sendChangeEvent = false;

Why do we need to update sendChangeEvent?

[Ryosuke Niwa]

Comment on attachment 106663 [details]
Patch

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

> Source/WebCore/html/HTMLInputElement.cpp:-1120
> -    if (sendChangeEvent) {
> -        // If the user is still editing this field, dispatch an input event rather than a change event.
> -        // The change event will be dispatched when editing finishes.
> -        if (isTextField() && focused())
> -            dispatchFormControlInputEvent();
> -        else
> -            dispatchFormControlChangeEvent();
> -    }

Oops, this is wrong. It should be replaced by:
if (sendChangeEvent)
    dispatchFormControlChangeEvent();

>> Source/WebCore/html/HTMLInputElement.h:239
>> +    class OffsetForCacheSelection {
> 
> What's the purpose of this class?
> Passing an int to cacheSelectionInResponseToSetValue() looks enough.

To restrict the access to cacheSelectionInResponseToSetValue to HTMLInputElement and TextFieldInputType.

>> Source/WebCore/html/TextFieldInputType.cpp:98

> 
> Why do we need to update sendChangeEvent?

To prevent HTMLInputElement::setValue from firing the change event. Apparently, there's no test for this :(

[Ryosuke Niwa]

Created attachment 106697 [details]
Updated per comment

[Kent Tamura]

Comment on attachment 106697 [details]
Updated per comment

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

> Source/WebCore/html/HTMLInputElement.cpp:1102
> +    if (sendChangeEvent)
> +        dispatchFormControlChangeEvent();

I prefer
 - copying the dispatchFormControlChangeEvent() call to non-TextField InputTypes too, or
 - introduce another virtual function, InputType::dispatchChangeEvent(), and TextFieldInputType overrides it.

> Source/WebCore/html/HTMLInputElement.h:240
> +    void cacheSelectionInReponseToSetValue(int caretOffset) { cacheSelection(caretOffset, caretOffset, SelectionHasNoDirection); }
> +

Please move this into a section above connectToColorChooser().

[Kent Tamura]

(In reply to comment #8)
>  - introduce another virtual function, InputType::dispatchChangeEvent(), and TextFieldInputType overrides it.

This might be a role of valueChanged().

[Ryosuke Niwa]

(In reply to comment #8)
>  - introduce another virtual function, InputType::dispatchChangeEvent(), and TextFieldInputType overrides it.

That's much cleaner!

[Ryosuke Niwa]

Created attachment 106699 [details]
Updated

[Darin Adler]

(In reply to comment #6)
> > Why do we need to update sendChangeEvent?
> 
> To prevent HTMLInputElement::setValue from firing the change event. Apparently, there's no test for this :(

If you haven’t done it already, I think it’s clear we should make a test for that and land it before landing this change.

[Ryosuke Niwa]

(In reply to comment #12)
> (In reply to comment #6)
> > > Why do we need to update sendChangeEvent?
> > 
> > To prevent HTMLInputElement::setValue from firing the change event. Apparently, there's no test for this :(
> 
> If you haven’t done it already, I think it’s clear we should make a test for that and land it before landing this change.

I've tried but I haven't been able to make one. This code path is used for non-text field but checkbox and ratiobox fire change event elsewhere when clicked.  I can't think of any input type that uses this code path.

[Ryosuke Niwa]

FYI, it is possible that tkent's new color, etc... UI makes use of this code path.  But as far as I can tell this code can't be hit in Mac port at the moment.

[Ryosuke Niwa]

(In reply to comment #14)
> FYI, it is possible that tkent's new color, etc... UI makes use of this code path.  But as far as I can tell this code can't be hit in Mac port at the moment.

Let me rephrase, the function itself may be called but change event is never fired from that call.

[Darin Adler]

Comment on attachment 106699 [details]
Updated

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

> Source/WebCore/html/HTMLInputElement.cpp:1093
> +    m_suggestedValue = String(); // TextFieldInputType::setValue uses the suggested value.

This comment does not make clear why clearing the suggested value is the correct thing to do. Neither did the old comment.

> Source/WebCore/html/HTMLInputElement.cpp:1104
> +    // FIXME: why do we do this when !sendChangeEvent?

The word “why” should be capitalized.

> Source/WebCore/html/TextFieldInputType.cpp:98
> +    if (element()->focused())
> +        element()->dispatchFormControlInputEvent();
> +    else
> +        element()->dispatchFormControlChangeEvent();

I think this should use early return instead of else, and call through to the base class dispatchChangeEventInResponseToSetValue instead of calling dispatchFormControlChangeEvent after the early return.

[Ryosuke Niwa]

Comment on attachment 106699 [details]
Updated

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

Thanks for the review!

>> Source/WebCore/html/HTMLInputElement.cpp:1093
>> +    m_suggestedValue = String(); // TextFieldInputType::setValue uses the suggested value.
> 
> This comment does not make clear why clearing the suggested value is the correct thing to do. Neither did the old comment.

Revised.

>> Source/WebCore/html/HTMLInputElement.cpp:1104
>> +    // FIXME: why do we do this when !sendChangeEvent?
> 
> The word “why” should be capitalized.

Fixed.

>> Source/WebCore/html/TextFieldInputType.cpp:98
>> +        element()->dispatchFormControlChangeEvent();
> 
> I think this should use early return instead of else, and call through to the base class dispatchChangeEventInResponseToSetValue instead of calling dispatchFormControlChangeEvent after the early return.

Fixed.

[Ryosuke Niwa]

Committed r94836: <http://trac.webkit.org/changeset/94836>