Closed Bug 557087 Opened 14 years ago Closed 14 years ago

Implement the fieldset element disabled attribute

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b7

People

(Reporter: mounir, Assigned: mounir)

References

(Depends on 1 open bug, )

Details

(Keywords: dev-doc-complete, html5)

Attachments

(7 files, 7 obsolete files)

20.21 KB, patch
dbaron
: review+
sicking
: approval2.0+
Details | Diff | Splinter Review
52.85 KB, patch
roc
: review+
sicking
: approval2.0+
Details | Diff | Splinter Review
1.76 KB, patch
dbaron
: review+
sicking
: approval2.0+
Details | Diff | Splinter Review
15.70 KB, patch
sicking
: review+
sicking
: approval2.0+
Details | Diff | Splinter Review
1.61 KB, patch
sicking
: review+
Details | Diff | Splinter Review
139.01 KB, patch
sicking
: review+
sicking
: approval2.0+
Details | Diff | Splinter Review
107.37 KB, patch
sicking
: review+
sicking
: approval2.0+
Details | Diff | Splinter Review
"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."
FWIW, I think this would actually be a quite useful feature
I agree but I'm not sure that could make it for beta5. A quick look at the specifications let me think some changes are needed.
For example:
<fieldset disabled>
  <input>
</fieldset>
What should return input.disabled? 'true' seems to be the best answer but the specifications says 'false' (reflects the content attribute).

Or:
<fieldset disabled>
  <input>
</fieldset>
<script>
  input.disabled = false;
</script>
Should input be enabled? or should <fieldset> disable state override the input's one even if explicitly set? I guess that's what we want.

As-is, the feature isn't trivial (considering the two tests cases, that changes a lot of basic behaviors) and considering that the specifications does not sound mature on that, maybe we should wait?
By the way, I will bring these issues to whatwg tomorrow.
Keywords: dev-doc-needed
I tried to found all situations where being disabled change something (according to the specifications) and only two applied to fieldset:
- prevent click events
- prevent being focused
which have been moved to "no DOM events when disabled" (that's what we are doing for all elements so...)

I've also added :enabled/:disabled support for fieldset.

I don't know if we should change fieldset rendering when disabled. If we want to, I guess we can do that in a follow-up.
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #474445 - Flags: review?(jonas)
Better with some reftests ;)
Attachment #474445 - Attachment is obsolete: true
Attachment #474448 - Flags: review?(jonas)
Attachment #474445 - Flags: review?(jonas)
This is making the content aware the new disabled state rule (ie. the frames are not going to change if they are looking the attribute change in the content but :disabled/:enabled are working).

