Bug 53160 – HTMLInputElement::setValue() should schedule change event when the element is focused

• Component: New Bugs
• Status: RESOLVED FIXED

[Ilya Sherman]

Fire onchange() event for text field after value is programmatically changed and focus is lost

[Ilya Sherman]

Created attachment 80167 [details]
Patch

[Ilya Sherman]

Created attachment 80168 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 80168 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Fire onchange() event for focused text field after its value is programmatically changed and focus is lost
> +        https://bugs.webkit.org/show_bug.cgi?id=53160

Please update the bug title here.

> Source/WebCore/html/HTMLInputElement.cpp:897
> +      else if (isTextField())
> +          toRenderTextControl(renderer())->setChangedSinceLastChangeEvent(true);

I don't think it's correct to do this just for text fields.  We probably want to fire change event for other types of input elements as well.

[Ilya Sherman]

This code really needs a test to go along with it, but I'm not sure how to go about adding one.  The difficulty is that there aren't any regular clients who call into setValue(..., true) (nor into setValueForUser()).  In fact, afaict, essentially the only clients are Safari and Chromium's autofill implementations -- and I'm only guessing that Safari still uses this, based on the comment in http://trac.webkit.org/browser/trunk/WebCore/dom/InputElement.h?rev=51602 .

Suggestions on how to add a test are very welcome!

[Ilya Sherman]

Created attachment 80171 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 80168 [details]
Patch

It's very unfortunate that there's no way to test this code per http://trac.webkit.org/changeset/51602.

[Ilya Sherman]

Created attachment 80173 [details]
Patch

[Ilya Sherman]

(In reply to comment #3)
> Please update the bug title here.

Done.

> > Source/WebCore/html/HTMLInputElement.cpp:897
> > +      else if (isTextField())
> > +          toRenderTextControl(renderer())->setChangedSinceLastChangeEvent(true);
> 
> I don't think it's correct to do this just for text fields.  We probably want to fire change event for other types of input elements as well.

You're right -- I just wasn't sure what exactly to do for other element types.  Judging from the other code, I think it might make sense to fire the event immediately for non-text fields -- updated the patch to do that in this case.

[Dimitri Glazkov (Google)]

Comment on attachment 80173 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        No new tests. (OOPS!)

Please explain why no test is possible here.

> Source/WebCore/html/HTMLInputElement.cpp:897
> +          toRenderTextControl(renderer())->setChangedSinceLastChangeEvent(true);

Would it be possible for renderer() to be not RenderTextControl here?

[Ryosuke Niwa]

Comment on attachment 80173 [details]
Patch

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

>> Source/WebCore/html/HTMLInputElement.cpp:897
>> +          toRenderTextControl(renderer())->setChangedSinceLastChangeEvent(true);
> 
> Would it be possible for renderer() to be not RenderTextControl here?

Hopefully not but we do should make sure the renderer is there (i.e. not NULL).

http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLInputElement.cpp#L1037
http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLInputElement.cpp#L1033

[Ilya Sherman]

Created attachment 83610 [details]
Patch

[WebKit Review Bot]

Attachment 83610 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebKit/mac/WebView/WebViewPrivate.h:680:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/DumpRenderTree/LayoutTestController.h:114:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ilya Sherman]

Comment on attachment 83610 [details]
Patch

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

Apologies for the long silence here -- got stuck writing the test code, and cycled away from this for a while.  The latest patch includes a stab at writing a test.  I would appreciate feedback and suggestions on how to improve that -- I've never tried to mess with DRT before, so I'm sure I did a bunch of stuff wrong.

Aaalso, the check-webkit-style script has a couple of complaints, but I think they are both erroneous.  Is the style checker correct?  If not, is there something I should do to silence it?

> LayoutTests/fast/forms/onchange-setvalueforuser.html:1
> +<html>

Should this test live in fast/forms or fast/events?  Both seem to have precedent.

> LayoutTests/fast/forms/onchange-setvalueforuser.html:20
> +        tf.blur();

Do we want to also test that the event is not fired until the blur occurs?  If so, any suggestions on a nice, straightforward way to do that?

> LayoutTests/fast/forms/onchange-setvalueforuser.html:22
> +    </script>

I've seen other tests have js in a separate file rather than inline -- is that preferred?

> Source/WebKit/mac/WebView/WebView.mm:6271
> +    JSStringRef jsStringValue = JSValueToStringCopy(context, value, NULL);
> +    String stringValue(jsStringValue->characters(), jsStringValue->length());

