Bug 58837 – Fieldset disabled attribute does not work

• Component: Forms
• Status: RESOLVED FIXED

[Naoki Takano]

http://www.w3schools.com/html5/tryit.asp?filename=tryhtml5_fieldset_disabled

Chrome Version       : 12.0.725.0 (Official Build 80304) dev
URLs (if applicable) : http://www.w3schools.com/html5/tryit.asp?filename=tryhtml5_fieldset_disabled

What steps will reproduce the problem?
1. Go the the above w3schools page
2. Type some text in one of the boxes on the right
3.

What is the expected result?
The boxes are disabled due to the "disabled" tag of the HTML5 fieldset

What happens instead?
You can write in the boxes

Comment 1 by [e-mail], Apr 8, 2011
Other browsers tested:
  Add OK or FAIL after other browsers where you have tested this issue:
     IE 9: OK
     Firefox 4: OK

Not only chrome, but I guess Safari have the same problem.

[Naoki Takano]

Now I know the reason. But still I'm pondering how I should implement it.

<FieldSet> is very similar to <Form> as parent-child relation.  The relation between <Form> and other child field elements is tied in the child element class, FormAssociatedElement.

It is the start point that we implement the similar relation between <FieldSet> and other input field elements.

In FormAssociatedElement,
1, The parent form element, HTMLFormElement, is passed into the constructor and the parent form is set.
2, If no valid parent is passed, traverse ancestors to find the form parent.
3, If the node doesn't have the form parent and is inserted into the node tree, insertedIntoTree() is called and traverse ancestors to the form parent.

As you know, HTMLFieldSetElement is not special element as HTMLFormElement. So we cannot choose 1 method for HTMLFieldSetElement.

At first, I tried to implement 2 and 3 methods to find HTMLFieldSetElement like HTMLFormElement. But traverse is pretty heavy process and I suspect that it might affect some perfomance impact to Webkit rendering. Because FormAssociatedElement is derived by all input field elements.

Is there any other better way you recommend?

Or is it Ok to implement traverse to find HTMLFieldSetElement?

Please give me any suggestion, comment and advice.

Thanks,

[Naoki Takano]

Does anybody give me advice or comment?

[Kent Tamura]