Some comments:
- OnFieldSetDisabledChanged is always calling ContentStatesChanged because there is no real way to know if we should notify. I could get the notify from AfterSetAttr call from nsHTMLFieldSetElement but that would be for the fieldset, not it's children ;
- IsDisabled is defined in nsGenericHTMLElement only because IsHTMLFocusable in nsGenericHTMLElement is always taking in account form elements (and I don't want to break to many things with this patch). I will open a follow up to split this method in two. Or maybe IsDisabled() should even move to nsIContent? Might be easier for frames I think.
- This is not following the rule "elements in the first legend child are not disabled". It is probably going to be part 4 or even a follow-up (maybe we could do that for beta7?).
Attachment #474813 - Flags: review?(jonas)
Tests will come in part 5.
Attachment #474899 - Flags: review?(dbaron)
(In reply to comment #7)
> Created attachment 474899 [details] [diff] [review]
> Part 3 - Make layout aware of the new disabled state rule
> 
> Tests will come in part 5.

Actually, there is a small bug: if <input type='file'> is inside a disabled fieldset, the button inside the nsFileControlFrame will not have it's text grayed because the frame informs the button it is invalid when the attribute changes.
However, we can't actually open the file picker because the frame ignores the events. So, if by any chance this can be reviewed for beta6, I would vote to fix that in a follow-up.
This will be tested with part 3 in a following patch.

Roc, this patch seems big but it's pretty simple. It is basically removing "IsDisabled" method and use the content states to know if the content is disabled.
Attachment #474906 - Flags: review?(roc)
Attachment #474906 - Attachment description: Part 3 - Make widget aware of the new disabled state rule → Part 4 - Make widget aware of the new disabled state rule
Depends on: 596088
Dynamic rendering has a bug for <input type='file'>, see bug 596088.

I ask the review to dbaron but I guess anyone can review that...
Attachment #474916 - Flags: review?(dbaron)
Comment on attachment 474899 [details] [diff] [review]
Part 3 - Make layout aware of the new disabled state rule

>+  if (eventStates & NS_EVENT_STATE_DISABLED || mFocused != this)

>+  if (eventStates & NS_EVENT_STATE_DISABLED && IsVisibleForPainting(aBuilder)) {

Even though these are correct, I'd prefer if you parenthesize the bitwise &, since I prefer it always parenthesized because its operator precedence is in the wrong place in C/C++.

r=dbaron
Attachment #474899 - Flags: review?(dbaron) → review+
Oups, i attached part4 instead of part5. Sorry David :(
Attachment #474916 - Attachment is obsolete: true
Attachment #474932 - Flags: review?(dbaron)
Attachment #474916 - Flags: review?(dbaron)
Comment on attachment 474932 [details] [diff] [review]
Part 5 - Test the rendering when inside a disabled fieldset

r=dbaron
Attachment #474932 - Attachment description: Part 5 - Test the rendrenig when inside a disabled fieldset → Part 5 - Test the rendering when inside a disabled fieldset
Attachment #474932 - Flags: review?(dbaron) → review+
Depends on: 596100
This patch works (as much as tested).

I still need to write more tests and make some optimizations if possible.
Updated to remove a reftest which was assuming that fieldset can't be disabled. I have reftests in this patch testing the exact opposite ;)
Attachment #474448 - Attachment is obsolete: true
Attachment #475182 - Flags: review?(jonas)
Attachment #474448 - Flags: review?(jonas)
Disabled should have no effect on <object>. We had a patch written to test focus and testing that object wasn't reacting to focus when disabled. This should change nothing.

Actually, I don't know why <object> reacts to focus by default. This is wrong.
Attachment #475183 - Flags: review?(jonas)
(In reply to comment #16)
> Created attachment 475183 [details] [diff] [review]
> Part 2.5 - Fixing a patch assuming disabled work on <object>
> 
> Disabled should have no effect on <object>. We had a patch written to test
> focus and testing that object wasn't reacting to focus when disabled. This
> should change nothing.
> 
> Actually, I don't know why <object> reacts to focus by default. This is wrong.

bug 596350 has been open for <object> focus.

FWIW, this patch is going to be folded with Part 2. I didn't update Part 2 to make it clearer that my patch is going to remove this weird behavior consisting of blocking focus on <object> when disabled is set even if disabled doesn't apply for <object>.
Attachment #475183 - Attachment description: Part 2.5 - Fixing a patch assuming disabled work on <object> → Part 2.5 - Fixing a test assuming disabled work on <object>
This code is quite simple. The size of the patch is mostly related to tests.
At some place in nsGenericHTMLElement.cpp, I'm calling static_cast twice. I could create a temp variable to the top class so I would only call static_cast once but I guess static_cast is cheap, right?
Attachment #475011 - Attachment is obsolete: true
Attachment #475431 - Flags: review?(jonas)
This feature seems dumb but the sum of the patches is 330k! :-/
Comment on attachment 474813 [details] [diff] [review]
Part 2 - Make the content aware of the new disabled state rule

Why does OnFieldSetDisabledChanged take a state bitfield? It seems like you could always just update the INVALID and VALID states, no?

>@@ -2521,16 +2531,19 @@ nsGenericHTMLFormElement::UnbindFromTree
> 
>   // We have to remove the form id observer if there was one.
>   // We will re-add one later if needed (during bind to tree).
>   if (nsContentUtils::HasNonEmptyAttr(this, kNameSpaceID_None,
>                                       nsGkAtoms::form)) {
>     RemoveFormIdObserver();
>   }
> 
>+  // The element has no fieldset parent anymore.
>+  mFieldSet = nsnull;

This does not appear to be correct. We can have a field ancestor. UnbindFromTree is called if a whole subtree is removed from its parent.


>+nsGenericHTMLFormElement::OnFieldSetDisabledChanged(PRInt32 aStates)
>+{
>+  aStates |= NS_EVENT_STATE_DISABLED | NS_EVENT_STATE_ENABLED;
>+
>+  nsIDocument* doc = GetCurrentDoc();
>+  // TODO: should we use aNotify ?!
>+  if (doc) {
>+    MOZ_AUTO_DOC_UPDATE(doc, UPDATE_CONTENT_STATE, PR_TRUE);
>+    doc->ContentStatesChanged(this, nsnull, aStates);
>+  }
>+}

Unfortunately I think the answer to your question is 'yes' :( I take it you can't completely avoid calling this function when aNotify is false since it also updates validity states?

Could you make IntrinsicState() look up the disabled state and that way avoid having to update the internal validity flags when the disabled state changes?


>diff --git a/content/html/content/src/nsGenericHTMLElement.h b/content/html/content/src/nsGenericHTMLElement.h
>+  /**
>+   * Returns the current disabled state of the element.
>+   */
>+  virtual bool IsDisabled() const {
>+    return HasAttr(kNameSpaceID_None, nsGkAtoms::disabled);
>+  }

Why does this need to be virtual? Can you make mFieldSet be a nsHTMLFieldSetElement pointer instead?


...still more to review
(In reply to comment #20)
> Comment on attachment 474813 [details] [diff] [review]
> Part 2 - Make the content aware of the new disabled state rule
> 
> Why does OnFieldSetDisabledChanged take a state bitfield? It seems like you
> could always just update the INVALID and VALID states, no?

Considering we need to updatee DISABLED and ENABLED states for all elements and INVALID/VALID for some, I thought calling ContentStatesChanged only once might be better... If the optimization is uselesse, I can remove that.

> >@@ -2521,16 +2531,19 @@ nsGenericHTMLFormElement::UnbindFromTree
> > 
> >   // We have to remove the form id observer if there was one.
> >   // We will re-add one later if needed (during bind to tree).
> >   if (nsContentUtils::HasNonEmptyAttr(this, kNameSpaceID_None,
> >                                       nsGkAtoms::form)) {
> >     RemoveFormIdObserver();
> >   }
> > 
> >+  // The element has no fieldset parent anymore.
> >+  mFieldSet = nsnull;
> 
> This does not appear to be correct. We can have a field ancestor.
> UnbindFromTree is called if a whole subtree is removed from its parent.

Indeed. I guess I should look at the parent chain then. Not very nice :-/

> >+nsGenericHTMLFormElement::OnFieldSetDisabledChanged(PRInt32 aStates)
> >+{
> >+  aStates |= NS_EVENT_STATE_DISABLED | NS_EVENT_STATE_ENABLED;
> >+
> >+  nsIDocument* doc = GetCurrentDoc();
> >+  // TODO: should we use aNotify ?!
> >+  if (doc) {
> >+    MOZ_AUTO_DOC_UPDATE(doc, UPDATE_CONTENT_STATE, PR_TRUE);
> >+    doc->ContentStatesChanged(this, nsnull, aStates);
> >+  }
> >+}
> 
> Unfortunately I think the answer to your question is 'yes' :( I take it you
> can't completely avoid calling this function when aNotify is false since it
> also updates validity states?

The think is if I have a aNotify, it would apply for the fieldset so it might not be correct to use it for the other elements.
I wouldn't have to avoid calling the function, I could just avoid notifying the content states change. 

> Could you make IntrinsicState() look up the disabled state and that way avoid
> having to update the internal validity flags when the disabled state changes?

I don't follow. IntrinsicState() is looking to the disabled state. This callback is used to update the rendering of the child (and the validity when needed). If the fieldset has it's disabled attribute changed, the child are not going to change their rendering if the state change isn't notified.

> >diff --git a/content/html/content/src/nsGenericHTMLElement.h b/content/html/content/src/nsGenericHTMLElement.h
> >+  /**
> >+   * Returns the current disabled state of the element.
> >+   */
> >+  virtual bool IsDisabled() const {
> >+    return HasAttr(kNameSpaceID_None, nsGkAtoms::disabled);
> >+  }
> 
> Why does this need to be virtual? Can you make mFieldSet be a
> nsHTMLFieldSetElement pointer instead?

This has to be virtual because IsHTMLFocusable() defined for nsGenericHTMLElement also applies for nsGenericHTMLFormElement (ie. looking for disabled). The nicest solution would be to create a IsHTMLFocusable() in nsGenericHTMLFormElement which would call IsDisabled(). However. IsHTMLFocusable is probably a sensitive code so I do not want to mess with it in this big and sensitive enough patch.
(said in comment 6 actually)
Comment on attachment 474813 [details] [diff] [review]
Part 2 - Make the content aware of the new disabled state rule

>@@ -551,23 +554,20 @@ nsHTMLButtonElement::SubmitNamesValues(n
> 
>   //
>   // We only submit if we were the button pressed
>   //
>   if (aFormSubmission->GetOriginatingElement() != this) {
>     return NS_OK;
>   }
> 
>-  //
>   // Disabled elements don't submit
>-  //
>-  PRBool disabled;
>-  rv = GetDisabled(&disabled);
>-  if (NS_FAILED(rv) || disabled) {
>-    return rv;
>+  // NOTE: would be unexpected to be here with a disabled button...
>+  if (IsDisabled()) {
>+    return NS_OK;
>   }

Hmm.. Might even be a good idea to ignore the disabled state here, that would allow page to prevent double-submission by disabling the button upon submission. Mind bringing this up with the WG? We should leave the code as is for now though of course.


>diff --git a/content/html/content/src/nsHTMLFieldSetElement.cpp b/content/html/content/src/nsHTMLFieldSetElement.cpp
>@@ -95,16 +97,17 @@ private:
> // construction, destruction
> 
> 
> NS_IMPL_NS_NEW_HTML_ELEMENT(FieldSet)
> 
> 
> nsHTMLFieldSetElement::nsHTMLFieldSetElement(already_AddRefed<nsINodeInfo> aNodeInfo)
>   : nsGenericHTMLFormElement(aNodeInfo)
>+  , mElements(new nsContentList(this, MatchListedElements, nsnull, nsnull, PR_TRUE))

I'm not sure this will work unfortunately. The problem is that we don't notify during parsing, and nsContentList relies on notifications.


Looks good otherwise. But I still think the code would be significantly simplified if instead of having to update state every time the disabled status changed we looked up the disabled status directly in calls to IntrisicState.

minusing due to the contentlist issue.
Attachment #474813 - Flags: review?(jonas) → review-
content/html/content/test/test_bug557087-6.html is testing your comments.
Attachment #474813 - Attachment is obsolete: true
Attachment #476443 - Flags: review?(jonas)
Attached patch Inter diff for part 2 (obsolete) — Splinter Review
Comment on attachment 476443 [details] [diff] [review]
Part 2 - Make the content aware of the new disabled state rule

Use the new IsHTML(nsIAtom*) function to check for fieldset parents.

Please add a test that checks that for a document like:

<fieldset id=f disabled>
  <input>
</fieldset>
<script>
document.getElementById('f').disabled = false;
</script>

that the input no longer is disabled. Either using a reftest or by using querySelectorAll(":disabled")
Attachment #476443 - Flags: review?(jonas) → review+
New (more simple) way to manage the <legend> inside disabled fieldset.
All tests pass.
Attachment #475431 - Attachment is obsolete: true
Attachment #476486 - Flags: review?(jonas)
Attachment #475431 - Flags: review?(jonas)
Attachment #474899 - Flags: approval2.0?
Attachment #474906 - Flags: approval2.0?
Attachment #474932 - Flags: approval2.0?
Attachment #475182 - Flags: approval2.0?
Attachment #476443 - Flags: approval2.0?
Attachment #476486 - Flags: approval2.0?
Attachment #476451 - Attachment is obsolete: true
I will try to write a patch to use aNotify when appropriate. If I got r+a in the meantime is it fine for you if I push these patches without aNotify patch, Jonas?
Comment on attachment 476486 [details] [diff] [review]
Part 6 - Elements in the first legend should not be disabled if the fieldset is disabled but not it's fieldset parent

>+nsHTMLFieldSetElement::NotifyElementsOnFirstLegendChange()
>+{
>+  PRUint32 length = mElements->Length(PR_TRUE);
>+  for (PRUint32 i=0; i<length; ++i) {
>+    static_cast<nsGenericHTMLFormElement*>(mElements->GetNodeAt(i))
>+      ->OnFieldSetFirstLegendChanged();
>+  }
>+}

mElements isn't guaranteed to be non-null here, is it? Also, add a comment saying that we could do this only if IsDisabled is true if we wanted to optimize.

Nit: NotifyElementsForFirstLegendChange and FieldSetFirstLegendChanged might be more correct (though i'm not fully sure) We don't usually call functions OnFoo (other than for callbacks in the DOM)

>diff --git a/content/html/content/src/nsHTMLFieldSetElement.h b/content/html/content/src/nsHTMLFieldSetElement.h
>+  nsIContent* const GetFirstLegend() const { return mFirstLegend; }

Why the first const?

r=me with that changed
Attachment #476486 - Flags: review?(jonas) → review+
I'd also probably rename OnFieldSetDisabledChanged to FieldSetDisabledChanged to avoid the 'On'. Up to you though.
Also, holy hell that's a lot of tests. That is awesome!
(In reply to comment #28)
> Comment on attachment 476486 [details] [diff] [review]
> Part 6 - Elements in the first legend should not be disabled if the fieldset is
> disabled but not it's fieldset parent
> 
> >+nsHTMLFieldSetElement::NotifyElementsOnFirstLegendChange()
> >+{
> >+  PRUint32 length = mElements->Length(PR_TRUE);
> >+  for (PRUint32 i=0; i<length; ++i) {
> >+    static_cast<nsGenericHTMLFormElement*>(mElements->GetNodeAt(i))
> >+      ->OnFieldSetFirstLegendChanged();
> >+  }
> >+}
> 
> mElements isn't guaranteed to be non-null here, is it? Also, add a comment
> saying that we could do this only if IsDisabled is true if we wanted to
> optimize.

Indeed. I'm now creating mElements in that case.

> Nit: NotifyElementsForFirstLegendChange and FieldSetFirstLegendChanged might be
> more correct (though i'm not fully sure) We don't usually call functions OnFoo
> (other than for callbacks in the DOM)

Fixed.

> >diff --git a/content/html/content/src/nsHTMLFieldSetElement.h b/content/html/content/src/nsHTMLFieldSetElement.h
> >+  nsIContent* const GetFirstLegend() const { return mFirstLegend; }
> 
> Why the first const?

Typo, I meant |const nsIContent*| not |nsIContent* const|
Depends on: 597744
Unfortunately I have some horrible failures on Windows and MacOS:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1284854696.1284857956.23445.gz (Windows)
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1284855151.1284855633.15091.gz (MacOS)

For Windows, I have been able to test on a Windows machine and it looks like there is two reasons:
1. Some issues in my reftests (the disabled style is kept on Windows for <select> even if there is a background-color);
2. <select disabled> keep the enabled appearance but <fieldest disabled><select></select></fieldset> make the fieldset look disabled...

For MacOS, some <textbox type='number'> tests are failing. I have no idea why. It looks like the <input> field doesn't have the native rendering. I don't see the link with my patches and I have no MacOS to test :(
> > >diff --git a/content/html/content/src/nsHTMLFieldSetElement.h b/content...> > >+  nsIContent* const GetFirstLegend() const { return mFirstLegend; }> > > > Why the first const?> > Typo, I meant |const nsIContent*| not |nsIContent* const|But why have the const at all? const on XPCOM objects doesn't generally make sense as we have very few functions marked up with const, and QI doesn't play well with const anyhow.
I think the windows issue is fixed. Except if with some magic, the MacOS bug is fixed with the few changes I did, I will have to investigate more.
But without a Mac and considering the *AWESOME* performance of the try server, it might take some time :(

Actually, I know that part4 is breaking the test on MacOS so it shouldn't be hard to found what in this patch is breaking the test but a run on try takes between 90 mins and 230 mins on OS X :-/
Depends on: 598316
Depends on: 598329
Blocks: 599163
Depends on: 603734
You need to log in before you can comment on or make changes to this bug.