Bug 26141 - Implement onformchange and onforminput event handlers
: Implement onformchange and onforminput event handlers
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
: 55755
: 19264
  Show dependency treegraph
 
Reported: 2009-06-02 10:45 PST by
Modified: 2011-03-04 02:04 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-06-02 10:45:41 PST
We should implement onformchange, onforminput, and oninvalid event handlers from HTML5.
------- Comment #1 From 2010-04-03 02:56:35 PST -------
oninvalid landed in r47649 (bug #27452). Updating bug details.
------- Comment #2 From 2010-10-22 11:09:18 PST -------
*** Bug 26551 has been marked as a duplicate of this bug. ***
------- Comment #3 From 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 From 2010-11-18 00:46:56 PST -------
Created an attachment (id=74212) [details]
Patch
------- Comment #5 From 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 From 2010-11-18 09:34:31 PST -------
(From update of attachment 74212 [details])
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 From 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 From 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 From 2011-01-11 20:02:11 PST -------
Created an attachment (id=78643) [details]
Patch
------- Comment #10 From 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 From 2011-01-12 11:07:35 PST -------
(From update of attachment 78643 [details])
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 From 2011-01-12 23:24:30 PST -------
(From update of attachment 78643 [details])
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 From 2011-01-13 23:57:42 PST -------
(From update of attachment 78643 [details])
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 From 2011-01-14 02:02:43 PST -------
Created an attachment (id=78913) [details]
Patch
------- Comment #15 From 2011-01-17 17:25:48 PST -------
(From update of attachment 78913 [details])
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 From 2011-01-17 17:30:47 PST -------
(From update of attachment 78913 [details])
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 From 2011-01-18 04:34:07 PST -------
Created an attachment (id=79261) [details]
Patch
------- Comment #18 From 2011-01-18 04:36:40 PST -------
(From update of attachment 78913 [details])
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 From 2011-01-18 12:53:17 PST -------
(From update of attachment 79261 [details])
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 From 2011-01-18 16:39:04 PST -------
(From update of attachment 79261 [details])
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 From 2011-01-18 17:09:23 PST -------
Created an attachment (id=79358) [details]
Patch
------- Comment #22 From 2011-01-18 17:14:47 PST -------
(From update of attachment 79261 [details])
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 From 2011-01-18 17:27:24 PST -------
(From update of attachment 79358 [details])
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 From 2011-01-18 17:50:03 PST -------
Created an attachment (id=79371) [details]
Patch
------- Comment #25 From 2011-01-18 22:27:45 PST -------
(From update of attachment 79371 [details])
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 From 2011-01-18 22:30:32 PST -------
Created an attachment (id=79389) [details]
Patch
------- Comment #27 From 2011-01-18 22:31:00 PST -------
(From update of attachment 79371 [details])
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 From 2011-01-18 22:39:03 PST -------
(From update of attachment 79389 [details])
Looks good!
------- Comment #29 From 2011-01-18 22:40:06 PST -------
(In reply to comment #28)
Kent-san, thank you for your review!
------- Comment #30 From 2011-01-19 04:56:36 PST -------
(From update of attachment 79389 [details])
Clearing flags on attachment: 79389

Committed r76115: <http://trac.webkit.org/changeset/76115>
------- Comment #31 From 2011-01-19 04:56:44 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #32 From 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 From 2011-01-19 12:47:50 PST -------
Anyone else seeing fast/forms/forminput-event.html crash on TOT?
------- Comment #34 From 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 From 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 From 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 From 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 From 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 From 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.)