Last Comment Bug 557087 - Implement the fieldset element disabled attribute
: Implement the fieldset element disabled attribute
Status: RESOLVED FIXED
: dev-doc-complete, html5
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla2.0b7
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
http://dev.w3.org/html5/spec/forms.ht...
Depends on: 596100 596088 597744 598316 598329 603734 613027
Blocks: html5forms 599163
  Show dependency treegraph
 
Reported: 2010-04-04 05:43 PDT by Mounir Lamouri (:mounir)
Modified: 2010-11-22 16:31 PST (History)
7 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 - Fieldset elements can be disabled (9.00 KB, patch)
2010-09-11 18:14 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part 1 - Fieldset elements can be disabled (13.99 KB, patch)
2010-09-11 18:32 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part 2 - Make the content aware of the new disabled state rule (136.34 KB, patch)
2010-09-13 13:56 PDT, Mounir Lamouri (:mounir)
jonas: review-
Details | Diff | Splinter Review
Part 3 - Make layout aware of the new disabled state rule (20.21 KB, patch)
2010-09-13 16:34 PDT, Mounir Lamouri (:mounir)
dbaron: review+
jonas: approval2.0+
Details | Diff | Splinter Review
Part 4 - Make widget aware of the new disabled state rule (52.85 KB, patch)
2010-09-13 16:48 PDT, Mounir Lamouri (:mounir)
roc: review+
jonas: approval2.0+
Details | Diff | Splinter Review
Part 5 - Test the rendrenig when inside a disabled fieldset (52.85 KB, patch)
2010-09-13 17:10 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part 5 - Test the rendering when inside a disabled fieldset (1.76 KB, patch)
2010-09-13 17:40 PDT, Mounir Lamouri (:mounir)
dbaron: review+
jonas: approval2.0+
Details | Diff | Splinter Review
Part 6 - WIP - Elements in first legend should not be disabled if the fieldset is disabled but not it's fieldset parent (18.09 KB, patch)
2010-09-14 00:37 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part 1 - Fieldset elements can be disabled (15.70 KB, patch)
2010-09-14 12:52 PDT, Mounir Lamouri (:mounir)
jonas: review+
jonas: approval2.0+
Details | Diff | Splinter Review
Part 2.5 - Fixing a test assuming disabled work on <object> (1.61 KB, patch)
2010-09-14 12:54 PDT, Mounir Lamouri (:mounir)
jonas: review+
Details | Diff | Splinter Review
Part 6 - Elements in the first legend should not be disabled if the fieldset is disabled but not it's fieldset parent (112.10 KB, patch)
2010-09-14 23:34 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part 2 - Make the content aware of the new disabled state rule (139.01 KB, patch)
2010-09-17 17:32 PDT, Mounir Lamouri (:mounir)
jonas: review+
jonas: approval2.0+
Details | Diff | Splinter Review
Inter diff for part 2 (7.09 KB, patch)
2010-09-17 17:51 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part 6 - Elements in the first legend should not be disabled if the fieldset is disabled but not it's fieldset parent (107.37 KB, patch)
2010-09-17 21:56 PDT, Mounir Lamouri (:mounir)
jonas: review+
jonas: approval2.0+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2010-04-04 05:43:28 PDT
"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."
Comment 1 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-08-23 22:57:40 PDT
FWIW, I think this would actually be a quite useful feature
Comment 2 Mounir Lamouri (:mounir) 2010-08-23 23:13:39 PDT
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?
Comment 3 Mounir Lamouri (:mounir) 2010-08-23 23:14:41 PDT
By the way, I will bring these issues to whatwg tomorrow.
Comment 4 Mounir Lamouri (:mounir) 2010-09-11 18:14:41 PDT
Created attachment 474445 [details] [diff] [review]
Part 1 - Fieldset elements can be disabled

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.
Comment 5 Mounir Lamouri (:mounir) 2010-09-11 18:32:12 PDT
Created attachment 474448 [details] [diff] [review]
Part 1 - Fieldset elements can be disabled

Better with some reftests ;)
Comment 6 Mounir Lamouri (:mounir) 2010-09-13 13:56:08 PDT
Created attachment 474813 [details] [diff] [review]
Part 2 - Make the content aware of the new disabled state rule

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?).
Comment 7 Mounir Lamouri (:mounir) 2010-09-13 16:34:32 PDT
Created attachment 474899 [details] [diff] [review]
Part 3 - Make layout aware of the new disabled state rule

