We have too much code for TextFieldInputType in HTMLInputElement::setValue. We should those into TextFieldInputType::setValue.
Created attachment 106663 [details] Patch
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.
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.
(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.
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?
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 :(
Created attachment 106697 [details] Updated per comment
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().
(In reply to comment #8) > - introduce another virtual function, InputType::dispatchChangeEvent(), and TextFieldInputType overrides it. This might be a role of valueChanged().
(In reply to comment #8) > - introduce another virtual function, InputType::dispatchChangeEvent(), and TextFieldInputType overrides it. That's much cleaner!
Created attachment 106699 [details] Updated
(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.
(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.
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.
(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.
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.
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.
Committed r94836: <http://trac.webkit.org/changeset/94836>