In order to resolve the bug 129035, we need to be notified whenever form control element's disabled-ness changes.
Created attachment 224723 [details] Patch
Created attachment 224724 [details] Reverted erroneous xcworkspace chagnes
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().
(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.
Created attachment 224818 [details] Updated per review comments
Committed r164475: <http://trac.webkit.org/changeset/164475>