[webkit-reviews] review denied: [Bug 58837] Fieldset disabled attribute does not work : [Attachment 127810] patch for review. - removed obsolete HTMLLegendElement::fieldset() as well.

[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 127810: patch for review. - removed obsolete
HTMLLegendElement::fieldset() as well.
https://bugs.webkit.org/attachment.cgi?id=127810&action=review

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


> Source/WebCore/html/HTMLFormControlElement.cpp:72
> +    if (form) {
> +	   setForm(form);
> +	   updateFieldSetAncestor();
> +    } else
> +	   updateFormAndFieldSetAncestor();
> +

This might make a performance regression because of ancestor lookup.

> I was also considering to make the parser pass a fieldset and a legend as a
parameter to the HTMLFormControlElement constructor.
> Would this solution be prefered? - I considered it slightly too intrusive and
therefore did not implement it.

Ideally the parser should pass <fieldset>/<legend> ancestors to the
constructor. Let's talk to the parser guru.

> Source/WebCore/html/HTMLFormControlElement.cpp:119
> +	   if (!formAncestor && ancestor->hasTagName(formTag)) {
> +	       formAncestor = static_cast<HTMLFormElement*>(ancestor);
> +	       break;

We shouldn't break here for the following case:
<fieldset><form><input></form></fieldset>

> Source/WebCore/html/HTMLFormControlElement.cpp:128
> +    if (formAncestor)
> +	   setForm(formAncestor);

We need to call setForm() even if formAncestor == 0.

> Source/WebCore/html/HTMLFormControlElement.cpp:255
> +    updateFormAndFieldSetAncestor();

I guess we don't need this change because
FormAssociatedElement::insertedIntoTree() does it.

On the other hand, we should do something in
FormAssociatedElement::removedFromTree() to refresh m_fieldSetAncestor and
mlegendAncestor?

> Source/WebCore/html/HTMLFormControlElement.cpp:313
> +    return !inFirstLegendOfFieldSet && (m_disabled || (m_fieldSetAncestor &&
m_fieldSetAncestor->disabled()));

It's incorrect.
return m_disabled || (m_fieldSetAncesotr && m_fieldSetAncestor->disabled() &&
!inFirstLegendOfFieldSet);

Because we need to check m_fieldSetAncestor to compute inFirstLegendOfFieldSet,
we had better do:

if (m_disabled)
    return true;
if (!m_fieldSetAncestor)
    return false;
if (!m_fieldSetAncesotr->disabled())
    return false;
if (!m_legendAncestor)
    return true;
return m_legendAncestor == m_fieldSetAncestor->firstElementChild();  // Note:
this is incorrect.

m_fieldSetAncestor->firstElementChild() is incorrect.  The specification says
"the fieldset element's first legend element child".  We need to find the first
legend element child, then compare to m_legendAncestor.