Tests will come in part 5.
Comment 8 Mounir Lamouri (:mounir) 2010-09-13 16:45:41 PDT
(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.
Comment 9 Mounir Lamouri (:mounir) 2010-09-13 16:48:32 PDT
Created attachment 474906 [details] [diff] [review]
Part 4 - Make widget aware of the new disabled state rule

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.
Comment 10 Mounir Lamouri (:mounir) 2010-09-13 17:10:21 PDT
Created attachment 474916 [details] [diff] [review]
Part 5 - Test the rendrenig when inside a disabled fieldset

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...
Comment 11 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-09-13 17:11:54 PDT
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
Comment 12 Mounir Lamouri (:mounir) 2010-09-13 17:40:08 PDT
Created attachment 474932 [details] [diff] [review]
Part 5 - Test the rendering when inside a disabled fieldset

Oups, i attached part4 instead of part5. Sorry David :(
Comment 13 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-09-13 17:51:14 PDT
Comment on attachment 474932 [details] [diff] [review]
Part 5 - Test the rendering when inside a disabled fieldset

r=dbaron
Comment 14 Mounir Lamouri (:mounir) 2010-09-14 00:37:23 PDT
Created attachment 475011 [details] [diff] [review]
Part 6 - WIP - Elements in first legend should not be disabled if the fieldset is disabled but not it's fieldset parent

This patch works (as much as tested).

I still need to write more tests and make some optimizations if possible.
Comment 15 Mounir Lamouri (:mounir) 2010-09-14 12:52:12 PDT
Created attachment 475182 [details] [diff] [review]
Part 1 - Fieldset elements can be disabled

Updated to remove a reftest which was assuming that fieldset can't be disabled. I have reftests in this patch testing the exact opposite ;)
Comment 16 Mounir Lamouri (:mounir) 2010-09-14 12:54:15 PDT
Created attachment 475183 [details] [diff] [review]
Part 2.5 - Fixing a test 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.
Comment 17 Mounir Lamouri (:mounir) 2010-09-14 13:12:15 PDT
(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>.
Comment 18 Mounir Lamouri (:mounir) 2010-09-14 23:34:20 PDT
Created attachment 475431 [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

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?
Comment 19 Mounir Lamouri (:mounir) 2010-09-14 23:35:36 PDT
This feature seems dumb but the sum of the patches is 330k! :-/
Comment 20 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-09-15 23:25:25 PDT
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
Comment 21 Mounir Lamouri (:mounir) 2010-09-15 23:38:20 PDT
(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 22 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-09-16 21:26:50 PDT
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.
Comment 23 Mounir Lamouri (:mounir) 2010-09-17 17:32:03 PDT
Created attachment 476443 [details] [diff] [review]
Part 2 - Make the content aware of the new disabled state rule

content/html/content/test/test_bug557087-6.html is testing your comments.
Comment 24 Mounir Lamouri (:mounir) 2010-09-17 17:51:51 PDT
Created attachment 476451 [details] [diff] [review]
Inter diff for part 2
Comment 25 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-09-17 18:06:46 PDT
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")
Comment 26 Mounir Lamouri (:mounir) 2010-09-17 21:56:02 PDT
Created 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

New (more simple) way to manage the <legend> inside disabled fieldset.
All tests pass.
Comment 27 Mounir Lamouri (:mounir) 2010-09-17 22:03:12 PDT
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 28 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-09-18 00:26:53 PDT
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
Comment 29 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-09-18 00:29:46 PDT
I'd also probably rename OnFieldSetDisabledChanged to FieldSetDisabledChanged to avoid the 'On'. Up to you though.
Comment 30 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-09-18 00:30:03 PDT
Also, holy hell that's a lot of tests. That is awesome!
Comment 31 Mounir Lamouri (:mounir) 2010-09-18 14:09:56 PDT
(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|
Comment 32 Mounir Lamouri (:mounir) 2010-09-18 19:25:32 PDT
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 :(
Comment 33 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-09-18 19:27:05 PDT
> > >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.
Comment 34 Mounir Lamouri (:mounir) 2010-09-18 23:29:07 PDT
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 :-/

Note You need to log in before you can comment on or make changes to this bug.