Bug 20780 – when choosing an auto-completed value in a text input field the javascript 'change' event is not fired.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Beat]

Steps to reproduce:

1) open any url with a form on any page (which doesn't have autocomplete="off" attribute)
2) fill in the form and submit it
3) open same url again
4) type-in same first letter then view autocompletion possibility, and with arrow key down+tab complete the field (or clicking the autocomplete proposal and then pressing TAB key has same effect)

the 'onchange'/'change' event isn't fired.

You can try that on the url in 'URL', which fires a jQuery change event to do an ajax email-check. If you type in with keyboard a new value, it fires change, but not with auto-complete.

Same thing when you have only one autocomplete value, even with typing with keyboard there is no change even fired if you accept the value by hitting tab.

This report, although with the same page as url, is different from the private security report 20778.

[Stephen]

Created attachment 26798 [details]
Simple HTML file with inline SCRIPT test

Demonstrates that in Safari/Webkit Version 3.2.1 (5525.27.1) no change event is triggered when a value is selected from a drop down provided via autocomplete.

"The change event occurs when a control loses the input focus and its value has been modified since gaining focus." http://www.w3.org/TR/DOM-Level-2-Events/events.html

related chromium bug: http://code.google.com/p/chromium/issues/detail?id=10879
related live test case: http://data.ici-bas.fr/chromiumissue10879/
would you please confirm this bug?

[Beat]

