[webkit-reviews] review denied: [Bug 58837] Fieldset disabled attribute does not work : [Attachment 125343] patch for review.

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Zeno Albisser <zeno at webkit.org>'s
request for review:
Bug 58837: Fieldset disabled attribute does not work
https://bugs.webkit.org/show_bug.cgi?id=58837

Attachment 125343: patch for review.
https://bugs.webkit.org/attachment.cgi?id=125343&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=125343&action=review


> LayoutTests/fast/forms/fieldset-disabled.html:163
> +</html>

We should have more tests:
 - 'disabled' IDL attribute should reflect 'disabled' HTML attribute regardless
of enclosing fieldsets
 - <input> is not disabled in a case of <fieldset
disabled><legend><input></legend></fieldset>
 - Changing disabled state of a <fieldset> correctly updates the style of
descendant form controls.
  You can use document.defaultView.getComputedStyle(element,
null).getProeprtyeValue() or docment.querySelector()/querySelectorAll().

> Source/WebCore/html/FormAssociatedElement.cpp:97
> +    if (element->isFormControlElement())
> +	  
static_cast<HTMLFormControlElement*>(element)->updateFieldSetAncestor();

The code should be in HTMLFormControlElement::insertedIntoTree().

> Source/WebCore/html/HTMLFieldSetElement.cpp:63
> +    if (attr->name() == disabledAttr) {

We should do this expensive operation only if disabled state is changed.
We had better introduce HTMLFormControlElement::disabledAttributeChanged()
virtual function like requiredAttributeChanged().

> Source/WebCore/html/HTMLFieldSetElement.cpp:80
> +	   Vector<Node*> nodesToUpdate;
> +	   nodesToUpdate.append(this);
> +	   while (!nodesToUpdate.isEmpty()) {
> +	       // Take the last node from the vector and mark it for style
recalculation.
> +	       Node* currentNode = nodesToUpdate.last();
> +	       nodesToUpdate.removeLast();
> +	       HTMLElement* element = toHTMLElement(currentNode);
> +	       if (element && element->isFormControlElement())
> +		  
static_cast<HTMLFormControlElement*>(element)->setNeedsStyleRecalc();
> +
> +	       // Append the children of currentNode to the vector of nodes
that need to be updated.
> +	       currentNode = currentNode->firstChild();
> +	       while (currentNode) {
> +		   nodesToUpdate.append(currentNode);
> +		   currentNode = currentNode->nextSibling();
> +	       }
> +	   }

Node::traverseNextNode(this) is helpful.

> Source/WebCore/html/HTMLFieldSetElement.h:42
> +    virtual void parseMappedAttribute(Attribute*);

You should append 'OVERRIDE'.

> Source/WebCore/html/HTMLFieldSetElement.idl:27
> +	   attribute [Reflect] boolean disabled;

Please insert this line at the beginning. We'd like to follow the IDL in the
HTML specification as possible.
http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#the-fiel
dset-element