Bug 129084 – m_ancestorDisabledState should never be unknown

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Ryosuke Niwa]

In order to resolve the bug 129035, we need to be notified whenever form control element's disabled-ness changes.

[Ryosuke Niwa]

Created attachment 224723 [details]
Patch

[Ryosuke Niwa]

Created attachment 224724 [details]
Reverted erroneous xcworkspace chagnes

[Benjamin Poulain]

Comment on attachment 224724 [details]
Reverted erroneous xcworkspace chagnes

View in context: https://bugs.webkit.org/attachment.cgi?id=224724&action=review

Ok with the idea but I have some questions when you have time.

> Source/WebCore/html/HTMLFieldSetElement.cpp:77
> +        document().removeDisabledFieldsetElement();

Don't you also need this on document destruction?

> Source/WebCore/html/HTMLFieldSetElement.cpp:89
> +        updateFromControlElementsAncestorDisabledStateUnder(control, thisFieldsetIsDisabled);

I am not following this. This loop will go over all the HTMLElements under the FieldSet. updateFromControlElementsAncestorDisabledStateUnder will again traverse the tree looking for any HTMLFormControlElement.
Aren't we traversing twice?

> Source/WebCore/html/HTMLFieldSetElement.cpp:104
> +    while ((legend = Traversal<HTMLLegendElement>::nextSibling(legend)))

I am not following here, why do check only the siblings instead of the whole tree after legend?

> Source/WebCore/html/HTMLFormControlElement.cpp:118
> +void HTMLFormControlElement::setAncestorDisabled(bool isDisabled)
>  {
> -    m_ancestorDisabledState = AncestorDisabledStateUnknown;
> -    disabledAttributeChanged();
> +    ASSERT(computeIsDisabledByFieldsetAncestor() == isDisabled);

Shouldn't this be updateIsAncestorDisabled() called without arguments disabledAttributeChanged().

[Ryosuke Niwa]

(In reply to comment #3)
> (From update of attachment 224724 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224724&action=review
> 
> Ok with the idea but I have some questions when you have time.
> 
> > Source/WebCore/html/HTMLFieldSetElement.cpp:77
> > +        document().removeDisabledFieldsetElement();
> 
> Don't you also need this on document destruction?

We don't because all fieldset elements would have been removed at that point.

> > Source/WebCore/html/HTMLFieldSetElement.cpp:89
> > +        updateFromControlElementsAncestorDisabledStateUnder(control, thisFieldsetIsDisabled);
> 
> I am not following this. This loop will go over all the HTMLElements under the FieldSet. updateFromControlElementsAncestorDisabledStateUnder will again traverse the tree looking for any HTMLFormControlElement.
> Aren't we traversing twice?

Here, I'm traversing over all child elements, and then for each child node and its descendants we update its internals states. We can't call updateFromControlElementsAncestorDisabledStateUnder on this here because we need to call it with isDisabled set to false when the child is the first legend element we saw.

> > Source/WebCore/html/HTMLFieldSetElement.cpp:104
> > +    while ((legend = Traversal<HTMLLegendElement>::nextSibling(legend)))
> 
> I am not following here, why do check only the siblings instead of the whole tree after legend?

See my change log description:
Update disabled-ness of form controls under child legend elements because controls aren't disabled in the first legend element, and adding or removing child elements may have changed the first legend element.

Removing or adding child nodes DO NOT change disabled-ness of elements outside of the new and the old first legend element.

> > Source/WebCore/html/HTMLFormControlElement.cpp:118
> > +void HTMLFormControlElement::setAncestorDisabled(bool isDisabled)
> >  {
> > -    m_ancestorDisabledState = AncestorDisabledStateUnknown;
> > -    disabledAttributeChanged();
> > +    ASSERT(computeIsDisabledByFieldsetAncestor() == isDisabled);
> 
> Shouldn't this be updateIsAncestorDisabled() called without arguments disabledAttributeChanged().

I don't understand what you're saying here.

[Ryosuke Niwa]

Created attachment 224818 [details]
Updated per review comments

[Ryosuke Niwa]

Committed r164475: <http://trac.webkit.org/changeset/164475>