[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