Bug 26141 – Implement onformchange and onforminput event handlers

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Sam Weinig]

We should implement onformchange, onforminput, and oninvalid event handlers from HTML5.

[Michelangelo De Simone]

oninvalid landed in r47649 (bug #27452). Updating bug details.

[Erik Arvidsson]

*** Bug 26551 has been marked as a duplicate of this bug. ***

[Dai Mikurube]

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.)

[Dai Mikurube]

Created attachment 74212 [details]
Patch

[Erik Arvidsson]

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.

[Erik Arvidsson]

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.

[Dai Mikurube]

Hi Erik,

Thanks for letting me know and your comments. Ok, I stop the work on it.

[Erik Arvidsson]

forminput and formchange are now resolved to not be removed. This means that patches are welcome again.

[Dai Mikurube]

Created attachment 78643 [details]
Patch

[Dai Mikurube]

(In reply to comment #8)
Hi Erik,

Thank you for the information. I've attached a new patch.

[Erik Arvidsson]

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

[Kent Tamura]

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.

[Dai Mikurube]

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().

[Dai Mikurube]

Created attachment 78913 [details]
Patch

[Kent Tamura]

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).

[Kent Tamura]

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>.

[Dai Mikurube]

Created attachment 79261 [details]
Patch

[Dai Mikurube]

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.

[Erik Arvidsson]

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

[Kent Tamura]

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.

[Dai Mikurube]

Created attachment 79358 [details]
Patch

[Dai Mikurube]

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.

[Kent Tamura]

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.

[Dai Mikurube]

Created attachment 79371 [details]
Patch

[Kent Tamura]

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".

[Dai Mikurube]

Created attachment 79389 [details]
Patch

[Dai Mikurube]

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.

[Kent Tamura]

Comment on attachment 79389 [details]
Patch

Looks good!

[Dai Mikurube]

(In reply to comment #28)
Kent-san, thank you for your review!

[WebKit Commit Bot]

Comment on attachment 79389 [details]
Patch

Clearing flags on attachment: 79389

Committed r76115: <http://trac.webkit.org/changeset/76115>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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

[Adele Peterson]

Anyone else seeing fast/forms/forminput-event.html crash on TOT?

[Adele Peterson]

This is the stderr output:

ASSERTION FAILED: this
(/OpenSource/Source/WebCore/dom/Node.h:348 WebCore::Document* WebCore::Node::document() const)

[Dai Mikurube]

(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)...

[Dai Mikurube]

(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'

[Kent Tamura]

(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".

[Olli Pettay (:smaug)]

(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

[Olli Pettay (:smaug)]

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.)