[webkit-reviews] review denied: [Bug 58837] Fieldset disabled attribute does not work : [Attachment 126520] patch for review. - fixed issues as commented before.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Feb 11 01:24:56 PST 2012


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 126520: patch for review. - fixed issues as commented before.
https://bugs.webkit.org/attachment.cgi?id=126520&action=review

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


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

More tests:

- Add literal <fieldset disabled> and controls to the HTML to test tree
building by the HTML parser, instead of creating a tree by DOM operations.
- <form id=f1></form><fieldset disabled><input form=f1></fieldset>

> Source/WebCore/html/HTMLElement.cpp:876
> +   
static_cast<HTMLFormControlElement*>(this)->setDisablingAncestor(disablingAnces
tor);

This function is called with non-HTMLFormControlElement "this".  So the cast is
wrong.
http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/js/JSHTMLElementCu
stom.cpp#L59

I recommend introducing new function member to HTMLFormControlElement, instead
of updating HTMLElement:findFormAncestor().

> Source/WebCore/html/HTMLFieldSetElement.cpp:49
> +void HTMLFieldSetElement::childrenChanged(bool changedByParser, Node*
beforeChange, Node* afterChange, int childCountDelta)
> +{
> +    if (childCountDelta) {
> +	   // The first direct child HTMLLegendElement should never be
disabled.
> +	   HTMLLegendElement* firstLegendElement = 0;
> +	   for (Node* child = firstChild(); child ; child =
child->nextSibling()) {
> +	       HTMLElement* element = toHTMLElement(child);

This code will be O(N^2) if we add N children to a fieldset element.
A form control inside <legend> is very rare.  I don't think we need to pay such
cost for rare cases.

IMO,
 * HTMLFormControlElement should have HTMLFieldSetElement* member.
 * HTMLFormControlElement should have "bool m_isInsideLegend" member.
m_isInsideLegend should be true only if the control is inside a legend element
and the legend element is a child of a fieldset element.
 * HTMLFormControlElement::disabled() should check if the ancestor legend
element is the first child of the fieldset ancestor.

> Source/WebCore/html/HTMLFieldSetElement.cpp:57
> +		   legend->setNeedsStyleRecalc();

<legend> doesn't have :disabled/:enabled style.
http://www.whatwg.org/specs/web-apps/current-work/multipage/selectors.html#sele
ctor-enabled

> Source/WebCore/html/HTMLFormControlElement.cpp:66
>  {
> -    setForm(form ? form : findFormAncestor());
> +    setForm(form ? form : updateFormAncestors());
>      setHasCustomWillOrDidRecalcStyle();

This looks incorrect.
The form argument is non-NULL if the control is added to the document tree by
the HTML parser. The code doesn't update m_disablingAncestor in such case.


More information about the webkit-reviews mailing list