Do either of these leak memory?  I don't have a good understanding of the memory ownership model here.

[Build Bot]

Attachment 83610 [details] did not build on win:
Build output: http://queues.webkit.org/results/7985388

[Collabora GTK+ EWS bot]

Attachment 83610 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7987630

[Kent Tamura]

Comment on attachment 83610 [details]
Patch

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

>> LayoutTests/fast/forms/onchange-setvalueforuser.html:1
>> +<html>
> 
> Should this test live in fast/forms or fast/events?  Both seem to have precedent.

Either is ok.  I prefer fast/forms because it's a test of a form-specific feature.

> LayoutTests/fast/forms/onchange-setvalueforuser.html:7
> +    function log(msg) {
> +        var res = document.getElementById('res');
> +        res.innerHTML = res.innerHTML + msg + "<br>";
> +    }

We had better use debug() or testPassed() defined in LayoutTests/fast/js/resources/js-test-pre.js.

>> LayoutTests/fast/forms/onchange-setvalueforuser.html:20
>> +        tf.blur();
> 
> Do we want to also test that the event is not fired until the blur occurs?  If so, any suggestions on a nice, straightforward way to do that?

Introducing a global counter for the number of fired 'change' events?

>> LayoutTests/fast/forms/onchange-setvalueforuser.html:22
>> +    </script>
> 
> I've seen other tests have js in a separate file rather than inline -- is that preferred?

Some developers don't like separating js file.
IMO, either is ok.