(In reply to comment #2)
> related chromium bug: http://code.google.com/p/chromium/issues/detail?id=10879
> related live test case: http://data.ici-bas.fr/chromiumissue10879/
> would you please confirm this bug?

Most probably same bug, I can use that demo to show that the changed didn't get fired with following steps:


Steps

First click in the pseudo field, type "mypseudo", click submit or hit enter - the onchange event works

Then click in the pseudo field, type "newvalue" do NOT submit, just CLICK outside the box - onchange is shown.

Then click in the pseudo field, type "my" and select "mypseudo" from dropdown list 

THEN just click outside the box - the onchange event does not work in Webkit / Safari 4 too....

So i can confirm it with that demo too.

[Daniel Bates]

Thanks Beat, Stephen, and Thomas for reporting this issue.

I have confirmed this issue is still present in the latest nightly build r49748.

[Daniel Bates]

Created attachment 41387 [details]
Patch (work in progress)

Work in progress.

Need to confirm that this is the right fix. Also, will need a test, probably a manual test will have to do since I doubt we have anything in DRT to test this.

[Daniel Bates]

Confirmed this also affects the Windows build (r49984).

[Daniel Bates]

Created attachment 41629 [details]
Patch with manual test

We cannot test this with DRT since DRT cannot emulate autocompletion. So, a manual test is included.

[Daniel Bates]

Remarks:

Whenever you press a key on the keyboard:
RenderTextControl::subtreeHasChanged is called, which sets m_edited, m_userEdited = true. 

When autocompletion kicks in the call flow is:
DOMHTML::_replaceCharactersInRange->HTMLInputElement::setValue->RenderTextControlSingleLine::updateFromElement->RenderTextControl::setInnerTextValue

Note, RenderTextControl::setInnerTextValue always sets m_edited, m_userEdited = false.

Since a user must type at least one character before the field is autocompleted (if applicable), RenderTextControl::setInnerTextValue should not explicitly set m_edited = false. Instead, RenderTextControl::setInnerTextValue should not touch this field.

On another note, the field RenderTextControl::m_userEdited seems to imply whether the field was edited by the user. Since the auto-completed result is not typed by the user, I believe m_userEdited should be set to false in RenderTextControl::setInnerTextValue (as it is currently).

[Eric Seidel (no email)]

CCing folks who might know this code.

[Darin Adler]

Comment on attachment 41629 [details]
Patch with manual test

claim that DumpRenderTree cannot emulate autocompletion, but I am not sure this is true. My reading of Safari's auto-fill code indicates that it works identically to setting the value attribute from JavaScript would. Can you reproduce the same problem with code that sets the value in JavaScript? If not, why not?

I'm definitely not clear on whether this change is always correct for setInnerTextValue. That's a quite-low-level function used in many different cases. For example, is this correct for the case where the placeholder is visible and RenderTextControlMultiLine::updateFromElement calls setInnerTextValue?

review- and please answer my questions -- it's possible we may be able to land this patch as is once you do, but I suspect this is not sufficiently researched and tested

Further, I think the code you are modifying needs a comment. It's not obvious why m_userEdited should be set to false there and why m_edited should not be. With a comment we might have a fighting chance of getting it right in the future.

[Daniel Bates]

Please excuse the length of my response. It is very long, as I tried to fully explain my proposed change. I've made an attempt to answer your questions at the very beginning of each response, as well as put all my assumptions and claims in the third and sixth paragraphs (demarcated as well) so that you can "bail out" before finishing if you disagree with my assumptions/claims.

(In reply to comment #10)
> claim that DumpRenderTree cannot emulate autocompletion, but I am not sure this
> is true. My reading of Safari's auto-fill code indicates that it works
> identically to setting the value attribute from JavaScript would. Can you
> reproduce the same problem with code that sets the value in JavaScript? If not,
> why not?

So far, I cannot reproduce the same problem with code that sets the value in JavaScript, because in addition to the call to RenderTextControl::setInnerTextValue there is another call from the keyboard key press that causes the state of RenderTextControl to change. (I guess once I figure out how to do DOM keyboard events to simulate a key press then I may be able to create a JavaScript based Layout Test for this issue - I was able to whip up a quick example for Firefox using their DOM keyboard events API and it looks promising). Regardless, the following fix I proposed is still needed.

The context of this bug is when the user has typed at least one character into the text input before it is auto-completed (either by the auto-fill code or via some JavaScript action). For your reference, we are interested in conforming to section 18.2.3 "Intrinsic events" of the HTML 4.01 spec. <http://www.w3.org/TR/REC-html40/interact/scripts.html#h-18.2.3>, which states that: "The onchange event occurs when a control loses the input focus and its value has been modified since gaining focus" ($).

ASSUMPTION: My interpretation of ($), is that the user must explicitly focus the input (by clicking on it) and choose to lose focus on the input (by clicking outside of the field). That is, the onchange event should not fire if all of this is done programmatically via JavaScript without at least one DOM keyboard event to simulate pressing a key. From my testing in Firefox, it seem to agree with this interpretation of the spec. If you have or anyone else has an opinion on this, please let me know. The remainder of my response assumes this interpretation.

From tracing the execution in a debugger, RenderTextControl::m_edited is only set to true in RenderTextControl::subtreeHasChanged() (**). And, RenderTextControl::subtreeHasChanged() is only called when an event that changes the editable content (i.e. the value of the field) is fired (such as a key press by the user - line 128 of TextControlInnerTextElement.cpp, <http://trac.webkit.org/browser/trunk/WebCore/rendering/TextControlInnerElements.cpp?rev=46815#L128>) (***).

(*) Currently, RenderTextControl::setInnerTextValue sets m_edited = false. And, both the auto-fill code and setting the value attribute from JavaScript call this method.

CLAIM: I claim there is an issue with the definition of what it means for a field to have been edited. The solution I proposed in my patch corrects this bug by re-defining this definition to be: A field is said to been edited by a person if and only if the person explicitly inserted, deleted, or substituted at least one character.

Lets look at the current definition and the one I propose:

Extracting the current definition from the code, I have: A field is said to been edited by a person if and only if the person explicitly inserted, deleted, or substituted every character. Why "every" character? By (*), since if one change is made via the auto-fill code or JavaScript, RenderTextControl::m_edited is set to false (i.e the field has not been edited).

I believe this definition needs to be relaxed a bit to correct this bug. Let A be a text input field on the page. Assume that a person neither selects View->AutoFill Form nor causes some JavaScript to fill in A. Suppose that the person types at least one character into A and that the auto-fill code completes what they were typing. Then (I claim) the field should be considered to have been edited, because the person typed at least one character into A. Therefore, the definition of a field that has been edited should be: A field is said to been edited by a person if and only if the person explicitly inserted, deleted, or substituted at least one character. ((%) Notice, this definition produces the correct result if we suppose that a person selects View->AutoFill Form or causes some JavaScript to fill in A. That is, the field is said to have NOT been edited - at least not by a human).

Translating this definition into implementation code and noticing that RenderTextControl initializes m_edited to false (in its constructor <http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderTextControl.cpp?rev=48792#L73>), we see that we should remove line 198 of RenderTextControl.cpp <http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderTextControl.cpp?rev=48792#L198> (#). A side-effect of making this change is that once RenderTextControl::m_edited is set to true by (**) it is never is reset to false. This would be problematic if it wasn't for the fact that every caller that calls B := RenderTextControl::isEdited() to get the value of m_edited subsequently calls RenderTextControl::setEdited(false) (if B == true), which sets RenderTextControl::m_edited to false. So, we have moved the responsibility of re-setting RenderTextControl::m_edited to false to the caller, but this is already being done (see below files)! Hence, we can remove (#).

For your reference, isEdited() is called in files:
Line 283 of WebCore/wml/WMLInputElement.cpp <http://trac.webkit.org/browser/trunk/WebCore/wml/WMLInputElement.cpp?rev=48792#L283>
Line 2657 of WebCore/dom/Document.cpp <http://trac.webkit.org/browser/trunk/WebCore/dom/Document.cpp?rev=50351#L2657>
Line 1545 of WebCore/html/HTMLInputElement.cpp <http://trac.webkit.org/browser/trunk/WebCore/html/HTMLInputElement.cpp?rev=50132#L1545>

(##) Line 153 of WebCore/rendering/RenderTextControlSingleLine.cpp <http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderTextControlSingleLine.cpp#L153>

With regards to (##), by making the change (#) we correct another bug with respect to the RenderTextControlSingleLine::subtreeHasChanged(). Notice, we call Frame::textFieldDidBeginEditing if the field is auto-completed and then the user changes the value by pressing a key on the keyboard. To see this do the following: Run the test for this bug, when the auto-fill code triggers and completes the word apple, press some other key on the keyboard, say "d". Notice, you still have focus of the input. When you press "d", (##) evaluates to false! And thus the code will call Frame::textFieldDidBeginEditing (on line 172 <http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderTextControlSingleLine.cpp#L172>). Instead, Frame::textDidChangeInTextField should have been called (line 177 <http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderTextControlSingleLine.cpp#L177>), because we just made a change.

> I'm definitely not clear on whether this change is always correct for
> setInnerTextValue. That's a quite-low-level function used in many different
> cases. For example, is this correct for the case where the placeholder is
> visible and RenderTextControlMultiLine::updateFromElement calls
> setInnerTextValue?

Yes, it is correct because of (***). Notice, class RenderTextControlMultiLine extends class RenderTextControl. By default RenderTextControl::m_edited is false. Only when the user explicitly types a key into the input, does RenderTextControl::m_edited become true. Using a debugger, it can can be observed that whenever a key is pressed, RenderTextControlMultiLine::subtreeHasChanged always fires before RenderTextControlMultiLine::updateFromElement, which combined with (#) yields the correct behavior.

[Daniel Bates]

Created attachment 42248 [details]
Alternative patch with manual test

After further thought, I came up with an alternative patch that doesn't have to touch the low-level details of RenderTextControl.

Instead, I have modified the corresponding routine called by the auto-fill code in the Mac and Windows builds to store the value of whether the RenderTextControl has already been edited (such as if the user started typing the value that is going to be auto-completed) and to restore the edited state after the we set the field to the auto-completed value.

[Daniel Bates]

Created attachment 42279 [details]
Patch with manual test

I updated the original patch based on Darin's comments.

I also added a comment to to RenderTextControl.h to remind developers to call setEdited(false) after they fire an onchange event. From searching the code, this is already being done.

[Darin Adler]

Given that m_edited is used only for change events, I am sure that the change in your patch is correct.

Pointing me at the related code I realize that there are things that would make the code clearer:

    1) The three copies of the code to dispatch a change event if a text control was edited should share code. RenderTextControl could have a public function to do this and the isEdited and setEdited functions could be either eliminated or made private.

    2) The rule for when a change event is dispatched seems wrong. I would think there are other times we'd have to dispatch a change event. For example, if the form is submitted some way other than by hitting return and there was an edited <input> element in that form. Or if you are leaving the page for some other reason, although the form element still has focus. We should write more test cases and file bugs if we differ from other browsers.

    3) The Document::setFocusedNode and WMLInputElement::defaultEventHandler functions should use the dispatchFormControlChangeEvent function.

    4) The code in Document::setFocusedNode to specifically deal with the text controls should instead be in setFocus function overrides in the two classes that can have text controls (HTMLInputElement, HTMLTextAreaElement), three counting WMLInputElement. It is bad factoring to have element-specific code in Document::setFocusedNode, even though it does save a few lines of code.

    5) The call to setEdited(false) should be done before dispatching the event. It’s not clear to me that there’s any benefit to how it’s done now and I think we might be able to construct a test case demonstrating some minor bug. Even if we don't put code to dispatch the event into RenderTextControl as I suggest in (3) above, perhaps we should at least make the function that returns the bit also clear it as a side effect.

    6) For m_edited I would prefer a name that makes the relationship with the change event clear. Perhaps m_wasChangedSinceLastChangedEvent. For the same reason, m_userEdited could be renamed m_wasEditedSinceSetProgrammatically. Or better, shorter names with a similar meaning. Both of my suggested names are wordy, but m_edited and m_userEdited are too vague.

None of these changes need to go into your bug fix -- just things I noticed because I had to read the code.

[Darin Adler]

Comment on attachment 42279 [details]
Patch with manual test

I'm now comfortable with the code change after your explanation.

I'm still slightly puzzled that we can't exercise this code path any way in DumpRenderTree, but you say so and I'll take your word for it. Maybe there's something simple we can do in DumpRenderTree so it can simulate the auto-fill code path just enough so this can be regression tested.

> -        m_edited = false;
> +        // We set m_userEdited to false so that we do not ignore the HTML 5 autofocus attribute on this form control.

This seems like a really mysterious comment. I don't understand how autofocus is related to this.

> +        // Why do we set this directly as opposed to calling setUserEdited(false) here?

This comment isn't needed. It's common to set data members inside a class rather than calling functions, and that's probably a good idiom.

In fact, I think it would be best to eliminate the setUserEdited function entirely. As far as I can tell nobody calls it.

> +    // Note, after you fire an onchange event, remember to call setEdited(false) to reset the edited state of this control.
>      bool isEdited() const { return m_edited; }
>      void setEdited(bool isEdited) { m_edited = isEdited; }

This comment points out the need for a design change. We don't want to make functions you can "use wrong" if we can avoid it.

[Daniel Bates]

(In reply to comment #15)
> (From update of attachment 42279 [details])
> I'm now comfortable with the code change after your explanation.

Great! Thank you for taking the time to look over this.

> I'm still slightly puzzled that we can't exercise this code path any way in
> DumpRenderTree, but you say so and I'll take your word for it. Maybe there's
> something simple we can do in DumpRenderTree so it can simulate the auto-fill
> code path just enough so this can be regression tested.

I'll take a look.

> 
> > -        m_edited = false;
> > +        // We set m_userEdited to false so that we do not ignore the HTML 5 autofocus attribute on this form control.
> 
> This seems like a really mysterious comment. I don't understand how autofocus
> is related to this.

(*) Notice, RenderTextControl::setUserEdited sets m_userEdited AND calls Document::setIgnoreAutofocus.

This comment was derived from the comment above Document::setIgnoreAutofocus in file Document.h <http://trac.webkit.org/browser/trunk/WebCore/dom/Document.h?rev=49998#L557>.

From my understanding and testing, Document::setIgnoreAutofocus is part of the implementation used to support the HTML 5 autofocus attribute <http://dev.w3.org/html5/spec/Overview.html#autofocusing-a-form-control>.

> 
> > +        // Why do we set this directly as opposed to calling setUserEdited(false) here?
> 
> This comment isn't needed. It's common to set data members inside a class
> rather than calling functions, and that's probably a good idiom.

See notice (*) above. I added this comment because it is unclear to me why we are explicitly setting the value of a data member here when RenderTextControl::setUserEdited also sets this data member plus calls Document::setIgnoreAutofocus. Why aren't we calling Document::setIgnoreAutofocus here?

> 
> In fact, I think it would be best to eliminate the setUserEdited function
> entirely. As far as I can tell nobody calls it.
> 
> > +    // Note, after you fire an onchange event, remember to call setEdited(false) to reset the edited state of this control.
> >      bool isEdited() const { return m_edited; }
> >      void setEdited(bool isEdited) { m_edited = isEdited; }
> 
> This comment points out the need for a design change. We don't want to make
> functions you can "use wrong" if we can avoid it.

I agree, I also do not like functions that you can "use wrong". This is the reason that lead me to come up with the Alternative patch with manual test <https://bugs.webkit.org/attachment.cgi?id=42248>.

I am still reading through you earlier comment which suggests cleaning up the code in the caller functions. Let me finish reading through and processing your earlier comment. Maybe we can find a clean solution.

[Daniel Bates]

(In reply to comment #15)

> 
> In fact, I think it would be best to eliminate the setUserEdited function
> entirely. As far as I can tell nobody calls it.

Forgot to address this bit in my last comment. Yeah, we could remove it as I don't see anyone calling this in the code. Probably best in a separate bug? I'll get back to you about this and the rest of your earlier comment shortly.

[Darin Adler]

(In reply to comment #16)
> (*) Notice, RenderTextControl::setUserEdited sets m_userEdited AND calls
> Document::setIgnoreAutofocus.

Since nobody calls it, that doesn’t matter. We should go back and find the change where someone modified the setUserEdited function and see if there’s a test case that should be failing. It seems like the autofocus work was done incorrectly since modifying a function that nobody calls clearly can’t have an effect.

[Daniel Bates]

I think it would probably be best if this function was removed in a separate bug. Let me know if you think we should remove this function in this patch.

For your reference:

RenderTextControl::setUserEdited was first added in r24652 <http://trac.webkit.org/changeset/24652> as part of the patch for bug <rdar://problem/5359921> (I don't know what the issue was other than what is implied from the change log comment, since I don't have rdar access).

Then in r34626 <http://trac.webkit.org/changeset/34626>, RenderTextControl::setUserEdited was changed to include the the line "document()->setIgnoreAutofocus(isUserEdited)" as part of the patch for bug #18887.

The following test cases were added mentioned in the change log for bug #18887:
fast/forms/autofocus-attribute.html
fast/forms/autofocus-opera-001.html
fast/forms/autofocus-opera-002.html
fast/forms/autofocus-opera-003.html
fast/forms/autofocus-opera-004.html
fast/forms/autofocus-opera-005.html
fast/forms/autofocus-opera-006.html
fast/forms/autofocus-opera-007.html
fast/forms/autofocus-opera-008.html

CC'ing Michelangelo De Simone, as he may have some insight into r34626, including the change to RenderTextControl::setUserEdited and the autofocus code.

(In reply to comment #18)
> (In reply to comment #16)
> > (*) Notice, RenderTextControl::setUserEdited sets m_userEdited AND calls
> > Document::setIgnoreAutofocus.
> 
> Since nobody calls it, that doesn’t matter. We should go back and find the
> change where someone modified the setUserEdited function and see if there’s a
> test case that should be failing. It seems like the autofocus work was done
> incorrectly since modifying a function that nobody calls clearly can’t have an
> effect.

[Daniel Bates]

Created attachment 42289 [details]
Patch with manual test

Updated according to Darin's suggestions. 

Asked Mark to clarify <rdar://problem/5359921> so that I could write a more descriptive comment above the line "m_userEdited = false" in RenderTextControl::setInnerTextValue.

[Daniel Bates]

Comment on attachment 42289 [details]
Patch with manual test

Forgot to rename m_edited/m_userEdited as per Darin's comment, to make it clear what their purpose is

[Michelangelo De Simone]

(In reply to comment #19)

> Then in r34626 <http://trac.webkit.org/changeset/34626>,
> RenderTextControl::setUserEdited was changed to include the the line
> "document()->setIgnoreAutofocus(isUserEdited)" as part of the patch for bug
> #18887.

Thanks for your feedback. Please, feel free to eliminate that line of code ("document()->setIgnoreAutofocus(isUserEdited)") from RenderTextControl::setUserEdited if you need it.

I'll address this issue in another bug and I'm gonna eliminate such line anyway. Thank you.

[Daniel Bates]

Created attachment 42464 [details]
Patch with manual test

Renamed RenderTextControl::m_edited to m_wasEditedSinceLastChangeEvent and RenderTextControl::m_userEdited to m_wasLastEditedByAUser, to reflect that whether the text control was changed since the last change event fired and whether a human performed the last edit (as opposed to being done programmatically), respectively. Also, renamed their corresponding getters and setters.

Let me know if these names are not sufficient.

[Darin Adler]

Typically it's best to address new issues in a separate bug. The naming issue is not the one this bug describes. And the automated commit queue tool works best when each bug is resolved with a single patch.

(In reply to comment #23)
> Renamed RenderTextControl::m_edited to m_wasEditedSinceLastChangeEvent

Maybe it should be m_wasChangedSinceLastChangeEvent. I’m not sure setting the text of a control counts as "being edited".

> RenderTextControl::m_userEdited to m_wasLastEditedByAUser

I don’t think "a user" is good here. We normally leave our articles from our names. I also think that we should not call setting the text "being edited". Perhaps m_wasEditedByUser is all we need for a good name here. Or maybe m_lastChangeWasUserEdit. Maybe there's a better idea for the name. But in general I think we should call changes done by the HTML editing commands "editing" and other changes not be called editing.

[Daniel Bates]

Comment on attachment 42464 [details]
Patch with manual test

As per Darin's comment, it would be cleaner if we rename the fields as part of a separate bug. So, I am removing the review flag on this patch.

[Daniel Bates]

Comment on attachment 42289 [details]
Patch with manual test

Marking this patch for review.

[Daniel Bates]

Filed a new bug #31186, to address the renaming of the fields in RenderTextControl. We will probably need to file some more cleanup bugs to address some of the other issues that Darin brought up in comment #14.

[Daniel Bates]

Committed r50618: <http://trac.webkit.org/changeset/50618>

[John Sullivan]

This change introduced a subtle bug in Safari's autocomplete that's tracked by <rdar://problem/7470529>. It's not completely clear to me yet whether Safari was relying on undefined or incorrect behavior, or whether this change introduced incorrect behavior. I'm looking into this more.

[John Sullivan]

It turns out that the regression in Safari's autocomplete behavior was caused by a combination of this change and an earlier one, <https://bugs.webkit.org/show_bug.cgi?id=32904>. Reverting this change alone doesn't fix the general problem, though it makes some cases work better.

However, it's not clear to me that this change is conceptually incorrect, and in any case I will work around the introduced problems in Safari.