(In reply to comment #1)
> At first, I tried to implement 2 and 3 methods to find HTMLFieldSetElement like HTMLFormElement. But traverse is pretty heavy process and I suspect that it might affect some perfomance impact to Webkit rendering. Because FormAssociatedElement is derived by all input field elements.

Please go ahead the traversal implementation, and let's test the performance by appending hundreds form controls to a node of which depth is a few hundreds.

[Naoki Takano]

Sure!!

Just a moment...

(In reply to comment #3)
> (In reply to comment #1)
> > At first, I tried to implement 2 and 3 methods to find HTMLFieldSetElement like HTMLFormElement. But traverse is pretty heavy process and I suspect that it might affect some perfomance impact to Webkit rendering. Because FormAssociatedElement is derived by all input field elements.
> 
> Please go ahead the traversal implementation, and let's test the performance by appending hundreds form controls to a node of which depth is a few hundreds.

[Naoki Takano]

Created attachment 90901 [details]
Patch

[Naoki Takano]

Comment on attachment 90901 [details]
Patch

Please review.

The current test time is 1.2sec. in my environment.

Is this too slow? What do you think?

Thanks,

[Build Bot]

Attachment 90901 [details] did not build on win:
Build output: http://queues.webkit.org/results/8510164

[Kent Tamura]

(In reply to comment #6)
> The current test time is 1.2sec. in my environment.
> Is this too slow? What do you think?

How different from the time without the patch?

[Kent Tamura]

Comment on attachment 90901 [details]
Patch

When the HTMLFieldSet::disabled is flipped, we need to update the style.

[Naoki Takano]

Thank you for review.

I have a question.

Do you have any special API call to update the style?

(In reply to comment #9)
> (From update of attachment 90901 [details])
> When the HTMLFieldSet::disabled is flipped, we need to update the style.

[Kent Tamura]

(In reply to comment #10)
> Do you have any special API call to update the style?

Oh, I'm sorry.  I found HTMLFieldSetElement inherited HTMLFormControlElement, and HTMLFormControlElement::parseMappedAttribute() already had setNeedsStyleRecalc().
However, we should have a test which confirms HTMLFieldSetElement::disabled changes the appearance of its descendant controls.

[Naoki Takano]

I measured again.

Then
new: 1.8sec.
old: 0.5sec.
Hmmmmm... it's pretty slow...

Is there any good way to improve the time?

One thing in my mind is to pass fieldset parent when HTMLFormControlElement is constructed like form parent.

What do you think?

(In reply to comment #8)
> (In reply to comment #6)
> > The current test time is 1.2sec. in my environment.
> > Is this too slow? What do you think?
> 
> How different from the time without the patch?

[Kent Tamura]

(In reply to comment #12)
> One thing in my mind is to pass fieldset parent when HTMLFormControlElement is constructed like form parent.

It will work only if the control is created by the parser.  It won't work for Document::createElement().

HTMLFormControlElement::disabled() is always called in CSS style matching.  So lazy-evaluation for m_fieldSet won't help.

It is slow because we need to traverse to the root if there is no fieldset ancestor.  We need a efficient way to check existence of fieldset elements.

[Naoki Takano]

I see.

If we implement efficient check for fieldset,  do you have any suggestion?

Easiest way is to cache the fieldset information into the whole node. But this causes resource problem ;-(

Or do we already have the similar element to refer the implementation?

(In reply to comment #13)
> (In reply to comment #12)
> > One thing in my mind is to pass fieldset parent when HTMLFormControlElement is constructed like form parent.
> 
> It will work only if the control is created by the parser.  It won't work for Document::createElement().
> 
> HTMLFormControlElement::disabled() is always called in CSS style matching.  So lazy-evaluation for m_fieldSet won't help.
> 
> It is slow because we need to traverse to the root if there is no fieldset ancestor.  We need a efficient way to check existence of fieldset elements.

[Kent Tamura]

(In reply to comment #14)
> Easiest way is to cache the fieldset information into the whole node. But this causes resource problem ;-(

Adding a new data member to Node is not acceptable.  But we have RareData mechanism.  We can associate an HTMLFieldSet reference to a Node only if the Node has an HTMLFieldSet ancestor.

Another idea is to add a <fieldset> counter to Document.  If the counter is 0, we don't need to search ancestors for a fieldset.


Darin, Adele,
do you have any other ideas?

[Naoki Takano]

Darin or Adele,

Do you have any comments?

I want to choose RareData method tkent suggested.

Thanks,

(In reply to comment #15)
> (In reply to comment #14)
> > Easiest way is to cache the fieldset information into the whole node. But this causes resource problem ;-(
> 
> Adding a new data member to Node is not acceptable.  But we have RareData mechanism.  We can associate an HTMLFieldSet reference to a Node only if the Node has an HTMLFieldSet ancestor.
> 
> Another idea is to add a <fieldset> counter to Document.  If the counter is 0, we don't need to search ancestors for a fieldset.
> 
> 
> Darin, Adele,
> do you have any other ideas?

[Naoki Takano]

I'm looking into NodeRareData.

So can I add another property into nodelist?

(In reply to comment #16)
> Darin or Adele,
> 
> Do you have any comments?
> 
> I want to choose RareData method tkent suggested.
> 
> Thanks,
> 
> (In reply to comment #15)
> > (In reply to comment #14)
> > > Easiest way is to cache the fieldset information into the whole node. But this causes resource problem ;-(
> > 
> > Adding a new data member to Node is not acceptable.  But we have RareData mechanism.  We can associate an HTMLFieldSet reference to a Node only if the Node has an HTMLFieldSet ancestor.
> > 
> > Another idea is to add a <fieldset> counter to Document.  If the counter is 0, we don't need to search ancestors for a fieldset.
> > 
> > 
> > Darin, Adele,
> > do you have any other ideas?

[Naoki Takano]

I asked Adele a couple of times, he looks very busy.

Is there anybody who can give me any comment about rare data mechanism?

As I wrote here, I try to add fieldset node pointer into nodelist and associate the node to the fieldset. Because it's kinda cache to the fieldset traversing.

Thanks,

(In reply to comment #17)
> I'm looking into NodeRareData.
> 
> So can I add another property into nodelist?
> 
> (In reply to comment #16)
> > Darin or Adele,
> > 
> > Do you have any comments?
> > 
> > I want to choose RareData method tkent suggested.
> > 
> > Thanks,
> > 
> > (In reply to comment #15)
> > > (In reply to comment #14)
> > > > Easiest way is to cache the fieldset information into the whole node. But this causes resource problem ;-(
> > > 
> > > Adding a new data member to Node is not acceptable.  But we have RareData mechanism.  We can associate an HTMLFieldSet reference to a Node only if the Node has an HTMLFieldSet ancestor.
> > > 
> > > Another idea is to add a <fieldset> counter to Document.  If the counter is 0, we don't need to search ancestors for a fieldset.
> > > 
> > > 
> > > Darin, Adele,
> > > do you have any other ideas?

[Darin Adler]

I don’t think rare data is relevant here. For the record, the cost of adding new rare data is:

    1) Every nodes with rare data uses a bit more memory.
    2) If this adds rare data to more nodes, then memory use goes up by a lot because all other rare data is allocated for each node, too.

But having a pointer to the fieldset from every form element is not the right way to go about this. Instead, when a fieldset is disabled we should iterate all the form items in the fieldset and tell them its state has changed. Then the form items would only need a single bit to keep track of whether the fieldset is disabled.

Similarly we need to detect when DOM manipulations change which fieldset a form element is in.

We need to do more than just return the right value from the disabled function; there is work to do at the time the disabled state changed. We need to trigger the appropriate style recalculation. Actively toggling a flag may is probably the best way to get the code right.

The code in the patch is not really close to right yet, because it doesn’t deal with the style recalculation issue at all.

[Naoki Takano]

Darin,

Thank you for your comment.

I have a couple of questions to make sure.

(In reply to comment #19)
> I don’t think rare data is relevant here. For the record, the cost of adding new rare data is:
> 
>     1) Every nodes with rare data uses a bit more memory.
>     2) If this adds rare data to more nodes, then memory use goes up by a lot because all other rare data is allocated for each node, too.
> 
> But having a pointer to the fieldset from every form element is not the right way to go about this. Instead, when a fieldset is disabled we should iterate all the form items in the fieldset and tell them its state has changed. Then the form items would only need a single bit to keep track of whether the fieldset is disabled.

I got it. I will add a single bit flag (something like "bool m_parentDisabled : 1;"?) in HTMLFormControlElement. The flag should be independent from m_disabled, because that is only for HTMLFormControlElement instance.

> Similarly we need to detect when DOM manipulations change which fieldset a form element is in.

I have a question. So do you mean to traverse the whole child nodes of fieldset whenever DOM manipulation of HTMLFormControlEmenet happens? It sounds like pretty heavy if here are many fieldsets... What do you think?


> We need to do more than just return the right value from the disabled function; there is work to do at the time the disabled state changed. We need to trigger the appropriate style recalculation. Actively toggling a flag may is probably the best way to get the code right.
> 
> The code in the patch is not really close to right yet, because it doesn’t deal with the style recalculation issue at all.

Ok, I want to make sure. If I try to implement how you suggested here, I will make the new function "setParentDisabled(bool)" in HTMLFormControlElement and triggers setNeedsStyleRecalc() inside. Also when fieldset disable flag is changed, then child setParentDisabled() fucntions are called. Is this correct?

Thanks

[Naoki Takano]

> Ok, I want to make sure. If I try to implement how you suggested here, I will make the new function "setParentDisabled(bool)" in HTMLFormControlElement and triggers setNeedsStyleRecalc() inside. Also when fieldset disable flag is changed, then child setParentDisabled() fucntions are called. Is this correct?
After thinking again, I suppose we don't have to call setNeedsStyleRecalc(), because, as tkent already mentioned, 
"HTMLFieldSetElement inherited HTMLFormControlElement, and HTMLFormControlElement::parseMappedAttribute() already had setNeedsStyleRecalc()."

So whenever HTMLFieldSetElement::setDisabled() is called, setNeedsStyleRecalc() is also correctly called, already.

Or do you mean other stuff?

Thanks,

[Naoki Takano]

Darin,

Could you give me your comment?

Thanks,

[Kent Tamura]

(In reply to comment #22)
Takano-san,  you already have enough information.  I recommend starting the implementation of Darin's algorithm with good tests rather than discussing.

[Naoki Takano]

Ok,

Still I'm worried about some speed problem that DOM manipulation, but I start implementation.

Thanks,

(In reply to comment #23)
> (In reply to comment #22)
> Takano-san,  you already have enough information.  I recommend starting the implementation of Darin's algorithm with good tests rather than discussing.

[Darin Adler]

I may have been wrong about style recalculation. I didn’t realize that a change to the enabled state of any element in a document causes the entire element’s style to be recalculated.

As far as performance is concerned, I am indeed concerned that keeping this up to date when manipulating the DOM could be expensive. But I believe this is true of any design. If we cache a pointer to a fieldset in a form element, that cache needs to be invalidated when DOM mutation occurs.

I’m sorry I can’t give more precise direction than that. There’s nothing fundamentally wrong with putting a fieldset pointer in the rare data of every form element if that really is what it takes to implement this feature.

[Naoki Takano]

Darin,

Thank you for your comment.

Now I start implementation according to your suggestion.

I let you know once I;m done.

Thanks,

(In reply to comment #25)
> I may have been wrong about style recalculation. I didn’t realize that a change to the enabled state of any element in a document causes the entire element’s style to be recalculated.
> 
> As far as performance is concerned, I am indeed concerned that keeping this up to date when manipulating the DOM could be expensive. But I believe this is true of any design. If we cache a pointer to a fieldset in a form element, that cache needs to be invalidated when DOM mutation occurs.
> 
> I’m sorry I can’t give more precise direction than that. There’s nothing fundamentally wrong with putting a fieldset pointer in the rare data of every form element if that really is what it takes to implement this feature.

[Naoki Takano]

Created attachment 92195 [details]
Patch

[Naoki Takano]

Comment on attachment 92195 [details]
Patch

I rewrite the patch.

The current test time is 1.9sec.
It is almost same as the last result.

[Darin Adler]

Comment on attachment 92195 [details]
Patch

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

> Source/WebCore/html/HTMLFieldSetElement.cpp:76
> +    // Traverse whole children to find HTMLFormControlElement is added or not.

This code only gets called when an immediate child is changed. What about when a descendant further down the DOM tree is changed?

[Naoki Takano]

I thought childrenChanged() is called whenever any descendant children is changed.

Or is there any other even handler to be called?

Thanks,

(In reply to comment #29)
> (From update of attachment 92195 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92195&action=review
> 
> > Source/WebCore/html/HTMLFieldSetElement.cpp:76
> > +    // Traverse whole children to find HTMLFormControlElement is added or not.
> 
> This code only gets called when an immediate child is changed. What about when a descendant further down the DOM tree is changed?

[Darin Adler]

(In reply to comment #30)
> I thought childrenChanged() is called whenever any descendant children is changed.

I don’t think it is. It would be bad for performance to have these operations be O(depth).

> Or is there any other event handler to be called?

I don’t think there is one for the reason above. That’s part of what makes this so tricky. Perhaps we should look more at how form elements moved into forms associate themselves with forms.

[Naoki Takano]

(In reply to comment #31)
> (In reply to comment #30)
> > I thought childrenChanged() is called whenever any descendant children is changed.
> 
> I don’t think it is. It would be bad for performance to have these operations be O(depth).
Yeah, that's why I'm worried...

> > Or is there any other event handler to be called?
> 
> I don’t think there is one for the reason above. That’s part of what makes this so tricky. Perhaps we should look more at how form elements moved into forms associate themselves with forms.
Hmmm...
This item is more tricky for me than I thought...

Ok, I leave it for a while. Once I get more knowledge I'll try again.

Thank you for taking your time!!

[Kent Tamura]

Comment on attachment 92195 [details]
Patch

r- because of a performance issue.

[Kent Tamura]

*** Bug 64395 has been marked as a duplicate of this bug. ***

[Zeno Albisser]

i'll try to address the performance issue and the changes further down in the DOM tree and hopefully upload a revised patch soon.

[Zeno Albisser]

Created attachment 125164 [details]
patch for review / feedback.

I changed the implementation to register FormControlElements to ancestor FieldSetElements if available.
So instead of traversing the whole FieldSet subtree after a change we can now simply register FormControlElements once and update them directly on a change. With the provided unit test i could not measure any significant performance impact.
But of course this implementation has it's weak spots as well. For example, when we have many (hundreds) of edges between a FormControlElement and its FieldSet ancestor, and this constellation many times again.

Any thoughts/comments?

[Kent Tamura]

Comment on attachment 125164 [details]
patch for review / feedback.

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


> LayoutTests/fast/forms/fieldset-disabled.html:10
> +<script src="script-tests/fieldset-disabled.js"></script>

Do not separate the .js file from the HTML file.

> LayoutTests/fast/forms/script-tests/fieldset-disabled.js:10
> +// Enabled by default, user can insertText into the text input field.

Such test information should be shown in the test result.
  debug('Fieldset is enabled by default. A user can insert text into the text input field:');

> LayoutTests/fast/forms/script-tests/fieldset-disabled.js:44
> +// Disable fieldSet.
> +fieldSet.disabled = false;

Disable -> Enable

> LayoutTests/fast/forms/script-tests/fieldset-disabled.js:91
> +// Add 3000 text input for performance test.
> +for (var i = 0; i < 3000; i++) {
> +  var newTextInput = document.createElement('input');
> +  newTextInput.type = "text";
> +  td.appendChild(newTextInput);
> +}

You shouldn't add performance test here.  If you need to add it, it should be in LayoutTests/perf or PerformanceTests/

> Source/WebCore/html/HTMLFieldSetElement.cpp:67
> +void HTMLFieldSetElement::attributeChanged(Attribute* attribute, bool preserveDecls)
> +{
> +    HTMLFormControlElement::attributeChanged(attribute, preserveDecls);
> +    if (attribute->name() == disabledAttr) {
> +        for (size_t i = 0; i < m_subordinatedFormControlElements.size(); i++) {
> +            HTMLFormControlElement* control = m_subordinatedFormControlElements.at(i);
> +            control->setParentDisabled(disabled());
> +        }
> +    }
> +}

We usually use parseMappedAttribute() instead of attributeChanged().

We need to call setNeedsStyleRecalc() for each of controls to apply :disabled/:enabled style.

> Source/WebCore/html/HTMLFieldSetElement.h:48
> +    Vector<HTMLFormControlElement*> m_subordinatedFormControlElements;

I don't think we need to keep track of descendant form controls.

We need to call setNeedsStyleRecalc() for descendant form controls when disabled attribute is change. I think it's ok to traverse all of descendant nodes and we don't need to care about the performance of updating HTMLFieldSetElement::disabled at this moment because this cost is not a regression.

> Source/WebCore/html/HTMLFormControlElement.cpp:107
> +    HTMLFieldSetElement* fieldSet = findFieldSetAncestor();

I'm worry about the performance of this.  findFieldSetAncestor() is O(depth).  This will be a performance regression even if a document has no <fieldset> elements.

> Source/WebCore/html/HTMLFormControlElement.h:66
> +    virtual bool disabled() const { return m_disabled || m_parentDisabled; }

Because we have HTMLFormControlElement::m_fieldSetAncestor, we don't need to have m_parentDisabled at all.  return m_disabled || (m_fieldSetAncestor && m_fieldSetAncestor->disabled()) is enough.

[Kent Tamura]

Comment on attachment 125164 [details]
patch for review / feedback.

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

> LayoutTests/fast/forms/script-tests/fieldset-disabled.js:92
> +

We need a test case for nested <fieldset>s.

<fieldset disable>
 <fieldset>
  <input ...>
 </fieldset>
</fieldset>

[Zeno Albisser]

Comment on attachment 125164 [details]
patch for review / feedback.

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

Thank you very much for reviewing! :-)
I'll upload the fixed version right away.

>> LayoutTests/fast/forms/fieldset-disabled.html:10
>> +<script src="script-tests/fieldset-disabled.js"></script>
> 
> Do not separate the .js file from the HTML file.

fixed.

>> LayoutTests/fast/forms/script-tests/fieldset-disabled.js:10
>> +// Enabled by default, user can insertText into the text input field.
> 
> Such test information should be shown in the test result.
>   debug('Fieldset is enabled by default. A user can insert text into the text input field:');

okay.. i'll change that.

>> LayoutTests/fast/forms/script-tests/fieldset-disabled.js:44
>> +fieldSet.disabled = false;
> 
> Disable -> Enable

fixed.

>> LayoutTests/fast/forms/script-tests/fieldset-disabled.js:91
>> +}
> 
> You shouldn't add performance test here.  If you need to add it, it should be in LayoutTests/perf or PerformanceTests/

I don't think this kind of performance test makes much sense anyway. - removed it.

>> LayoutTests/fast/forms/script-tests/fieldset-disabled.js:92
>> +
> 
> We need a test case for nested <fieldset>s.
> 
> <fieldset disable>
>  <fieldset>
>   <input ...>
>  </fieldset>
> </fieldset>

added.

>> Source/WebCore/html/HTMLFieldSetElement.cpp:67
>> +}
> 
> We usually use parseMappedAttribute() instead of attributeChanged().
> 
> We need to call setNeedsStyleRecalc() for each of controls to apply :disabled/:enabled style.

fixed.

>> Source/WebCore/html/HTMLFieldSetElement.h:48
>> +    Vector<HTMLFormControlElement*> m_subordinatedFormControlElements;
> 
> I don't think we need to keep track of descendant form controls.
> 
> We need to call setNeedsStyleRecalc() for descendant form controls when disabled attribute is change. I think it's ok to traverse all of descendant nodes and we don't need to care about the performance of updating HTMLFieldSetElement::disabled at this moment because this cost is not a regression.

I changed that as you proposed.

>> Source/WebCore/html/HTMLFormControlElement.cpp:107
>> +    HTMLFieldSetElement* fieldSet = findFieldSetAncestor();
> 
> I'm worry about the performance of this.  findFieldSetAncestor() is O(depth).  This will be a performance regression even if a document has no <fieldset> elements.

I am aware of this issue. But I don't think there is a much better solution for this. Either it is complex or it is memory expensive.
I would suggest to merge this function with HTMLElement::findFormAncestor(). Whenever we execute findFormAncestor we will come by any existing fieldset ancestor anyway.
So we would get it for "free". But i did not change this yet, we would need to change the naming/implementation of HTMLElement::findFormAncestor() const.
I think this requires a separate bug report & patch.

>> Source/WebCore/html/HTMLFormControlElement.h:66
>> +    virtual bool disabled() const { return m_disabled || m_parentDisabled; }
> 
> Because we have HTMLFormControlElement::m_fieldSetAncestor, we don't need to have m_parentDisabled at all.  return m_disabled || (m_fieldSetAncestor && m_fieldSetAncestor->disabled()) is enough.

Good point. - thx :)
I'll change that accordingly.

[Zeno Albisser]

Created attachment 125343 [details]
patch for review.

fixed as commented before.

[Kent Tamura]

Comment on attachment 125343 [details]
patch for review.

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

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

We should have more tests:
 - 'disabled' IDL attribute should reflect 'disabled' HTML attribute regardless of enclosing fieldsets
 - <input> is not disabled in a case of <fieldset disabled><legend><input></legend></fieldset>
 - Changing disabled state of a <fieldset> correctly updates the style of descendant form controls.
  You can use document.defaultView.getComputedStyle(element, null).getProeprtyeValue() or docment.querySelector()/querySelectorAll().

> Source/WebCore/html/FormAssociatedElement.cpp:97
> +    if (element->isFormControlElement())
> +        static_cast<HTMLFormControlElement*>(element)->updateFieldSetAncestor();

The code should be in HTMLFormControlElement::insertedIntoTree().

> Source/WebCore/html/HTMLFieldSetElement.cpp:63
> +    if (attr->name() == disabledAttr) {

We should do this expensive operation only if disabled state is changed.
We had better introduce HTMLFormControlElement::disabledAttributeChanged() virtual function like requiredAttributeChanged().

> Source/WebCore/html/HTMLFieldSetElement.cpp:80
> +        Vector<Node*> nodesToUpdate;
> +        nodesToUpdate.append(this);
> +        while (!nodesToUpdate.isEmpty()) {
> +            // Take the last node from the vector and mark it for style recalculation.
> +            Node* currentNode = nodesToUpdate.last();
> +            nodesToUpdate.removeLast();
> +            HTMLElement* element = toHTMLElement(currentNode);
> +            if (element && element->isFormControlElement())
> +                static_cast<HTMLFormControlElement*>(element)->setNeedsStyleRecalc();
> +
> +            // Append the children of currentNode to the vector of nodes that need to be updated.
> +            currentNode = currentNode->firstChild();
> +            while (currentNode) {
> +                nodesToUpdate.append(currentNode);
> +                currentNode = currentNode->nextSibling();
> +            }
> +        }

Node::traverseNextNode(this) is helpful.

> Source/WebCore/html/HTMLFieldSetElement.h:42
> +    virtual void parseMappedAttribute(Attribute*);

You should append 'OVERRIDE'.

> Source/WebCore/html/HTMLFieldSetElement.idl:27
> +        attribute [Reflect] boolean disabled;

Please insert this line at the beginning. We'd like to follow the IDL in the HTML specification as possible.
http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#the-fieldset-element

[Kent Tamura]

(In reply to comment #39)
> >> Source/WebCore/html/HTMLFormControlElement.cpp:107
> >> +    HTMLFieldSetElement* fieldSet = findFieldSetAncestor();
> > 
> > I'm worry about the performance of this.  findFieldSetAncestor() is O(depth).  This will be a performance regression even if a document has no <fieldset> elements.
> 
> I am aware of this issue. But I don't think there is a much better solution for this. Either it is complex or it is memory expensive.
> I would suggest to merge this function with HTMLElement::findFormAncestor(). Whenever we execute findFormAncestor we will come by any existing fieldset ancestor anyway.

It's a nice idea. Would you try it in this bug please?
I don't think we should commit a patch which has a known performance regression.

[Zeno Albisser]

Comment on attachment 125343 [details]
patch for review.

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

As mentioned inline, i changed
HTMLFormElement* HTMLElement::findFormAncestor() const
to
HTMLFormElement* HTMLElement::updateFormAncestors().
This seems to be the least intrusive way of achieving to update disablingAncestor together with formAncestor.
Unfortunately it requires a nasty const_cast in HTMLElement::virtualForm().
Better ideas / opinions on that are very welcome!

>> LayoutTests/fast/forms/fieldset-disabled.html:163
>> +</html>
> 
> We should have more tests:
>  - 'disabled' IDL attribute should reflect 'disabled' HTML attribute regardless of enclosing fieldsets
>  - <input> is not disabled in a case of <fieldset disabled><legend><input></legend></fieldset>
>  - Changing disabled state of a <fieldset> correctly updates the style of descendant form controls.
>   You can use document.defaultView.getComputedStyle(element, null).getProeprtyeValue() or docment.querySelector()/querySelectorAll().

- added a test for verifying the behavior regardless of enclosing fieldset.
- added implementation + tests for overriding inherited disabled state in case of the first legend element.
- added a test to check that the style of a form control is updated and reflects the disabled state.
- added a test to verify the behavior in case a new legend element is inserted at position 0 in the child liset.

>> Source/WebCore/html/FormAssociatedElement.cpp:97
>> +        static_cast<HTMLFormControlElement*>(element)->updateFieldSetAncestor();
> 
> The code should be in HTMLFormControlElement::insertedIntoTree().

removed - We don't need that anymore, because we now update the fieldSetAncestor (disablingAncestor) when executing findFormAncestor (updateFormAncestors).
What was a fieldSetAncestor before has now been renamed to diablingAncestor, because it is the ancestor that propagates its disabled state to its children.
I also renamed findFormAncestor to updateFormAncestors. This should reflect that it implicitly updates the disablingAncestor. Updating the disablingAncestor implicitly allows us, to traverse the tree only once for finding both the formAncestor and the disablingAncestor.
If there is a more beautiful solution for that, that is as performant as this one... i am all up for it!

>> Source/WebCore/html/HTMLFieldSetElement.cpp:63
>> +    if (attr->name() == disabledAttr) {
> 
> We should do this expensive operation only if disabled state is changed.
> We had better introduce HTMLFormControlElement::disabledAttributeChanged() virtual function like requiredAttributeChanged().

done.

>> Source/WebCore/html/HTMLFieldSetElement.cpp:80
>> +        }
> 
> Node::traverseNextNode(this) is helpful.

good hint! - thank you. :)

>> Source/WebCore/html/HTMLFieldSetElement.h:42
>> +    virtual void parseMappedAttribute(Attribute*);
> 
> You should append 'OVERRIDE'.

This function is not needed anymore, because of the disabledAttributeChanged() function being introduced. - I marked disabledAttributeChanged() OVERRIDE, where applicable.

>> Source/WebCore/html/HTMLFieldSetElement.idl:27
>> +        attribute [Reflect] boolean disabled;
> 
> Please insert this line at the beginning. We'd like to follow the IDL in the HTML specification as possible.
> http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#the-fieldset-element

fixed.

[Zeno Albisser]

Created attachment 126520 [details]
patch for review. - fixed issues as commented before.

[Kent Tamura]

Comment on attachment 126520 [details]
patch for review. - fixed issues as commented before.

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(disablingAncestor);

This function is called with non-HTMLFormControlElement "this".  So the cast is wrong.
http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/js/JSHTMLElementCustom.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#selector-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.

[Kent Tamura]

(In reply to comment #45)
> - 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><fieldset disabled><input></fiedset></form>

[Zeno Albisser]

Comment on attachment 126520 [details]
patch for review. - fixed issues as commented before.

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

I'll upload a new patch with the changes as commented asap.

>> 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>

Added a test for the parser.

>> Source/WebCore/html/HTMLElement.cpp:876
>> +    static_cast<HTMLFormControlElement*>(this)->setDisablingAncestor(disablingAncestor);
> 
> This function is called with non-HTMLFormControlElement "this".  So the cast is wrong.
> http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/js/JSHTMLElementCustom.cpp#L59
> 
> I recommend introducing new function member to HTMLFormControlElement, instead of updating HTMLElement:findFormAncestor().

I reverted findFormAncestor() to its original state.
Further i added a function updateFormAndFieldSetAncestor() as well as a function updateFieldSetAncestor().
The latter is being used when we get the information about the form already from the parser.
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.

>> Source/WebCore/html/HTMLFieldSetElement.cpp:49
>> +            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.

I added an HTMLFieldSetElement* member to HTMLFormControlElement.
Instead of adding "bool m_isInsideLegend", i further added an HTMLLegendElement* member to HTMLFormControlElement.
Because just having "bool m_isInsideLegend" does not help to figure out if the first legend in the parent fieldset is also an ancestor of the HTMLFormControlElement.

>> 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#selector-enabled

I removed this part.

>> Source/WebCore/html/HTMLFormControlElement.cpp:66
>>      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.

This has been addressed with the newly introduced updateFieldSetAncestor() function.

[Zeno Albisser]

Created attachment 127809 [details]
patch for review. - fixed issues as commented before.

[Zeno Albisser]

Comment on attachment 127809 [details]
patch for review. - fixed issues as commented before.

sorry - just realized i forgot to delete HTMLLegendElement::disabled().

[Zeno Albisser]

Created attachment 127810 [details]
patch for review. - removed obsolete HTMLLegendElement::fieldset() as well.

[WebKit Review Bot]

Comment on attachment 127810 [details]
patch for review. - removed obsolete HTMLLegendElement::fieldset() as well.

Attachment 127810 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11549253

New failing tests:
fast/parser/form-pointer-4.html
fast/forms/form-attribute-elements-order.html
fast/forms/form-attribute.html
fast/forms/form-attribute-nonexistence-form-id.html
fast/forms/form-attribute-elements-order2.html
fast/forms/radio-remove-form-attr.html

[Kent Tamura]

Comment on attachment 127810 [details]
patch for review. - removed obsolete HTMLLegendElement::fieldset() as well.

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.

[Kent Tamura]

Adam, Eric,
We have a question about the HTML parser.

The HTMLFormControlElement constructor needs to know its <fieldset> and <legend> ancestor, and I'm afraid adding ancestor lookup code will make a performance regression. Is it reasonable to change the HTML parser so that it keeps track of <fieldset> and <legend>, and passes them to the HTMLFormControlElement constructor?


> > 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.

[Eric Seidel (no email)]

(In reply to comment #53)
> Adam, Eric,
> We have a question about the HTML parser.
> 
> The HTMLFormControlElement constructor needs to know its <fieldset> and <legend> ancestor, and I'm afraid adding ancestor lookup code will make a performance regression. Is it reasonable to change the HTML parser so that it keeps track of <fieldset> and <legend>, and passes them to the HTMLFormControlElement constructor?

From my limited understanding of this bug, this doesn't sound like what you want.  It seems you'd rather have a cache for these values which is updated on insertedIntoDocument and removedFromDocument(), no?

I'm not entirely sure why walking your parent chain for these ancestor elements is bad?  How commonly do we need to access this information?  During style resolve?  If so, how often during style resolve?  If we cache this information about our parent chain, when do we invalidate those caches?

if I were writing this, I would write the simple solution which always walked the parent chain for these pointers, and then once I had that working and I had a performance test which demonstrated it was too slow, I would then add caching.

But it's possible I'm coming late to the party and we've already decided your proposal is the proper approach?

[Kent Tamura]

(In reply to comment #54)
> From my limited understanding of this bug, this doesn't sound like what you want.  It seems you'd rather have a cache for these values which is updated on insertedIntoDocument and removedFromDocument(), no?
> 
> I'm not entirely sure why walking your parent chain for these ancestor elements is bad?  How commonly do we need to access this information?  During style resolve?  If so, how often during style resolve?  If we cache this information about our parent chain, when do we invalidate those caches?
> 
> if I were writing this, I would write the simple solution which always walked the parent chain for these pointers, and then once I had that working and I had a performance test which demonstrated it was too slow, I would then add caching.
> 
> But it's possible I'm coming late to the party and we've already decided your proposal is the proper approach?

I might be too afraid of the ancestor lookup cost.

ok, we should do:
 - adding a bool data member to HTMLFormControlElement to represent validity of m_fieldSetAncestor and m_legendAncestor
 - m_fieldSetAncesotr and m_legendAncestor are updated in HTMLFormControlElement::disabled() if the bool data member indicates they are invalid.  They are not updated in other places.
 - removedFromTree() changes the bool data member invalid state.

 - Land the patch, and watch the performance bots.

[Eric Seidel (no email)]

I'm not sure "removedFromTree" is enough?  Is that called when an element is moved within the tree?  I would start w/o caches and try and write a performance test which shows that your cache-less solution is slow.

[Kent Tamura]

*** Bug 80105 has been marked as a duplicate of this bug. ***

[Zeno Albisser]

Created attachment 133832 [details]
patch for review. - updated as requested.

[Zeno Albisser]

Comment on attachment 133832 [details]
patch for review. - updated as requested.

thanks for reviewing! :-)

Comment on attachment 133832 [details]
patch for review. - updated as requested.

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

> Source/WebCore/html/HTMLFormControlElement.cpp:231
> +    m_fieldSetAncestorValid = false;

We also want to set m_fieldSetAncestorValid to false in insertedIntoTree.

> Source/WebCore/html/HTMLFormControlElement.cpp:289
> +    if (!m_fieldSetAncestor || !m_fieldSetAncestor->disabled())

nit: Negative condition is sometimes hard to read. Could you rewrite someething like below?
// Form controls in the first legend elements aren't affected by fieldset.disabled.
if (m_fieldSetAncestor && m_fileAncestor->disabled())
  reutrn !(m_legendAncestor && m_legendAncestor == m_fieldSetAncesotr->legend());
return false;

(In reply to comment #60)
> (From update of attachment 133832 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133832&action=review
> 
> > Source/WebCore/html/HTMLFormControlElement.cpp:231
> > +    m_fieldSetAncestorValid = false;
> 
> We also want to set m_fieldSetAncestorValid to false in insertedIntoTree.

Sorry, upon insertedIntoTree. m_fieldSetAncestorValid is always false.
Please ignore this comment. Sorry for confusion.

Comment on attachment 133832 [details]
patch for review. - updated as requested.

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

> LayoutTests/fast/forms/fieldset-disabled.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Could you also check IDL attribute "disabled" in this test?

> LayoutTests/fast/forms/fieldset-disabled.html:39
> +var fieldSet = 0;

nit: I think using global variable declarations here is confusing and hard to maintain, even if we know they are for "shouldBe" and variations.
We may forget to add "var foo", when we add new "shouldBe('foo', 'true')".

nit: We may not need to have "= 0". "var fieldSet" statement sets "undefined" to varaible "fieldSet".

> LayoutTests/fast/forms/fieldset-disabled.html:62
> +    shouldBeFalse(fieldSet.disabled.toString());

Could you use shouldBeFalse('fieldSet.disabled') for better message in output?

> LayoutTests/fast/forms/fieldset-disabled.html:76
> +    shouldBe('textInput.value', '"A"');

Please check textInput.disabled is true too. This comment is applied for others disable state changes.
Personally, I think checking IDL attribute "disabled" and style change are enough for this patch.

> LayoutTests/fast/forms/fieldset-disabled.html:253
> +    var parserGeneratedInput1 = document.getElementById("parserGeneratedInput1");

nit: Can we encode expected value into HTML document and use loop?

<fieldset>
<input class="parserGeneratedInput" value="enabled">
</fieldset>
<fieldset disabled>
<input class="parserGeneratedInput" value="disabled">
</fieldset>
...

var parserGeneratedInputs = document.getElementsByClass("parserGeneratedInput");
for (var i = 0; i < parserGeneratedInputs.length; i++) {
  shouldBe('parserGeneratedInputs[' + i + ']', parserGeneratedInput[i].value);
}

> LayoutTests/fast/forms/fieldset-disabled.html:254
> +    var parserGeneratedInput2 = document.getElementById("parserGeneratedInput2");

Local variable parseGeneratedInput[1-9] aren't used in shouldBe. It is better to call shouldBe in the toplevel instead of inside function. It is confusing to use same name for both global and local.

> Source/WebCore/html/HTMLFieldSetElement.cpp:95
> +    while ((node = node->traverseNextNode(this))) {

nit: We don't need to have parenthesis for node = node->traverseNextNode(this).

> Source/WebCore/html/HTMLFormControlElement.cpp:109
> +            m_legendAncestor = static_cast<HTMLLegendElement*>(ancestor);

Note: We can quit ancestor traversal before reaching to fieldset element if we use HTMLLegendElement::m_fieldSetAncestor here.
Although, it isn't enough reason to make m_fieldAncestor public from HTMLLegendElement.

> Source/WebCore/html/HTMLFormControlElement.cpp:110
> +        if (!m_fieldSetAncestor && ancestor->hasTagName(fieldsetTag)) {

nit: m_fieldSetAncestor is always 0 in this loop. So, we don't need to have !m_fieldSetAncestor.

> Source/WebCore/html/HTMLLegendElement.cpp:72
>  HTMLFormControlElement* HTMLLegendElement::associatedControl()

Note: Once this patch is landed. We can remove associatedControl method and rewrie accessKeyAction and focus methods to use m_fieldSetAncestor.

[Zeno Albisser]

Comment on attachment 133832 [details]
patch for review. - updated as requested.

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

>> LayoutTests/fast/forms/fieldset-disabled.html:253
>> +    var parserGeneratedInput1 = document.getElementById("parserGeneratedInput1");
> 
> nit: Can we encode expected value into HTML document and use loop?
> 
> <fieldset>
> <input class="parserGeneratedInput" value="enabled">
> </fieldset>
> <fieldset disabled>
> <input class="parserGeneratedInput" value="disabled">
> </fieldset>
> ...
> 
> var parserGeneratedInputs = document.getElementsByClass("parserGeneratedInput");
> for (var i = 0; i < parserGeneratedInputs.length; i++) {
>   shouldBe('parserGeneratedInputs[' + i + ']', parserGeneratedInput[i].value);
> }

I think we are not allowed to update the disabled IDL attribute of the children, when a parent fieldset is being disabled.
This would overwrite the disabled attribute of the FormControlElement children and therefore always enable or disable all the children of the fieldset, even when these are specifically defined to be disabled.

Consider the following situation:
<fieldset id=”myFieldSet” disabled>
<input id=”myInput1”>
<input id=”myInput2” disabled>
</fieldset>
...
var before1 = document.getElementById(“myInput1”).disabled
var before2 = document.getElementById(“myInput2”).disabled
...
document.getElementById(“myFieldSet”).disabled = false
var after1 = document.getElementById(“myInput1”).disabled
var after2 = document.getElementById(“myInput2”).disabled

before1 will be true (disabled).
before2 will be true (disabled).
after1 will be false (enabled).
after2 will be false (enabled). -> unexpected

I don't think this is the expected behavior.

The spec says:
4.10.4 "The disabled attribute, when specified, causes all the form control descendants of the fieldset element, excluding those that are descendants of the fieldset element's first legend element child, if any, to be disabled."
4.10.19.2 "A form control is disabled if its disabled attribute is set, or if it is a descendant of a fieldset element whose disabled attribute is set and is not a descendant of that fieldset element's first legend element child, if any. ... The disabled IDL attribute must reflect the disabled content attribute."

I interpret the second quote as follows: The disabled IDL attribute should always reflect the disabled state of the element itself (without inheriting disabled state from its parents).
This nevertheless leads to a mismatch of the optical representation and the IDL attributes available to JS.

Any thoughts or comments?

[Zeno Albisser]

Comment on attachment 133832 [details]
patch for review. - updated as requested.

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

>> LayoutTests/fast/forms/fieldset-disabled.html:1
>> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
> 
> Could you also check IDL attribute "disabled" in this test?

I added "shouldBeTrue/False" to wherever i adjust the disabled attribute.

>> LayoutTests/fast/forms/fieldset-disabled.html:39
>> +var fieldSet = 0;
> 
> nit: I think using global variable declarations here is confusing and hard to maintain, even if we know they are for "shouldBe" and variations.
> We may forget to add "var foo", when we add new "shouldBe('foo', 'true')".
> 
> nit: We may not need to have "= 0". "var fieldSet" statement sets "undefined" to varaible "fieldSet".

I got rid of the functions and put everything directly into the global scope. While i believe it makes the test a bit less readable i hope this is a more correct solution.

>> LayoutTests/fast/forms/fieldset-disabled.html:62
>> +    shouldBeFalse(fieldSet.disabled.toString());
> 
> Could you use shouldBeFalse('fieldSet.disabled') for better message in output?

Yes, that looks much better. :)

>> LayoutTests/fast/forms/fieldset-disabled.html:76
>> +    shouldBe('textInput.value', '"A"');
> 
> Please check textInput.disabled is true too. This comment is applied for others disable state changes.
> Personally, I think checking IDL attribute "disabled" and style change are enough for this patch.

Please see my previous comment about altering disabled attributes of children.

>> LayoutTests/fast/forms/fieldset-disabled.html:254
>> +    var parserGeneratedInput2 = document.getElementById("parserGeneratedInput2");
> 
> Local variable parseGeneratedInput[1-9] aren't used in shouldBe. It is better to call shouldBe in the toplevel instead of inside function. It is confusing to use same name for both global and local.

fixed.

>> Source/WebCore/html/HTMLFieldSetElement.cpp:95
>> +    while ((node = node->traverseNextNode(this))) {
> 
> nit: We don't need to have parenthesis for node = node->traverseNextNode(this).

These are necessary for silencing a compiler warning in clang.

>> Source/WebCore/html/HTMLFormControlElement.cpp:109
>> +            m_legendAncestor = static_cast<HTMLLegendElement*>(ancestor);
> 
> Note: We can quit ancestor traversal before reaching to fieldset element if we use HTMLLegendElement::m_fieldSetAncestor here.
> Although, it isn't enough reason to make m_fieldAncestor public from HTMLLegendElement.

We could do that for optimization. - But i leave it out for now.

>> Source/WebCore/html/HTMLFormControlElement.cpp:110
>> +        if (!m_fieldSetAncestor && ancestor->hasTagName(fieldsetTag)) {
> 
> nit: m_fieldSetAncestor is always 0 in this loop. So, we don't need to have !m_fieldSetAncestor.

fixed.

>> Source/WebCore/html/HTMLFormControlElement.cpp:289
>> +    if (!m_fieldSetAncestor || !m_fieldSetAncestor->disabled())
> 
> nit: Negative condition is sometimes hard to read. Could you rewrite someething like below?
> // Form controls in the first legend elements aren't affected by fieldset.disabled.
> if (m_fieldSetAncestor && m_fileAncestor->disabled())
>   reutrn !(m_legendAncestor && m_legendAncestor == m_fieldSetAncesotr->legend());
> return false;

done.

[Zeno Albisser]

Created attachment 134139 [details]
patch for review. - fixed issues as commented before.

(In reply to comment #63)
> (From update of attachment 133832 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133832&action=review
> 
> >> LayoutTests/fast/forms/fieldset-disabled.html:253
> >> +    var parserGeneratedInput1 = document.getElementById("parserGeneratedInput1");
> > 
> > nit: Can we encode expected value into HTML document and use loop?
> > 
> > <fieldset>
> > <input class="parserGeneratedInput" value="enabled">
> > </fieldset>
> > <fieldset disabled>
> > <input class="parserGeneratedInput" value="disabled">
> > </fieldset>
> > ...
> > 
> > var parserGeneratedInputs = document.getElementsByClass("parserGeneratedInput");
> > for (var i = 0; i < parserGeneratedInputs.length; i++) {
> >   shouldBe('parserGeneratedInputs[' + i + ']', parserGeneratedInput[i].value);
> > }
> 
> I think we are not allowed to update the disabled IDL attribute of the children, when a parent fieldset is being disabled.
> This would overwrite the disabled attribute of the FormControlElement children and therefore always enable or disable all the children of the fieldset, even when these are specifically defined to be disabled.
> 
> Consider the following situation:
> <fieldset id=”myFieldSet” disabled>
> <input id=”myInput1”>
> <input id=”myInput2” disabled>
> </fieldset>
> ...
> var before1 = document.getElementById(“myInput1”).disabled
> var before2 = document.getElementById(“myInput2”).disabled
> ...
> document.getElementById(“myFieldSet”).disabled = false
> var after1 = document.getElementById(“myInput1”).disabled
> var after2 = document.getElementById(“myInput2”).disabled
> 
> before1 will be true (disabled).
> before2 will be true (disabled).
> after1 will be false (enabled).
> after2 will be false (enabled). -> unexpected
> 
> I don't think this is the expected behavior.
> 
> The spec says:
> 4.10.4 "The disabled attribute, when specified, causes all the form control descendants of the fieldset element, excluding those that are descendants of the fieldset element's first legend element child, if any, to be disabled."
> 4.10.19.2 "A form control is disabled if its disabled attribute is set, or if it is a descendant of a fieldset element whose disabled attribute is set and is not a descendant of that fieldset element's first legend element child, if any. ... The disabled IDL attribute must reflect the disabled content attribute."
> 
> I interpret the second quote as follows: The disabled IDL attribute should always reflect the disabled state of the element itself (without inheriting disabled state from its parents).
> This nevertheless leads to a mismatch of the optical representation and the IDL attributes available to JS.
> 
> Any thoughts or comments?

You're right. IDL "disabled" attribute is "reflect" of content attribute. We can't use it for inherited disabled value.
Although, encoding expectation into HTML can work with document.execCommand. It can reduce repeated code such as "var parseGeneratedINput1 = ..." and makes test script easier to maintain.

[Kent Tamura]

Comment on attachment 134139 [details]
patch for review. - fixed issues as commented before.

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

> Source/WebCore/html/HTMLFieldSetElement.cpp:80
> +HTMLLegendElement* HTMLFieldSetElement::legend() const
> +{
> +    return m_legend;
> +}

This is called only if a form control is put in a <legend>.  It should be very rare case, and it should be acceptable to find the <legend> on demand.

So, I propose
 - Remove HTMLFieldSetElement::m_legend
 - HTMLFieldSetElement::legend() finds the first-child <legend> on demand.
 - Revert every changes of HTMLLegendElement.{cpp,h}

> Source/WebCore/html/HTMLFieldSetElement.cpp:95
> +    while ((node = node->traverseNextNode(this))) {

This is wrong.  We should search only direct child nodes for <legend> according to the specification.
   for (Element* node = firstEleemntChild(); node; node = node->nextElementSibling()) { ...

[Kent Tamura]

Comment on attachment 134139 [details]
patch for review. - fixed issues as commented before.

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

> LayoutTests/ChangeLog:11
> +        * fast/forms/fieldset-disabled-expected.txt: Added.
> +        * fast/forms/fieldset-disabled.html: Added.

We had better move the test to fast/forms/fieldset/.

[Zeno Albisser]

Comment on attachment 134139 [details]
patch for review. - fixed issues as commented before.

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

>> LayoutTests/ChangeLog:11
>> +        * fast/forms/fieldset-disabled.html: Added.
> 
> We had better move the test to fast/forms/fieldset/.

moved.

>> Source/WebCore/html/HTMLFieldSetElement.cpp:80
>> +}
> 
> This is called only if a form control is put in a <legend>.  It should be very rare case, and it should be acceptable to find the <legend> on demand.
> 
> So, I propose
>  - Remove HTMLFieldSetElement::m_legend
>  - HTMLFieldSetElement::legend() finds the first-child <legend> on demand.
>  - Revert every changes of HTMLLegendElement.{cpp,h}

changed as requested.

>> Source/WebCore/html/HTMLFieldSetElement.cpp:95
>> +    while ((node = node->traverseNextNode(this))) {
> 
> This is wrong.  We should search only direct child nodes for <legend> according to the specification.
>    for (Element* node = firstEleemntChild(); node; node = node->nextElementSibling()) { ...

fixed.

[Zeno Albisser]

Created attachment 134287 [details]
patch for review. - fixed issues as commented before.

[Kent Tamura]

Comment on attachment 134287 [details]
patch for review. - fixed issues as commented before.

Looks good.  Thank you for the hard work!

[Zeno Albisser]

(In reply to comment #71)
> (From update of attachment 134287 [details])
> Looks good.  Thank you for the hard work!

Thanks for reviewing! :)

[Zeno Albisser]

Comment on attachment 134287 [details]
patch for review. - fixed issues as commented before.

Clearing flags on attachment: 134287

Committed r112515: <http://trac.webkit.org/changeset/112515>

[Zeno Albisser]

All reviewed patches have been landed.  Closing bug.