Bug 26141 - Implement onformchange and onforminput event handlers
Summary: Implement onformchange and onforminput event handlers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 26551 (view as bug list)
Depends on: 55755
Blocks: HTML5Forms
  Show dependency treegraph
 
Reported: 2009-06-02 10:45 PDT by Sam Weinig
Modified: 2011-03-04 02:04 PST (History)
13 users (show)

See Also:


Attachments
Patch (34.55 KB, patch)
2010-11-18 00:46 PST, Dai Mikurube
no flags Details | Formatted Diff | Diff
Patch (42.85 KB, patch)
2011-01-11 20:02 PST, Dai Mikurube
no flags Details | Formatted Diff | Diff
Patch (50.08 KB, patch)
2011-01-14 02:02 PST, Dai Mikurube
no flags Details | Formatted Diff | Diff
Patch (48.41 KB, patch)
2011-01-18 04:34 PST, Dai Mikurube
no flags Details | Formatted Diff | Diff
Patch (48.18 KB, patch)
2011-01-18 17:09 PST, Dai Mikurube
no flags Details | Formatted Diff | Diff
Patch (48.38 KB, patch)
2011-01-18 17:50 PST, Dai Mikurube
no flags Details | Formatted Diff | Diff
Patch (48.36 KB, patch)
2011-01-18 22:30 PST, Dai Mikurube
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2009-06-02 10:45:41 PDT
We should implement onformchange, onforminput, and oninvalid event handlers from HTML5.
Comment 1 Michelangelo De Simone 2010-04-03 02:56:35 PDT
oninvalid landed in r47649 (bug #27452). Updating bug details.
Comment 2 Erik Arvidsson 2010-10-22 11:09:18 PDT
*** Bug 26551 has been marked as a duplicate of this bug. ***
Comment 3 Dai Mikurube 2010-11-18 00:46:42 PST
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.)
Comment 4 Dai Mikurube 2010-11-18 00:46:56 PST
Created attachment 74212 [details]
Patch
Comment 5 Erik Arvidsson 2010-11-18 09:34:01 PST
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 6 Erik Arvidsson 2010-11-18 09:34:31 PST
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.
Comment 7 Dai Mikurube 2010-11-18 20:02:04 PST
Hi Erik,

Thanks for letting me know and your comments. Ok, I stop the work on it.
Comment 8 Erik Arvidsson 2011-01-04 09:54:05 PST
forminput and formchange are now resolved to not be removed. This means that patches are welcome again.
Comment 9 Dai Mikurube 2011-01-11 20:02:11 PST
Created attachment 78643 [details]
Patch
Comment 10 Dai Mikurube 2011-01-11 20:03:08 PST
(In reply to comment #8)
Hi Erik,

Thank you for the information. I've attached a new patch.
Comment 11 Erik Arvidsson 2011-01-12 11:07:35 PST
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 12 Kent Tamura 2011-01-12 23:24:30 PST
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 13 Dai Mikurube 2011-01-13 23:57:42 PST
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().
Comment 14 Dai Mikurube 2011-01-14 02:02:43 PST
Created attachment 78913 [details]
Patch
Comment 15 Kent Tamura 2011-01-17 17:25:48 PST
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 16 Kent Tamura 2011-01-17 17:30:47 PST
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>.
Comment 17 Dai Mikurube 2011-01-18 04:34:07 PST
Created attachment 79261 [details]
Patch
Comment 18 Dai Mikurube 2011-01-18 04:36:40 PST
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 19 Erik Arvidsson 2011-01-18 12:53:17 PST
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 20 Kent Tamura 2011-01-18 16:39:04 PST
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.
Comment 21 Dai Mikurube 2011-01-18 17:09:23 PST
Created attachment 79358 [details]
Patch
Comment 22 Dai Mikurube 2011-01-18 17:14:47 PST
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 23 Kent Tamura 2011-01-18 17:27:24 PST
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.
Comment 24 Dai Mikurube 2011-01-18 17:50:03 PST
Created attachment 79371 [details]
Patch
Comment 25 Kent Tamura 2011-01-18 22:27:45 PST
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".
Comment 26 Dai Mikurube 2011-01-18 22:30:32 PST
Created attachment 79389 [details]
Patch
Comment 27 Dai Mikurube 2011-01-18 22:31:00 PST
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 28 Kent Tamura 2011-01-18 22:39:03 PST
Comment on attachment 79389 [details]
Patch

Looks good!
Comment 29 Dai Mikurube 2011-01-18 22:40:06 PST
(In reply to comment #28)
Kent-san, thank you for your review!
Comment 30 WebKit Commit Bot 2011-01-19 04:56:36 PST
Comment on attachment 79389 [details]
Patch

Clearing flags on attachment: 79389

Committed r76115: <http://trac.webkit.org/changeset/76115>
Comment 31 WebKit Commit Bot 2011-01-19 04:56:44 PST
All reviewed patches have been landed.  Closing bug.
Comment 32 WebKit Review Bot 2011-01-19 06:09:19 PST
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
Comment 33 Adele Peterson 2011-01-19 12:47:50 PST
Anyone else seeing fast/forms/forminput-event.html crash on TOT?
Comment 34 Adele Peterson 2011-01-19 12:48:50 PST
This is the stderr output:

ASSERTION FAILED: this
(/OpenSource/Source/WebCore/dom/Node.h:348 WebCore::Document* WebCore::Node::document() const)
Comment 35 Dai Mikurube 2011-01-19 18:12:24 PST
(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)...
Comment 36 Dai Mikurube 2011-01-19 19:27:32 PST
(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'
Comment 37 Kent Tamura 2011-01-20 17:42:30 PST
(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".
Comment 38 Olli Pettay (:smaug) 2011-01-24 11:31:41 PST
(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
Comment 39 Olli Pettay (:smaug) 2011-01-27 02:08:40 PST
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.)