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

Fri Oct 30 15:42:18 PDT 2009

https://bugs.webkit.org/show_bug.cgi?id=20780

--- Comment #11 from Daniel Bates <dbates at webkit.org>  2009-10-30 15:42:17 PDT ---
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.