[Webkit-unassigned] [Bug 26141] Implement onformchange and onforminput event handlers

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=26141


Kent Tamura <tkent at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #78913|review?                     |review-
               Flag|                            |




--- Comment #15 from Kent Tamura <tkent at chromium.org>  2011-01-17 17:25:48 PST ---
(From update of attachment 78913)
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).

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.