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

[bugzilla-daemon at webkit.org]

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





--- Comment #16 from Daniel Bates <dbates at webkit.org>  2009-11-01 14:50:36 PDT ---
(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.

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