> Source/WebCore/html/HTMLInputElement.cpp:899
> +    if (sendChangeEvent) {
> +      // For text fields, don't dispatch the change event when focused.
> +      // It will be dispatched when the control loses focus.
> +      RenderObject* r = renderer();
> +      if (r && r->isTextField() && document()->focusedNode() == this)
> +          toRenderTextControl(r)->setChangedSinceLastChangeEvent(true);
> +      else
>          dispatchFormControlChangeEvent();

Wrong indentation.

>> Source/WebKit/mac/WebView/WebView.mm:6271
>> +    String stringValue(jsStringValue->characters(), jsStringValue->length());
> 
> Do either of these leak memory?  I don't have a good understanding of the memory ownership model here.

I'm not sure too.

[Ilya Sherman]

Created attachment 84131 [details]
Patch

[Ilya Sherman]

Comment on attachment 84131 [details]
Patch

Updated patch to address Kent's comments and add stub implementations so that non-Mac platforms compile.

[WebKit Review Bot]

Attachment 84131 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebKit/mac/WebView/WebViewPrivate.h:680:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/DumpRenderTree/LayoutTestController.h:114:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 84131 [details]
Patch

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

> Source/WebCore/html/HTMLInputElement.cpp:895
> +        RenderObject* r = renderer();

I know you were doing this the same as the code in HTMLInputDefaultEventHandler, but please don’t use a single letter for a local variable. One way to avoid that is to write it like this:

    RenderObject* renderer = this->renderer();

> Source/WebCore/html/HTMLInputElement.cpp:897
> +        if (r && r->isTextField() && document()->focusedNode() == this)
> +            toRenderTextControl(r)->setChangedSinceLastChangeEvent(true);

It is kind of weak that when we have a focused node that for some reason has no renderer or a renderer of the wrong type we behave differently. Generally speaking it is not safe to access a renderer from a function like this that is callable directly from the DOM without first updating style and renderers with a call like updateLayout. It’s possible that the renderer is hanging around from before and the style has recently changed, so we could get the wrong behavior. To test that out you’d want a test case where the input starts out as a text field, then is focused, then changed to another type and then its value is changed. We might find that we get no change event even though we should get one because this code will see the old renderer.

Again, I know you are matching existing code, but it's best to make the check match the typecast. Since the check is isTextField, then the cast should be toRenderTextControlSingleLine. If the cast was to RenderTextControl, then the appropriate check would be isTextControl.

I think it would be better to find a way to share the setChangedSinceLastChangeEvent code, since the call sites are all doing the same check. But I guess this one call site is different in that the logic is entirely different when the renderer is not created or is not a text field.

[Ilya Sherman]

Created attachment 84138 [details]
Patch

[Ilya Sherman]

Comment on attachment 84138 [details]
Patch

Renamed "r" to "renderer", checking for isTextControl() rather than isTextField() because I think we want the same logic for text areas as well.

[Darin Adler]

(In reply to comment #22)
> checking for isTextControl() rather than isTextField() because I think we want the same logic for text areas as well.

Sensible, but text areas use HTMLTextAreaElement, not HTMLInputElement.

[WebKit Review Bot]

Attachment 84138 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebKit/mac/WebView/WebViewPrivate.h:680:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/DumpRenderTree/LayoutTestController.h:114:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ilya Sherman]

Comment on attachment 84131 [details]
Patch

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

>> Source/WebCore/html/HTMLInputElement.cpp:897
>> +            toRenderTextControl(r)->setChangedSinceLastChangeEvent(true);
> 
> It is kind of weak that when we have a focused node that for some reason has no renderer or a renderer of the wrong type we behave differently. Generally speaking it is not safe to access a renderer from a function like this that is callable directly from the DOM without first updating style and renderers with a call like updateLayout. It’s possible that the renderer is hanging around from before and the style has recently changed, so we could get the wrong behavior. To test that out you’d want a test case where the input starts out as a text field, then is focused, then changed to another type and then its value is changed. We might find that we get no change event even though we should get one because this code will see the old renderer.
> 
> Again, I know you are matching existing code, but it's best to make the check match the typecast. Since the check is isTextField, then the cast should be toRenderTextControlSingleLine. If the cast was to RenderTextControl, then the appropriate check would be isTextControl.
> 
> I think it would be better to find a way to share the setChangedSinceLastChangeEvent code, since the call sites are all doing the same check. But I guess this one call site is different in that the logic is entirely different when the renderer is not created or is not a text field.

Please pardon my ignorance, as I don't know this code very well:

I tried writing the test you suggested, and the change event was fired as expected.  That obviously doesn't cover all possible cases, though.  Is it safe/appropriate to add code along the lines of

if (isTextControl())
    updateLayout()

before the rest of the code here?

I think it makes sense to delay the onchange event for text areas as well as single-line fields, so I changed the check to be isTextControl().  I'm not sure that's right, though, since the other callers all check isTextField().  Perhaps someone who knows the history of this could chime in?

[Ilya Sherman]

(In reply to comment #23)
> (In reply to comment #22)
> > checking for isTextControl() rather than isTextField() because I think we want the same logic for text areas as well.
> 
> Sensible, but text areas use HTMLTextAreaElement, not HTMLInputElement.

Ah, makes sense -- I'll change the cast instead, then.

[Ilya Sherman]

Created attachment 84142 [details]
Patch

[WebKit Review Bot]

Attachment 84142 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebKit/mac/WebView/WebViewPrivate.h:680:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/DumpRenderTree/LayoutTestController.h:114:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

(In reply to comment #25)
> I tried writing the test you suggested, and the change event was fired as expected. That obviously doesn't cover all possible cases, though. Is it safe/appropriate to add code along the lines of
> 
> if (isTextControl())
>     updateLayout()
> 
> before the rest of the code here?

I am surprised the test didn’t fail. Maybe there was nothing to force the renderer to be updated before you changed the type so there was no leftover text field renderer. One way to force the renderer to be created is to call a function like offsetLeft from the JavaScript, since it calls updateLayoutIgnorePendingStylesheets. I think you should keep trying.

You can’t call isTextControl before calling a function updateLayoutIgnorePendingStylesheets, because the type of renderer could change based on the update.

Sorry, I don’t have an immediate complete answer for you. The general principle is that when making DOM calls the renderers may not yet reflect recent DOM or style changes, and so such calls shouldn’t make decisions based on the state of the renderer. This code is doing that.

I think that the decision about how to handle the change event should be based on the result of isTextType() rather than on the type of the renderer at the time of the value change. And we can construct some test cases where the change happens when the type is <input type="text"> but the element has no renderer at the time because of "display: none" and then later gets a renderer to make sure we get the right behavior for change events there.

[Early Warning System Bot]

Attachment 84142 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8073533

[Ilya Sherman]

(In reply to comment #29)
> (In reply to comment #25)
> > I tried writing the test you suggested, and the change event was fired as expected. That obviously doesn't cover all possible cases, though. Is it safe/appropriate to add code along the lines of
> > 
> > if (isTextControl())
> >     updateLayout()
> > 
> > before the rest of the code here?
> 
> I am surprised the test didn’t fail. Maybe there was nothing to force the renderer to be updated before you changed the type so there was no leftover text field renderer. One way to force the renderer to be created is to call a function like offsetLeft from the JavaScript, since it calls updateLayoutIgnorePendingStylesheets. I think you should keep trying.

I tried this as well, and everything still worked properly.  I suspect this is because setValue() calls setSuggestedValue(), which in turn calls renderer()->updateFromElement();

> You can’t call isTextControl before calling a function updateLayoutIgnorePendingStylesheets, because the type of renderer could change based on the update.
> 
> Sorry, I don’t have an immediate complete answer for you. The general principle is that when making DOM calls the renderers may not yet reflect recent DOM or style changes, and so such calls shouldn’t make decisions based on the state of the renderer. This code is doing that.

Would it be appropriate to track whether the value has changed since the last change event on the DOM side, rather than on the renderer side?  If not, I don't see how we can avoid making decisions based on the state of the renderer :(

> I think that the decision about how to handle the change event should be based on the result of isTextType() rather than on the type of the renderer at the time of the value change. And we can construct some test cases where the change happens when the type is <input type="text"> but the element has no renderer at the time because of "display: none" and then later gets a renderer to make sure we get the right behavior for change events there.

What is the right behavior?  Fire a change event without delay?  Fire a change event as soon as the element is displayed?

[Darin Adler]

(In reply to comment #31)
> Would it be appropriate to track whether the value has changed since the last change event on the DOM side, rather than on the renderer side?

Yes.

> What is the right behavior? Fire a change event without delay? Fire a change event as soon as the element is displayed?

I don’t know. I suggest testing behavior of other browsers since compatibility may be the main issue. Generally speaking, I am thinking that the principle is that the DOM code should not be able to detect whether there is a renderer or not.

[Ilya Sherman]

Created attachment 84402 [details]
Patch

[WebKit Review Bot]

Attachment 84402 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebKit/mac/WebView/WebViewPrivate.h:680:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 84402 [details]
Patch

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

Great approach. This looks good.

If it wasn’t for WML I would be absolutely sure we should put this bit into HTMLFormControlElement and not add any new virtual functions to Element. I am really unhappy we have to make such a mess just to handle WML. I still think we should put this into HTMLFormControlElement. If we have to have virtual functions that’s OK, but I would prefer not to spread this across so many classes.

> Source/WebCore/dom/Document.cpp:3172
> +        if (oldFocusedNode->isElementNode()) {

Since we have to do a type check here anyway, I suggest we instead call isFormControlElement and then cast to HTMLFormControlElement instead of just casting to Element. We then should put the flag bit into HTMLFormControlElement instead of having two separate flag bits and virtual functions to access them. Even though only text input and textareas would use the flags, I think we can spare the bit in HTMLFormControlElement.

> Source/WebCore/dom/Document.cpp:3173
> +            Element* element = static_cast<Element *>(oldFocusedNode.get());

Instead of Element * this should say Element*. But actually HTMLFormControlElement* if you take my advice above.

> Source/WebCore/dom/Element.h:328
> +    virtual void setChangedSinceLastChangeEvent(bool) { }
>      virtual void dispatchFormControlChangeEvent() { }

I think the function names should consistently use the phrase “form control change event” if we are adding them to Element.h. But we should not add new functions to Element.h (see my comment above).

> Source/WebCore/html/HTMLFormControlElement.cpp:656
> +void HTMLTextFormControlElement::dispatchFormControlChangeEvent()
> +{
> +    HTMLFormControlElement::dispatchFormControlChangeEvent();
> +    setChangedSinceLastChangeEvent(false);
> +}

Since we’ll put the bit directly into HTMLFormControlElement, this code can go there too.

> Source/WebCore/html/HTMLInputElement.cpp:899
> +        // For text fields, don't dispatch the change event when focused.
> +        // It will be dispatched when the control loses focus.
> +        if (isTextField() && document()->focusedNode() == this)
> +            setChangedSinceLastChangeEvent(true);
> +        else
> +            dispatchFormControlChangeEvent();

What about the other sites that call dispatchFormControlChangeEvent? Aren’t there any others that have the same issue, and should defer the change event in the case of a text input or textarea? A change specific to setValue may not cover enough cases.

We should also investigate whether we can call focused() here instead of document()->focusedNode() == this.

Do we need to clear this flag when the type of the input element changes?

> Source/WebCore/html/HTMLInputElement.cpp:-1044
>          // Fire onChange for text fields.
> -        RenderObject* r = renderer();
> -        if (r && r->isTextField() && toRenderTextControl(r)->wasChangedSinceLastChangeEvent()) {
> +        if (isTextField() && wasChangedSinceLastChangeEvent())
>              dispatchFormControlChangeEvent();
> -            // Refetch the renderer since arbitrary JS code run during onchange can do anything, including destroying it.
> -            r = renderer();
> -            if (r && r->isTextField())
> -                toRenderTextControl(r)->setChangedSinceLastChangeEvent(false);
> -        }

We don’t need the isTextField check here. It was needed before because we needed to type check the renderer before casting and calling functions on it. But here we would be OK unconditionally dispatching the change event if the wasChangedSinceLastChangeEvent flag was set. It won’t be set for other types of input elements that don’t need to send a change event.

Please remove the isTextField check and also consider removing or rewording the existing comment. For one thing, the event is a "change event", not "onChange". And a comment that just states what the code does is not valuable. It needs to explain *why* the code does something.

> Source/WebCore/html/HTMLInputElement.cpp:1413
>          if (renderer() && renderer()->isTextField())
> -            toRenderTextControl(renderer())->setChangedSinceLastChangeEvent(true);
> +            setChangedSinceLastChangeEvent(true);

The if statement here should be removed. It was a type check for the renderer. Instead we can unconditionally call setChangedSinceLastChangeEvent(true).

> Source/WebCore/html/HTMLInputElement.h:144
> +    virtual bool wasChangedSinceLastChangeEvent() const { return m_wasChangedSinceLastChangeEvent; }
> +    virtual void setChangedSinceLastChangeEvent(bool changed) { m_wasChangedSinceLastChangeEvent = changed; }

Virtual functions like these should not have function definitions in the header; there is no chance of inlining them.

I know that others in this file do, but one effect of this is to make our compile and link times slower with the many duplicate copies of such functions. Another effect is to make it so we have to recompile more if we ever decide to change the function bodies. The functions don’t actually get inlined when called as virtual functions, and these are never called in a non-virtual fashion.

Please move the function definitions into the .cpp file, except you probably will remove them entirely if you take my other suggestion above.

> Source/WebCore/html/HTMLTextAreaElement.h:55
> +    virtual bool wasChangedSinceLastChangeEvent() const { return m_wasChangedSinceLastChangeEvent; }
> +    virtual void setChangedSinceLastChangeEvent(bool changed) { m_wasChangedSinceLastChangeEvent = changed; }

Same comment as above. Please put these in the .cpp file.

> Source/WebCore/html/shadow/TextControlInnerElements.cpp:214
> +                input->setValue("", true);

Good change, but this points out why we prefer enums instead of booleans. That “true” is completely mysterious!

Is there a test that covers this? To put it another way: What test fails if you pass false here instead of true?

> Source/WebCore/rendering/RenderTextControlMultiLine.cpp:49
> +    textArea->setChangedSinceLastChangeEvent(true);

Which test fails if you don’t add this?

[Ilya Sherman]

Comment on attachment 84402 [details]
Patch

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

I should also note that I haven't actually verified that the WML changes are correct -- I'm not even sure how to compile the WML code :/

>> Source/WebCore/dom/Document.cpp:3172

> 
> Since we have to do a type check here anyway, I suggest we instead call isFormControlElement and then cast to HTMLFormControlElement instead of just casting to Element. We then should put the flag bit into HTMLFormControlElement instead of having two separate flag bits and virtual functions to access them. Even though only text input and textareas would use the flags, I think we can spare the bit in HTMLFormControlElement.

Probably HTMLTextFormControlElement is even better.  I originally didn't add the bit there because it looked like none of the abstract superclasses of HTMLInputElement had any data members -- I'd assumed that was an intentional aspect of the design, perhaps for the sake of better bit-packing the data structure in memory.

>> Source/WebCore/html/HTMLFormControlElement.cpp:656
>> +}
> 
> Since we’ll put the bit directly into HTMLFormControlElement, this code can go there too.

HTMLTextFormControlElement seems more appropriate to me -- is there something I'm overlooking?

>> Source/WebCore/html/HTMLInputElement.cpp:899

> 
> What about the other sites that call dispatchFormControlChangeEvent? Aren’t there any others that have the same issue, and should defer the change event in the case of a text input or textarea? A change specific to setValue may not cover enough cases.
> 
> We should also investigate whether we can call focused() here instead of document()->focusedNode() == this.
> 
> Do we need to clear this flag when the type of the input element changes?

Scanning the results of "git gs dispatchFormControlChangeEvent", there don't seem to be other relevant call sites not already covered in this patch -- I might've missed something though.

It seems ok to use focused() here -- switched to that.

You're probably right that we should clear the flag when the type changes: that's the behavior that we had previously, and that's what Gecko does.  Added that tweak.

>> Source/WebCore/html/HTMLInputElement.cpp:1413
>> +            setChangedSinceLastChangeEvent(true);
> 
> The if statement here should be removed. It was a type check for the renderer. Instead we can unconditionally call setChangedSinceLastChangeEvent(true).

We should probably only ever call setChangedSinceLastChangeEvent for text fields -- updated the code to decide based on the field's type.

>> Source/WebCore/html/shadow/TextControlInnerElements.cpp:214
>> +                input->setValue("", true);
> 
> Good change, but this points out why we prefer enums instead of booleans. That “true” is completely mysterious!
> 
> Is there a test that covers this? To put it another way: What test fails if you pass false here instead of true?

search-cancel-button-events.html at your service.  Actually, that test caught a bug in the previous implementation: if we delay the change event, we should send an input event -- fixed :)

>> Source/WebCore/rendering/RenderTextControlMultiLine.cpp:49
>> +    textArea->setChangedSinceLastChangeEvent(true);
> 
> Which test fails if you don’t add this?

That would be fast/forms/formchange-event.html =)

[Ilya Sherman]

Created attachment 84531 [details]
Patch

[WebKit Review Bot]

Attachment 84531 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebKit/mac/WebView/WebViewPrivate.h:680:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 84531 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:3182
> +            if (element->isTextFormControl()) {
> +                bool wasChanged = false;
> +                if (element->isHTMLElement())
> +                    wasChanged = static_cast<HTMLTextFormControlElement*>(element)->wasChangedSinceLastChangeEvent();
> +#if ENABLE(WML)
> +                else if (element->isWMLElement())
> +                    wasChanged = static_cast<WMLInputElement*>(element)->wasChangedSinceLastChangeEvent();
> +#endif

I think I steered you in the wrong direction moving this code out of Element like this. It kills me that WML is the reason we have to do it with virtual functions, but all these if statements and typecasts are terrible.

If we do stick with this design, then getting the “was changed” flag should be encapsulated in a separate function. The code would read much more clearly if it was.

Honestly, this code and the code below in RenderTextControlSingleLine::subtreeHasChanged seems like an argument for putting the virtual functions back into Element. All these specific cases are ugly and hide the real logic in a forest of trivia.

> Source/WebCore/html/HTMLFormControlElement.h:209
> +    virtual bool wasChangedSinceLastChangeEvent() const;
> +    virtual void setChangedSinceLastChangeEvent(bool);

Why are these functions virtual? They should not be!

> Source/WebCore/html/HTMLFormControlElement.h:237
> +    bool m_wasChangedSinceLastChangeEvent : 1;

Adding the bit toHTMLTextFormControlElement  is not great. By adding a single bit here we end up spending an entire machine word. I know the flag is only needed for the text controls, but I think it’s OK to have the bit in the base class. I suggest adding this to HTMLFormControlElement instead because that has other bits that this could combine with in a single word.

> Source/WebCore/html/HTMLInputElement.cpp:901
> +        // For text fields, don't dispatch the change event when focused.
> +        // It will be dispatched when the control loses focus.
> +        if (isTextField() && focused()) {
> +            setChangedSinceLastChangeEvent(true);
> +            dispatchFormControlInputEvent();
> +        } else
> +            dispatchFormControlChangeEvent();

This comment does not make it clear why it’s right to send an input event. I think we could have a much better comment here, while still keeping it short.

> Source/WebCore/html/HTMLInputElement.cpp:1046
> +        // Dispatch a change event for text fields that have been edited prior
> +        // to form submission. Normally this event is dispatched when the field
> +        // loses focus, but form submission is another special case where we
> +        // consider editing to have finished.

Can you say the same thing in a shorter comment? I would say something more like:

    // Form submission finishes editing as loss of focus does. If there was a change, send the event now.

> Source/WebCore/html/HTMLInputElement.cpp:1418
> +        if (isTextField())
> +            setChangedSinceLastChangeEvent(true);

Might be clearer to put this inside the else below. It’s a little confusing since the range control branch sends a change event and this involves change events too.

> Source/WebCore/rendering/RenderTextControlSingleLine.cpp:205
> +    bool wasChanged = false;
> +    if (element->isHTMLElement()) {
> +        HTMLTextFormControlElement* textFormControlElement = static_cast<HTMLTextFormControlElement*>(element);
> +        wasChanged = textFormControlElement->wasChangedSinceLastChangeEvent();
> +        textFormControlElement->setChangedSinceLastChangeEvent(true);
> +    }
> +#if ENABLE(WML)
> +    else if (element->isWMLElement()) {
> +        WMLInputElement* wmlInput = static_cast<WMLInputElement*>(element);
> +        wasChanged = wmlInput->wasChangedSinceLastChangeEvent();
> +        wmlInput->setChangedSinceLastChangeEvent(true);
> +    }
> +#endif

If we do stick with this design, then we should find some way to encapsulate this in a separate function. The code would read much more clearly if we could concentrate on what we are getting and setting and not so much on the mechanics of how.

> Source/WebCore/wml/WMLInputElement.h:70
> +    virtual bool wasChangedSinceLastChangeEvent() const { return m_wasChangedSinceLastChangeEvent; }
> +    virtual void setChangedSinceLastChangeEvent(bool changed) { m_wasChangedSinceLastChangeEvent = changed; }

Why are these functions virtual?

> Source/WebKit/mac/WebView/WebView.mm:6266
> +    JSElement* jsElement = static_cast<JSElement*>(asObject(jsElementValue));
> +    Element* webcoreElement = jsElement->impl();
> +    InputElement* inputElement = toInputElement(webcoreElement);

There are too many local variables here. Makes the code confusing for no good reason. It should just be like this:

    JSValue elementValue = toJS(exec, element);
    if (!elementValue.inherits(&JSElement::s_info))
        return;
    InputElement* inputElement = toInputElement(static_cast<JSElement*>(asObject(elementValue))->impl());

> Source/WebKit/mac/WebView/WebView.mm:6271
> +    if (!JSValueIsString(context, value))
> +        return;

This check is incorrect. Any JSValue can be converted to a string. There is no need to reject the argument since it isn’t already a string.

> Source/WebKit/mac/WebView/WebView.mm:6273
> +    JSStringRef jsStringValue = JSValueToStringCopy(context, value, NULL);
> +    String stringValue(jsStringValue->characters(), jsStringValue->length());

The jsStringValue here leaks. You should be using a JSRetainPtr or explicitly releasing it.

I don’t know why this code is compiling for you. For a JSStringRef, you can’t just call ->characters().

> Source/WebKit/mac/WebView/WebView.mm:6275
> +    inputElement->setValueForUser(stringValue);

The local variable is not needed. It doesn’t make things read better. It should be like this:

    JSRetainPtr<JSStringRef> string(Adopt, JSValueToStringCopy(context, value, 0);
    inputElement->setValueForUser(String(JSStringGetCharacterPtr(string.get()), JSStringGetLength(string.get())));

> Source/WebKit/mac/WebView/WebViewPrivate.h:681
> +- (void)_setValueForUser:(JSContextRef)context forElement:(JSValueRef)element withValue:(JSValueRef)value;

This is not the right way to hook this up; too much of the code is in WebKit here. You model for this should be _markerTextForListItem. The argument for the element can just be a DOMElement * and the argument for the value can just be a NSString *.

Conversion from a JSValueRef to a DOMElement * and an NSString * can be handled in the layout test controller.

[Darin Adler]

Thanks for your persistence on this. We’re getting there!

[Ilya Sherman]

Comment on attachment 84531 [details]
Patch

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

Thanks for your patience in reviewing this =)

>> Source/WebCore/dom/Document.cpp:3182
>> +#endif
> 
> I think I steered you in the wrong direction moving this code out of Element like this. It kills me that WML is the reason we have to do it with virtual functions, but all these if statements and typecasts are terrible.
> 
> If we do stick with this design, then getting the “was changed” flag should be encapsulated in a separate function. The code would read much more clearly if it was.
> 
> Honestly, this code and the code below in RenderTextControlSingleLine::subtreeHasChanged seems like an argument for putting the virtual functions back into Element. All these specific cases are ugly and hide the real logic in a forest of trivia.

Yeah, WML really doesn't play nicely with the inheritance hierarchy.  I switched back to hanging these functions off Element -- it adds a bit of bloat to the API, but I like the alternative even less :(

>> Source/WebCore/html/HTMLFormControlElement.h:209
>> +    virtual void setChangedSinceLastChangeEvent(bool);
> 
> Why are these functions virtual? They should not be!

Oops... they shouldn't have been, though now they again should be.

>> Source/WebCore/html/HTMLFormControlElement.h:237
>> +    bool m_wasChangedSinceLastChangeEvent : 1;
> 
> Adding the bit toHTMLTextFormControlElement  is not great. By adding a single bit here we end up spending an entire machine word. I know the flag is only needed for the text controls, but I think it’s OK to have the bit in the base class. I suggest adding this to HTMLFormControlElement instead because that has other bits that this could combine with in a single word.

Ah, somehow I missed the fact that HTMLFormControlElement had data members... oops.  Moved the code into there.

>> Source/WebKit/mac/WebView/WebView.mm:6273
>> +    String stringValue(jsStringValue->characters(), jsStringValue->length());
> 
> The jsStringValue here leaks. You should be using a JSRetainPtr or explicitly releasing it.
> 
> I don’t know why this code is compiling for you. For a JSStringRef, you can’t just call ->characters().

Uh, you can if you #include "OpaqueJSString.h" :P

Mostly I don't know this code at all, so I was just trying whatever I could to get the right logic to compile, and hoping that reviewers would tell me the proper way to do it.  So, thanks! =)

>> Source/WebKit/mac/WebView/WebViewPrivate.h:681
>> +- (void)_setValueForUser:(JSContextRef)context forElement:(JSValueRef)element withValue:(JSValueRef)value;
> 
> This is not the right way to hook this up; too much of the code is in WebKit here. You model for this should be _markerTextForListItem. The argument for the element can just be a DOMElement * and the argument for the value can just be a NSString *.
> 
> Conversion from a JSValueRef to a DOMElement * and an NSString * can be handled in the layout test controller.

Could you point me at where DOMElement, etc. are defined?  I found what looks like it might be an auto-generated header "DOMHTMLInputElement.h", but it doesn't have an accessor to the setValueForUser() function that I'm trying to expose.  Is it appropriate to expose this function there?  If so, approximately how do I set about doing so?

[Ilya Sherman]

Created attachment 84715 [details]
Patch

[WebKit Review Bot]

Attachment 84715 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebKit/mac/WebView/WebViewPrivate.h:680:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

(In reply to comment #41)
> >> Source/WebKit/mac/WebView/WebViewPrivate.h:681
> >> +- (void)_setValueForUser:(JSContextRef)context forElement:(JSValueRef)element withValue:(JSValueRef)value;
> > 
> > This is not the right way to hook this up; too much of the code is in WebKit here. You model for this should be _markerTextForListItem. The argument for the element can just be a DOMElement * and the argument for the value can just be a NSString *.
> > 
> > Conversion from a JSValueRef to a DOMElement * and an NSString * can be handled in the layout test controller.
> 
> Could you point me at where DOMElement, etc. are defined?  I found what looks like it might be an auto-generated header "DOMHTMLInputElement.h", but it doesn't have an accessor to the setValueForUser() function that I'm trying to expose.  Is it appropriate to expose this function there?  If so, approximately how do I set about doing so?

Those classes are defined in automatically generated headers.

To expose a setValueForUser function, you will want to use what’s called a “category” in Objective-C. The example here is the _markerTextForListItem method in WebDOMOperationsPrivate.h and WebDOMOperations.mm. You could either define this on DOMElement and have it just not do anything for non-input elements, or you could add a category on DOMHTMLInputElement named WebDOMElementOperationsPrivate, analogous to the one that exists for DOMElement.

You’ll call core(self) on the element to get a pointer to the WebCore DOM class.

[Ilya Sherman]

Created attachment 84824 [details]
Patch

[Ilya Sherman]

Comment on attachment 84824 [details]
Patch

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

> Source/WebKit/mac/DOM/WebDOMOperations.mm:202
> +    static_cast<HTMLInputElement*>(core(self))->setValueForUser(value);

This typecast seems redundant, but I'm not sure if there's a better way to achieve this.

> Tools/DumpRenderTree/mac/LayoutTestControllerMac.mm:485
> +    if (!element || ![element isKindOfClass:[DOMHTMLInputElement class]])

Is this type check the right way to go?

[WebKit Review Bot]

Attachment 84824 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebKit/mac/DOM/WebDOMOperationsPrivate.h:41:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 84824 [details]:

inspector/audits/audits-panel-functional.html bug 55776 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 84824 [details]
Patch

Clearing flags on attachment: 84824

Committed r80412: <http://trac.webkit.org/changeset/80412>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/80412 might have broken Windows 7 Release (Tests) and GTK Linux 32-bit Debug
The following tests are not passing:
fast/forms/onchange-setvalueforuser.html