| Summary: | Implement onformchange and onforminput event handlers | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||||||||||
| Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | abarth, adele, amwebdk, arv, bugs, commit-queue, dmikurube, eric, michelangelo, peter, tkent, webkit.review.bot, webmaster | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | All | ||||||||||||||||||
| OS: | All | ||||||||||||||||||
| Bug Depends on: | 55755 | ||||||||||||||||||
| Bug Blocks: | 19264 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Sam Weinig
2009-06-02 10:45:41 PDT
oninvalid landed in r47649 (bug #27452). Updating bug details. *** Bug 26551 has been marked as a duplicate of this bug. *** I'm working on this bug. formchange events look like working. But facing difficulties in handling forminput events. Could someone give me a few pointers? Briefly, I cannot find exactly all sites to dispatch forminput events. I found many sites dispatching input and TextInput events where I need to also dispatch forminput events. One option for them is to dispatch forminput events at all of sites dispatching input and TextInput events I found. But it looks not good since it looses maintainability and I can miss some sites. Another option is to modify dispatchEvent() to catch all of input and TextInput events. But specializing for input events at dispatchEvent() looks not good. For formchange events, I could focus on HTMLFormControlElement::dispatchFormControlChangeEvent() to find sites dispatching change events. I'm wondering if you could tell me some appropriate methods to hook all sites dispatching input events accurately. I attach the current patch. (It doesn't work for forminput events.) Created attachment 74212 [details]
Patch
onformchange, onforminut and the dispatch methods are to be removed from HTML5 because they are not useful enough. See http://www.w3.org/Bugs/Public/show_bug.cgi?id=11129 Sorry, for not updating this bug earlier. Sam, since you opened this I'll let you make the call to close it or not. Comment on attachment 74212 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74212&action=review > WebCore/html/HTMLFormElement.cpp:583 > + for (unsigned i = 0; i < m_associatedElements.size(); ++i) { These are not the right elements to dispatch to. Check the spec (it hasn't been removed yet). > WebCore/html/HTMLInputElement.cpp:1989 > dispatchEvent(Event::create(eventNames().inputEvent, true, false)); I think it would be cleaner to have a dispatchInputEvent that handled the forminput too. Hi Erik, Thanks for letting me know and your comments. Ok, I stop the work on it. forminput and formchange are now resolved to not be removed. This means that patches are welcome again. Created attachment 78643 [details]
Patch
(In reply to comment #8) Hi Erik, Thank you for the information. I've attached a new patch. Comment on attachment 78643 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78643&action=review > LayoutTests/fast/forms/script-tests/formchange-event.js:3 > +function sendKey(element, keyName) { You should probably use eventSender here instead. > LayoutTests/fast/forms/script-tests/formchange-event.js:52 > +shouldBe("result1.innerHTML", '"none"'); You can use shouldBeEqualToString > Source/WebCore/html/HTMLElement.cpp:828 > + dispatchEvent(Event::create(eventNames().inputEvent, true, false)); Node::dispatchInputEvents(); > Source/WebCore/html/HTMLFormControlElement.cpp:181 > +void HTMLFormControlElement::dispatchFormControlInputEvent() Why is this needed? I think you can remove dispatchFormControlInputEvent and simply use dispatchInputEvents? If you really don't want to do the extra work that HTMLElement::dispatchInputEvents does you could make HTMLFormControlElement override dispatchFormInputs > Source/WebCore/html/HTMLFormElement.cpp:595 > + for (unsigned i = 0; i < m_associatedElements.size(); ++i) { This should be the "resettable elements" and not the "associated elements" http://dev.w3.org/html5/spec/Overview.html#broadcast-forminput-events http://dev.w3.org/html5/spec/Overview.html#category-reset http://dev.w3.org/html5/spec/Overview.html#form-associated-element > Source/WebCore/html/HTMLFormElement.cpp:607 > + for (unsigned i = 0; i < m_associatedElements.size(); ++i) { same here Comment on attachment 78643 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78643&action=review Please see HTMLFormElement::collectUnhandledInvalidControls() for how to fix the following dispatchEvent() issues. >> LayoutTests/fast/forms/script-tests/formchange-event.js:3 >> +function sendKey(element, keyName) { > > You should probably use eventSender here instead. I like avoiding eventSender in order to make the test workable on browsers. >> Source/WebCore/html/HTMLElement.cpp:828 >> + dispatchEvent(Event::create(eventNames().inputEvent, true, false)); > > Node::dispatchInputEvents(); Because dispatchEvent() may call JavaScript event handlers, "this" object can be deleted in the event handlers. > Source/WebCore/html/HTMLFormControlElement.cpp:183 > + dispatchEvent(Event::create(eventNames().inputEvent, true, false)); ditto. "this" might be deleted in dispatchEvent(). We can't call any methods of this. > Source/WebCore/html/HTMLFormElement.cpp:600 > + if (!formElement->dispatchEvent(Event::create(eventNames().forminputEvent, false, false))) ditto. dispatchEvent() calls JavaScript code. "this" can be deleted and m_associatedElements can be updated. > Source/WebCore/html/HTMLFormElement.cpp:612 > + if (!formElement->dispatchEvent(Event::create(eventNames().formchangeEvent, false, false))) ditto. Comment on attachment 78643 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78643&action=review >>> LayoutTests/fast/forms/script-tests/formchange-event.js:3 >>> +function sendKey(element, keyName) { >> >> You should probably use eventSender here instead. > > I like avoiding eventSender in order to make the test workable on browsers. I decided to use these methods for the reason which Kent-san said. (FYI: I'm using eventSender in forminput-event.js to fire input events.) >> LayoutTests/fast/forms/script-tests/formchange-event.js:52 >> +shouldBe("result1.innerHTML", '"none"'); > > You can use shouldBeEqualToString Thanks. Replaced to shouldBeEqualToString. >>> Source/WebCore/html/HTMLElement.cpp:828 >> >> Node::dispatchInputEvents(); > > Because dispatchEvent() may call JavaScript event handlers, "this" object can be deleted in the event handlers. Erik> Replaced to Node::dispatchInputEvents(); Kent-san> Modified it by reference to HTMLFormElement::collectUnhandledInvalidControls(). >> Source/WebCore/html/HTMLFormControlElement.cpp:181 > > Why is this needed? I think you can remove dispatchFormControlInputEvent and simply use dispatchInputEvents? > > If you really don't want to do the extra work that HTMLElement::dispatchInputEvents does you could make HTMLFormControlElement override dispatchFormInputs Changed HTMLFormControlElement::dispatchFormControlInputEvent() just to call HTMLElement::dispatchChangeEvents(). However dispatchFormControlInputEvent() cannot be removed since dispatchInputEvent() and dispatchFormControlInputEvent have difference behaviors in case of non-HTML elements. dispatchInputEvent() fires an "input" event, and dispatchFormControlInputEvent() fires nothing for non-HTML elements. >> Source/WebCore/html/HTMLFormElement.cpp:595 > > This should be the "resettable elements" and not the "associated elements" > > http://dev.w3.org/html5/spec/Overview.html#broadcast-forminput-events > http://dev.w3.org/html5/spec/Overview.html#category-reset > http://dev.w3.org/html5/spec/Overview.html#form-associated-element Introduced isResettable() into FormAssociatedElement, and checked with it. >> Source/WebCore/html/HTMLFormElement.cpp:600 >> + if (!formElement->dispatchEvent(Event::create(eventNames().forminputEvent, false, false))) > > ditto. > dispatchEvent() calls JavaScript code. "this" can be deleted and m_associatedElements can be updated. Modified it by reference to HTMLFormElement::collectUnhandledInvalidControls(). Created attachment 78913 [details]
Patch
Comment on attachment 78913 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78913&action=review > LayoutTests/fast/forms/script-tests/formchange-event.js:25 > +var results = document.createElement('ul'); > +document.body.appendChild(results); > +var result1 = document.createElement('li'); > +result1.setAttribute('id', 'result1'); > +results.appendChild(result1); > +var result2 = document.createElement('li'); > +result2.setAttribute('id', 'result2'); > +results.appendChild(result2); > +var result3 = document.createElement('li'); > +result3.setAttribute('id', 'result3'); > +results.appendChild(result3); I don't think we need to use a DOM tree to represent these results. Three global variables, or one object with three properties should be enough. > LayoutTests/fast/forms/script-tests/formchange-event.js:29 > +form.innerHTML = "<ul><li id='li1'></li><li id='li2'></li><li id='li3'></li><li id='li4'></li><li id='li5'></li><li id='li6'></li><li id='li7'></li></ul>"; Do we need these <li> elements? Why don't you build these and the following input element by just one .innerHTML? > LayoutTests/fast/forms/script-tests/formchange-event.js:33 > +document.getElementById('li1').innerHTML = '<input type="number" id="input1" value="28" onformchange="document.getElementById(\'result1\').innerText=\'foo\'" />'; > +document.getElementById('li2').innerHTML = '<input type="number" id="input2" value="12" />'; > +document.getElementById('li3').innerHTML = '<input type="text" id="input3" value="some" onformchange="document.getElementById(\'result3\').innerText=\'bar\'" />'; Please set meaningful strings instead of "foo" "bar". For example, innerText = 'input1-formchange-called'. > LayoutTests/fast/forms/script-tests/formchange-event.js:44 > +var input52 = document.getElementById('input52'); nit: If you call document.getElmentById() many times, you may define a function like function $(id) { return document.getElementById(id); } It helps to make the code cleaner. > LayoutTests/fast/forms/script-tests/formchange-event.js:168 > +var successfullyParsed = true; We need more tests: - Remove an associated element in an event handler - Remove the form element from the DOM tree in an event handler > LayoutTests/fast/forms/script-tests/forminput-event.js:1 > +description('Test for forminput events.'); Same comments as formchange-event.js > Source/WebCore/html/HTMLElement.cpp:879 > + RefPtr<HTMLFormElement> ownerForm; > + Node* ancestorNode = shadowAncestorNode(); > + if (ancestorNode) { > + if (!ancestorNode->isHTMLElement()) > + return; > + HTMLElement* ancestorHTML = static_cast<HTMLElement*>(ancestorNode); > + if (!ancestorHTML) > + return; > + ownerForm = ancestorHTML->form(); > + } else > + ownerForm = form(); This part is identical to dispatchChangeEvents(). We had better introduce a new function for the common part. > Source/WebCore/html/HTMLFormElement.cpp:604 > + RefPtr<FormAssociatedElement> formAssociatedElement = elements[i]; We don't need to use RefPtr<> and a raw pointer is enough because elements[i] holds a reference count. > Source/WebCore/html/HTMLFormElement.cpp:607 > + RefPtr<HTMLFormControlElement> formElement = static_pointer_cast<HTMLFormControlElement>(formAssociatedElement); ditto. RefPtr<> is not needed. > Source/WebCore/html/HTMLFormElement.cpp:628 > + if (!formElement->dispatchEvent(Event::create(eventNames().formchangeEvent, false, false))) The difference from dispatchFormInput() is only the event name. We had better have a new function like broadcastFormEvent(const AtomicString& eventName). Comment on attachment 78913 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78913&action=review >> LayoutTests/fast/forms/script-tests/formchange-event.js:168 >> +var successfullyParsed = true; > > We need more tests: > - Remove an associated element in an event handler > - Remove the form element from the DOM tree in an event handler Also, we should have tests for <keygen>, <object>, <output>, <select>, and <textarea>. Created attachment 79261 [details]
Patch
Comment on attachment 78913 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78913&action=review Changed two tests (formchange-event and forminput-event) totally. >> Source/WebCore/html/HTMLElement.cpp:879 >> + ownerForm = form(); > > This part is identical to dispatchChangeEvents(). We had better introduce a new function for the common part. Done. Thanks. >> Source/WebCore/html/HTMLFormElement.cpp:604 >> + RefPtr<FormAssociatedElement> formAssociatedElement = elements[i]; > > We don't need to use RefPtr<> and a raw pointer is enough because elements[i] holds a reference count. Stopped to use RefPtr here. Thank you. >> Source/WebCore/html/HTMLFormElement.cpp:628 >> + if (!formElement->dispatchEvent(Event::create(eventNames().formchangeEvent, false, false))) > > The difference from dispatchFormInput() is only the event name. We had better have a new function like broadcastFormEvent(const AtomicString& eventName). Created a new function. Comment on attachment 79261 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79261&action=review > Source/WebCore/html/HTMLFormElement.cpp:598 > + Vector<RefPtr<FormAssociatedElement> > elements; Would it make sense to add an accessor for Resettable Elements? I think these collections come up a lot in forms handling and I think it would be better to expose these better. http://www.w3.org/TR/html5/forms.html#categories Comment on attachment 79261 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79261&action=review There are minor comments. > LayoutTests/fast/forms/script-tests/formchange-event.js:64 > + result_removing_input = 'Delivered'; We don't use '_' for variable names even in JavaScript. it should be resultRemovingInput. >> Source/WebCore/html/HTMLFormElement.cpp:598 >> + Vector<RefPtr<FormAssociatedElement> > elements; > > Would it make sense to add an accessor for Resettable Elements? > > I think these collections come up a lot in forms handling and I think it would be better to expose these better. > > http://www.w3.org/TR/html5/forms.html#categories I don't think the resettable element collection is needed by other classes. > Source/WebCore/html/HTMLFormElement.cpp:601 > + for (unsigned i = 0; i < m_associatedElements.size(); ++i) > + elements.append(m_associatedElements[i]); We can avoid to append non-resettable elements by checking isResettable() here. Created attachment 79358 [details]
Patch
Comment on attachment 79261 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79261&action=review >> LayoutTests/fast/forms/script-tests/formchange-event.js:64 >> + result_removing_input = 'Delivered'; > > We don't use '_' for variable names even in JavaScript. it should be resultRemovingInput. Thank you. Done. >>> Source/WebCore/html/HTMLFormElement.cpp:598 >>> + Vector<RefPtr<FormAssociatedElement> > elements; >> >> Would it make sense to add an accessor for Resettable Elements? >> >> I think these collections come up a lot in forms handling and I think it would be better to expose these better. >> >> http://www.w3.org/TR/html5/forms.html#categories > > I don't think the resettable element collection is needed by other classes. Erik : Thank you for the comments. Does it mean the current handling may be slow for a lot of elements in forms? Or another way to expose resettable properties would be better for other purpose? >> Source/WebCore/html/HTMLFormElement.cpp:601 >> + elements.append(m_associatedElements[i]); > > We can avoid to append non-resettable elements by checking isResettable() here. Kent-san: Thanks. Moved the checking. Comment on attachment 79358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79358&action=review > Source/WebCore/html/HTMLFormElement.cpp:601 > + if (!m_associatedElements[i]->isFormControlElement() || !m_associatedElements[i]->isResettable()) You shouldn't check isFormControlElement() here. We might have resettable non-HTMLFormControlElement classes in the future. > Source/WebCore/html/HTMLFormElement.cpp:607 > + HTMLFormControlElement* formElement = static_cast<HTMLFormControlElement*>(elements[i].get()); Yes, we can cast elements[i] to HTMLFormControlElement unconditionally because a resettable control is always HTMLFormControlElement for now. But please add ASSERT(elements[i]->isFormControlElement()) before this line and add a comment about a reason why we don't need to handle non-HTMLFormControlElement case. Created attachment 79371 [details]
Patch
Comment on attachment 79371 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79371&action=review > Source/WebCore/html/HTMLFormElement.cpp:607 > + // We can assume a resettable control is always a HTMLFormControlElement as of Jan 19, 2011. A comment with a date looks very strange. Please remove "as of Jan 19, 2011". Created attachment 79389 [details]
Patch
Comment on attachment 79371 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79371&action=review >> Source/WebCore/html/HTMLFormElement.cpp:607 >> + // We can assume a resettable control is always a HTMLFormControlElement as of Jan 19, 2011. > > A comment with a date looks very strange. Please remove "as of Jan 19, 2011". Ok, removed it. Comment on attachment 79389 [details]
Patch
Looks good!
(In reply to comment #28) Kent-san, thank you for your review! Comment on attachment 79389 [details] Patch Clearing flags on attachment: 79389 Committed r76115: <http://trac.webkit.org/changeset/76115> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/76115 might have broken Qt Linux Release The following tests are not passing: fast/dom/Window/window-properties.html fast/dom/Window/window-property-descriptors.html Anyone else seeing fast/forms/forminput-event.html crash on TOT? This is the stderr output: ASSERTION FAILED: this (/OpenSource/Source/WebCore/dom/Node.h:348 WebCore::Document* WebCore::Node::document() const) (In reply to comment #34) Hi Adele, Thank you for the information. Umm, the current HEAD (pulled) passed the tests in my environment (Mac, Snow Leopard)... (In reply to comment #32) Checked the following tests in QtLinux WebKit, > fast/dom/Window/window-properties.html > fast/dom/Window/window-property-descriptors.html I found the error points were not from this patch. The errors were for fast/dom/Window/window-properties.html: - 2301 window.onorientationchange [null] - 2335 window.orientation [number] for fast/dom/Window/window-property-descriptors.html: - 44 PASS typeof Object.getOwnPropertyDescriptor(window, 'DeviceMotionEvent') is 'object' - 45 PASS typeof Object.getOwnPropertyDescriptor(window, 'DeviceOrientationEvent') is 'object' - 371 PASS typeof Object.getOwnPropertyDescriptor(window, 'ondevicemotion') is 'object' - 405 PASS typeof Object.getOwnPropertyDescriptor(window, 'onorientationchange') is 'object' - 437 PASS typeof Object.getOwnPropertyDescriptor(window, 'orientation') is 'object' (In reply to comment #33) > Anyone else seeing fast/forms/forminput-event.html crash on TOT? (In reply to comment #34) > This is the stderr output: > > ASSERTION FAILED: this > (/OpenSource/Source/WebCore/dom/Node.h:348 WebCore::Document* WebCore::Node::document() const) Adele, I couldn't reproduce this assertion failure with r76315 Debug on Snow Leopard. I tried "run-webkit-tests --debug fast/forms/" and "run-webkit-tests --debug fast/forms/forminput-event.html". (In reply to comment #8) > forminput and formchange are now resolved to not be removed. This means that patches are welcome again. Just FYI, the bug to remove forminput and formchange has been re-opened http://www.w3.org/Bugs/Public/show_bug.cgi?id=11129 Since the code was just committed to webkit, would you be willing to back it out if forminput/change will be removed from HTML? So far I haven't seen *any* real use cases for forminput/change which couldn't be trivially achieved using just the old input/change events. We shouldn't be polluting web platform with rather useless features. (If someone comes up with a good and reasonable use case for these events, I would change my mind immediately.) |