Last Comment Bug 302186 - Support :default pseudoclass
: Support :default pseudoclass
Status: RESOLVED FIXED
: css3, qawanted
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- enhancement with 1 vote (vote)
: ---
Assigned To: Allan Beaufour
: Hixie (not reading bugmail)
Mentors:
http://www.w3.org/TR/css3-ui/#pseudo-...
Depends on: 349355 371061
Blocks: 347165
  Show dependency treegraph
 
Reported: 2005-07-26 05:25 PDT by Allan Beaufour
Modified: 2007-07-19 17:25 PDT (History)
16 users (show)
sayrer: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Work in progress (28.30 KB, patch)
2005-07-26 07:00 PDT, Allan Beaufour
no flags Details | Diff | Review
Patch (36.57 KB, patch)
2005-07-27 05:43 PDT, Allan Beaufour
no flags Details | Diff | Review
Testcase (4.96 KB, application/xhtml+xml)
2005-07-28 02:10 PDT, Allan Beaufour
no flags Details
Patch v2 (41.16 KB, patch)
2005-07-28 06:02 PDT, Allan Beaufour
no flags Details | Diff | Review
Testcase v2 (7.69 KB, application/xhtml+xml)
2005-07-28 06:10 PDT, Allan Beaufour
no flags Details
Patch v2 updated to tip (37.62 KB, patch)
2005-10-10 04:12 PDT, Allan Beaufour
bzbarsky: review-
Details | Diff | Review
patch_v3(-w Not for checkin) (39.12 KB, patch)
2006-08-03 06:42 PDT, jpl24
bzbarsky: review-
Details | Diff | Review
patch_v4(-w Not for check in) (40.33 KB, patch)
2006-08-14 19:53 PDT, jpl24
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Review
patch_v4 (40.44 KB, patch)
2006-08-14 20:20 PDT, jpl24
no flags Details | Diff | Review
bustage_fix (915 bytes, patch)
2006-08-15 22:17 PDT, jpl24
no flags Details | Diff | Review
default_testacases (7.75 KB, application/zip)
2006-08-19 15:13 PDT, jpl24
no flags Details

Description Allan Beaufour 2005-07-26 05:25:31 PDT
We should support the :default pseudoclass defined in CSS3 UI:
http://www.w3.org/TR/css3-ui/#pseudo-default
and WebForms 2:
http://whatwg.org/specs/web-forms/current-work/#relation
Comment 2 Allan Beaufour 2005-07-26 07:00:39 PDT
Created attachment 190551 [details] [diff] [review]
Work in progress
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-07-26 08:23:22 PDT
See bug 271720 comment 74.  Based on the specs, we should be supporting :default
on the appropriate options, checkboxes, and radios.
Comment 4 Anne (:annevk) 2005-07-27 02:21:58 PDT
I have asked Ian about that in the past, he said it did not make sense and
therefore there is no relationship defined. See:
<http://listserver.dreamhost.com/pipermail/whatwg-whatwg.org/2005-March/003266.html>
Comment 5 Anne (:annevk) 2005-07-27 04:06:21 PDT
The issue mentioned in comment 1 is different. It seems that Firefox always
choses the first button as default submit button, whether disabled or not and
irrelevant which form control field has focus. So Firefox is conforming to Web
Forms 2 in this regard. This also means we should pass this additional testcase:
 <http://annevankesteren.nl/test/css/ui/008>

(Only when :disabled is supported though.)
Comment 6 Anne (:annevk) 2005-07-27 05:36:42 PDT
type DOM attribute change testcase:
 <http://annevankesteren.nl/test/css/ui/012>
Comment 7 Allan Beaufour 2005-07-27 05:43:22 PDT
Created attachment 190695 [details] [diff] [review]
Patch

(In reply to https://bugzilla.mozilla.org/show_bug.cgi?id=271720#c69)
> (From update of attachment 187107 [details] [diff] [review])
> >Index: content/html/content/src/nsGenericHTMLElement.cpp
> >@@ -3152,17 +3152,18 @@ nsGenericHTMLFormElement::SetForm(nsIDOM
> >+	// XXX (when) should SetForm() send notifications?
> >+	mForm->RemoveElement(this, PR_TRUE);
> 
> Please sort this out (look at the callers of SetForm and figure out what
> they're doing, etc).	Notifying unnecessarily would be a major perf issue (eg

> would force flushing out the content sink for no reason from the callsite at
>
<http://lxr.mozilla.org/seamonkey/source/content/html/document/src/nsHTMLConten

> tSink.cpp#842>; that same callsite could also leave to broken behavior on the

> part of the content sink).  It's entirely possible that SetForm() needs an
> aNotify argument of its own.

I've added an aNotify argument to SetForm() which is set to false by the
content sink. (AddElement() also got an aNotify argument)
 
> >Index: content/html/content/src/nsHTMLFormElement.cpp
> >+  NS_IMETHOD GetDefaultSubmitElement(nsIFormControl** aElement) const;
> 
> Again, I'd like this to be an NS_IMETHOD_(nsIFormControl*).  There's no
reason
> for the refcounting here.  I just checked with jst, and he agrees on this.

Done.
 
> @@ -1195,50 +1203,137 @@ nsHTMLFormElement::AddElement(nsIFormCon
> 
> >+	    mozAutoDocUpdate(document, UPDATE_CONTENT_STATE, PR_TRUE);
> 
> Ouch.  This will notify in the middle of sink notifications.	It'll also
notify
> in the middle of attr changes that have aNotify set to false.  Please fix
this
> to only notify when it should, ok?
> 
> Note that notifying in the middle of sink notifications may well be ok here
--
> we at least want to notify that oldElement is not the default anymore, since
it
> might have a frame.  But I really don't think we want to be doing this if
> aNotify is false on an attr change...

I do not know enough on how content sink, frames, etc, actually work to say in
which situations we need to notify from the content sink call. But
SetAttr(aNotify == PR_FALSE) was clearly a mistake.
 
> >+nsHTMLFormElement::ResetDefaultSubmitElement(PRBool aNotify)
> 
> >+	  if (type == NS_FORM_INPUT_SUBMIT ||
> >+	      type == NS_FORM_BUTTON_SUBMIT ||
> >+	      type == NS_FORM_INPUT_IMAGE) {
> 
> Maybe we should have an nsIFormControl function that does this?  followup
bug,
> perhaps?

nsIFormControl::IsSubmitControl(). (Should be const, but then GetType() should
be const too, which is part of bug 84400... so I'll let the consts surf in on
that one :-) )
 
> >+  // Inform about change
> 
> Even if aNotify is false?  That can't be right...

Most of the notify errors grounded in my misunderstanding of what actually
happens on ContentStatesChanged... I think I've gotten it now.
 
> >+  if (oldDefault || mDefaultSubmitElement) {
> >+	nsIDocument* document = GetCurrentDoc();
> >+	if (document) {
> 
> I really wish we could avoid notifying here for cases when the default
control
> is being removed from the document altogether (since in that case the fact
that
> its state changed really doesn't matter)...  But I bet we get into here
before
> oldDefault's document is nulled out in that case, right?

That's my understanding, yes.


> I'm not seeing any provisions for toggling the :default state for
> radio/checkbox when the relevant attr changes...
> 
> I don't see any provisions for toggling the :default state when the
"selected"
> attr changes.
 
My understanding is that :default for these should match the default selection
on load, so it shouldn't matter if attributes are changed. Or?

Only (known) bug left in this, is the case with multiple radio buttons having
checked="checked".
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-07-27 08:25:19 PDT
(In reply to comment #4)
> he said it did not make sense

The email in which he said so is wrong on one major point -- it _is_ possible to
tell from the DOM which elements are the _default_ selected ones (the ones the
form would reset to; this is not quite the same as "originally selected", of
course).

(In reply to comment #7)
> I do not know enough on how content sink, frames, etc, actually work to say in
> which situations we need to notify from the content sink call.

You never do, basically.

> My understanding is that :default for these should match the default selection
> on load, so it shouldn't matter if attributes are changed. Or?

I would assume :default should match the default selection that would be reset
to if the reset button is pushed...  That makes a lot more sense than the "on
load" thing, especially if the whole form is constructed via the DOM (say I
create an input, insert in the DOM, then set the checked attribute; with your
current impl it won't match :default, but if I set the attr before inserting in
the DOM it will....  that seems wrong).

> Only (known) bug left in this, is the case with multiple radio buttons having
> checked="checked".

That's not really a bug; that's a case where behavior is so badly undefined that
just about anything we do is right.
Comment 9 Anne (:annevk) 2005-07-27 08:33:05 PDT
I talked to Ian today. CSS3 UI should be followed. Web Forms 2 will hopefully be
updated this week. It will _probably_ say that :default should match the
defaultChecked DOM attribute for radio buttons and checkboxes and the
defaultSelected DOM attribute for option elements. (This means that although the
last radio button with defaultChecked set to true remains checked, but that more
can match :default.)
Comment 10 Allan Beaufour 2005-07-28 02:10:25 PDT
Created attachment 190819 [details]
Testcase
Comment 11 Allan Beaufour 2005-07-28 06:02:05 PDT
Created attachment 190830 [details] [diff] [review]
Patch v2

This one also handles dynamic changes to checked and selected attributes.
Comment 12 Allan Beaufour 2005-07-28 06:10:12 PDT
Created attachment 190831 [details]
Testcase v2

Added buttons to toggle checked and selected.
Comment 13 Aaron Leventhal 2005-08-03 07:20:32 PDT
It doesn't sound like this issue can help with bug 120527.

A button's dark border is supposed to show only when the Enter key will activate
it -- in otherwords not when another button or a textarea has focus.

Perhaps for bug 120527 we need a :-moz-default pseudo class to implement that?
Or perhaps we should use Neil's JS hack over there.
Comment 14 Allan Beaufour 2005-08-04 02:08:47 PDT
(In reply to comment #13)
> It doesn't sound like this issue can help with bug 120527.
> 
> A button's dark border is supposed to show only when the Enter key will activate
> it -- in otherwords not when another button or a textarea has focus.

You mean input I guess.

That's also what WF2 says ... CSS3UI is sort of vague. So :default should
possibly behave like that?
Comment 15 Anne (:annevk) 2005-08-04 02:10:30 PDT
Yes. That is the behavior that would make sense.
Comment 16 Allan Beaufour 2005-08-04 03:02:19 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > It doesn't sound like this issue can help with bug 120527.
> > 
> > A button's dark border is supposed to show only when the Enter key will activate
> > it -- in otherwords not when another button or a textarea has focus.
> 
> You mean input I guess.

Oh man, I constantly misread people these days :(... you definately mean textarea.
Comment 17 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-08-04 12:05:52 PDT
The default button doesn't depend on which input is currently focused, though. 
So I'm not sure it makes sense to affect matching of :default based on that.
Comment 18 Allan Beaufour 2005-08-04 22:29:50 PDT
(In reply to comment #17)
> The default button doesn't depend on which input is currently focused, though. 
> So I'm not sure it makes sense to affect matching of :default based on that.

Not if the inputs are in the same form, no, but for multiple forms it does
matter.  If :default only matches a submit element when it will actually be
triggered when the user presses enter, it would need to be controlled by focus
and blur on inputs and submit elements.
Comment 19 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-08-21 17:45:43 PDT
> If :default only matches a submit element when it will actually be
> triggered when the user presses enter

I'm not sure that makes a lot of sense as a definition of :default...
Comment 20 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-10-05 14:46:14 PDT
Allan, is that patch still current as far as needing review?
Comment 21 Allan Beaufour 2005-10-07 01:42:32 PDT
(In reply to comment #20)
> Allan, is that patch still current as far as needing review?

/me digs around his memory... finding it full of holes as usual...

Yes, I think it's there functionality-wise, but it needs to be brought up to
tip. I'll do that Monday, ok?
Comment 22 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-10-07 05:34:31 PDT
Sounds good.
Comment 23 Allan Beaufour 2005-10-10 04:12:09 PDT
Created attachment 199056 [details] [diff] [review]
Patch v2 updated to tip
Comment 24 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-02-10 14:08:18 PST
Comment on attachment 199056 [details] [diff] [review]
Patch v2 updated to tip

I'm sorry it took me so long to get to this....

>Index: content/events/public/nsIEventStateManager.h
>+#define NS_EVENT_STATE_DEFAULT      0x00008000 // CSS3-UI

This needs to be updated to tip, of course.

>Index: layout/style/nsCSSStyleSheet.cpp
>+    else if (nsCSSPseudoClasses::defaultPseudo == pseudoClass->mAtom) {
>+      result = STATE_CHECK(NS_EVENT_STATE_DEFAULT);
>+    }

This also....

>Index: content/html/content/public/nsIForm.h
>+   * @param aNotify send notifications?

  @param aNotify If true, send nsIDocumentObserver notifications as needed.

Same for other spots where this comment appears (there are 3-5 places in this patch).

>+   * Get the default submit element. If there's no default submit element, it
>+   * returns nsnull.

s/it returns null/return null/

>+   * Resets the default submit element. Used to find the new default submit

s/resets/reset/

>Index: content/html/content/public/nsIFormControl.h
>   NS_IMETHOD SetForm(nsIDOMHTMLFormElement* aForm,
>-                     PRBool aRemoveFromForm = PR_TRUE) = 0;
>+                     PRBool aRemoveFromForm = PR_TRUE,
>+                     PRBool aNotify = PR_TRUE) = 0;

Please make the aNotify arg non-default.  Better yet, make both non-default, since I doubt you can make aNotify non-default without making aRemoveFromForm so in a safe way.  That will make it clear which places are flushing when they should not, etc.

I suspect there are few enough callers that this shouldn't be a big deal.

>+  /**
>+   * Is the control a submit control?

What does that mean?  Perhaps this should give examples of the sort of thing that is considered a "submit control"?  Or say something like "a control which submits the form when activated by the user" or something?

>Index: content/html/content/src/nsGenericHTMLElement.cpp

>@@ -3261,17 +3263,17 @@ nsGenericHTMLFormElement::SetAttr(PRInt3

This function doesn't exist anymore.  The code is in nsGenericHTMLFormElement::AfterSetAttr now.  Merge that?

> nsGenericHTMLFormElement::UnsetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,

Which means your changes to this function are not needed, I believe.

>@@ -3365,16 +3376,30 @@ nsGenericHTMLFormElement::IntrinsicState
>+nsGenericHTMLFormElement::IsSubmitControl()
>+  if (type == NS_FORM_INPUT_SUBMIT ||
>+      type == NS_FORM_BUTTON_SUBMIT ||
>+      type == NS_FORM_INPUT_IMAGE)
>+  {
>+    return PR_TRUE;

How about:

  return type == NS_FORM_INPUT_SUBMIT ||
         type == NS_FORM_BUTTON_SUBMIT ||
         type == NS_FORM_INPUT_IMAGE;

instead?

>Index: content/html/content/src/nsHTMLButtonElement.cpp

>+nsHTMLButtonElement::IntrinsicState() const

Why not put this logic in nsGenericHTMLFormElement::IntrinsicState (conditioned on IsSubmitcontrol()) instead of putting it in multiple places?  I think I'd much prefer that.

>Index: content/html/content/src/nsHTMLFormElement.cpp

>@@ -1212,50 +1220,128 @@ nsHTMLFormElement::AddElement(nsIFormCon

>+    // First submit element added to the form
>+    if (!mDefaultSubmitElement) {
>+      mDefaultSubmitElement = aChild; // weak

Why no need to notify here?  Doesn't this change the child's state?  Or are we relying on all codepaths that lead to AddElement recreating frames (which I _believe_ is true)?  But then why notify in the other branch?  Or is the notification in that branch only needed for the old default?  But then why notify for both?

I guess my point is the two branches in this function should be consistent.

>+    else if (aNotify && !lastElement &&
>+             CompareFormControlPosition(aChild, mDefaultSubmitElement) < 0) {

So if aNotify is false and we're inserting somewhere in the middle of the form we can keep a stale mDefaultSubmitElement?  Why?  It seems to me that we should set mDefaultSubmitElement correctly no matter what aNotify is.  This should have been caught in testing, no?

>+nsHTMLFormElement::ResetDefaultSubmitElement(PRBool aNotify)
>+  nsCOMPtr<nsISimpleEnumerator> formControls;

I really wish we had a faster "internal" way of doing it.  This is ok for now, but want to file a followup bug on improving both this and the code in nsHTMLInputElement, perhaps?

>Index: content/html/content/src/nsHTMLInputElement.cpp

>@@ -502,27 +502,39 @@ nsHTMLInputElement::AfterSetAttr(PRInt32
>+  if (aName == nsHTMLAtoms::checked) {
>+    if (aNotify && !GET_BOOLBIT(mBitField, BF_PARSER_CREATING)) {

Why the mBitField check?  What's that for?  I think you should just use aNotify here and not second-guess the caller...

>+      if (aNotify) {

And this check is not needed.

Note that this function might need merging on trunk too.  :(

> nsHTMLInputElement::MaybeSubmitForm(nsPresContext* aPresContext)

Please post a -w version of the next patch in addition to the normal diff?  That would make this part much much easier to review.  Probably save me an hour or so, and I doubt it takes more than a minute or two to do.

>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    if (numTextControlsFound == 1) {

Why do you need that NS_ENSURE_SUCESS(rv, rv) there?

> nsHTMLInputElement::IntrinsicState() const
>+  } else if (mType == NS_FORM_INPUT_SUBMIT || mType == NS_FORM_INPUT_IMAGE) {

This part of the change can largely be undone if the :default handling for submits moves to nsGenericHTMLFormElement

I'd like to see a version of this patch with the comments addressed, but please let me know if you don't have time to do that and I'll see what I can do.  Again, sorry for the ludicrous review delay; next round will be much much faster.

Oh, and please attach the tests for this to the bug and make sure to check them into the regression tests when you land this?
Comment 25 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-05-30 14:22:55 PDT
Martijn, do you think you might have time to get this updated and drive it in?
Comment 26 jpl24 2006-07-31 21:17:14 PDT
If it would be helpful I could probably find some time to merge this and run the test cases.
Comment 27 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-07-31 22:49:29 PDT
If you could, and fix up the review issues, that would be wonderful!
Comment 28 jpl24 2006-08-01 17:29:58 PDT
Okay, will do. I've got it merged and most of the review comments addressed, but it's failing some of the dynamic test cases. I'll try and figure those out.
Comment 29 jpl24 2006-08-03 06:41:09 PDT
Okay, I've got it working again and passing Allan's tests(Testcase_v2). The issue with dynamic modification was that the change to IsStateSelector got lost along the way. I'll attach a patch in a moment that addresses Boris's review comments. I also fixed several places where MozAutoDocUpdate was used without giving the instance a name. Responses to select review comments, the rest I think I fixed.

> > nsGenericHTMLFormElement::UnsetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
> 
> Which means your changes to this function are not needed, I believe.

I think they are still needed for the case of the type attribute being removed. Or did I miss something?

> >Index: content/html/content/src/nsHTMLButtonElement.cpp
> 
> >+nsHTMLButtonElement::IntrinsicState() const
> 
> Why not put this logic in nsGenericHTMLFormElement::IntrinsicState (conditioned
> on IsSubmitcontrol()) instead of putting it in multiple places?  I think I'd
> much prefer that.

Moved the part checking which element was the default submit element. Did you want more than that moved?

> >Index: content/html/content/src/nsHTMLFormElement.cpp
> 
> >@@ -1212,50 +1220,128 @@ nsHTMLFormElement::AddElement(nsIFormCon
> 
> >+    // First submit element added to the form
> >+    if (!mDefaultSubmitElement) {
> >+      mDefaultSubmitElement = aChild; // weak
> 
> Why no need to notify here?  Doesn't this change the child's state?  Or are we
> relying on all codepaths that lead to AddElement recreating frames (which I
> _believe_ is true)?  But then why notify in the other branch?  Or is the
> notification in that branch only needed for the old default?  But then why
> notify for both?
> 
> I guess my point is the two branches in this function should be consistent.

The new element only needs to be notified on in the case where the type attribute was added, which causes the form element to be removed and added. I'm trying to think if there's a case where that could cause the first branch to be hit.


> >+    else if (aNotify && !lastElement &&
> >+             CompareFormControlPosition(aChild, mDefaultSubmitElement) < 0) {
> 
> So if aNotify is false and we're inserting somewhere in the middle of the form
> we can keep a stale mDefaultSubmitElement?  Why?  It seems to me that we should
> set mDefaultSubmitElement correctly no matter what aNotify is.  This should
> have been caught in testing, no?

I changed this to notify, I think it has to.

> >+nsHTMLFormElement::ResetDefaultSubmitElement(PRBool aNotify)
> >+  nsCOMPtr<nsISimpleEnumerator> formControls;
> 
> I really wish we had a faster "internal" way of doing it.  This is ok for now,
> but want to file a followup bug on improving both this and the code in
> nsHTMLInputElement, perhaps?

Filed bug 347165

> 
> >Index: content/html/content/src/nsHTMLInputElement.cpp
> 
> >@@ -502,27 +502,39 @@ nsHTMLInputElement::AfterSetAttr(PRInt32
> >+  if (aName == nsHTMLAtoms::checked) {
> >+    if (aNotify && !GET_BOOLBIT(mBitField, BF_PARSER_CREATING)) {
> 
> Why the mBitField check?  What's that for?  I think you should just use aNotify
> here and not second-guess the caller...

This looks like it was copied from an earlier part of this function, I removed it.

Comment 30 jpl24 2006-08-03 06:42:48 PDT
Created attachment 231940 [details] [diff] [review]
patch_v3(-w Not for checkin)
Comment 31 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-08-06 17:21:17 PDT
Comment on attachment 231940 [details] [diff] [review]
patch_v3(-w Not for checkin)

>+++ content/html/content/public/nsIForm.h

So... I'm not sure how you created this diff...

> // IID for the nsIFormManager interface
> #define NS_IFORM_IID    \
>-{ 0x6982e217, 0x8b5b, 0x4fd2,  \
>-  { 0x98, 0xc, 0x3a, 0xce, 0xc5, 0xe, 0x82, 0x88 } }

But that's not an IID this interface ever had.

>-class nsIForm : public nsIDOMNSHTMLFormElement
>+class nsIForm : public nsISupports

And this part makes no sense.  I'd like to see an updated diff of this file that makes sense...

>+++ content/html/content/public/nsIFormControl.h

>   virtual PRBool AllowDrop() = 0;
>+
>+
>+  /**

Remove one of those newlines.

>+++ content/html/content/src/nsGenericHTMLElement.cpp

> nsGenericHTMLFormElement::~nsGenericHTMLFormElement()
> {
>   // Clean up.  Set the form to nsnull so it knows we went away.
>-  SetForm(nsnull);
>+  SetForm(nsnull, PR_TRUE, PR_TRUE);
> }

So... won't this possibly dispatch notifications on this content that's being destroyed?  We don't want that.  I think it's safe to pass PR_FALSE for aNotify here (and perhaps assert that we are not the default content of our form or something?).

> nsGenericHTMLFormElement::UnsetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,

Why are the changes to this function needed?  Doesn't the removal in BeforeSetAttr and readdition in AfterSetAttr do what we want?  If this code goes away, we can also stop exposing ResetDefaultSubmitElement and make it a protected method of nsHTMLFormElement, right?

>@@ -3325,16 +3343,21 @@ nsGenericHTMLFormElement::IntrinsicState

>+  if (IsSubmitControl() && mForm && mForm->GetDefaultSubmitElement() == this) {

Why bother with the IsSubmitControl() check?  If we're the default submit element, we're a submit control.  Add an assert to that effect here if you want.

>+++ content/html/content/src/nsHTMLFormElement.cpp

>+nsHTMLFormElement::AddElement(nsIFormControl* aChild,
>+                              PRBool aNotify)
>+    // When the type of the form element changes the element is removed
>+    // and added. In that case we need to notify on both the old and new
>+    // elements. Otherwise, the new element will already have been
>+    // notified on.

I'm not following this comment...

>+    else if (!lastElement &&
>+             CompareFormControlPosition(aChild, mDefaultSubmitElement) < 0) {

Why aren't we checking aNotify?  We should be.

Note that the changes here don't actually address my comment on the version 2 patch....

What I _think_ is the right code flow is that we never notify on aChild, and notify on the current mDefaultSubmitElement if aChild is replacing it and aNotify is true.  But that assumes that all callers of AddElement lead to a frame reconstruct on aChild.  Is that true?

>+      if (document) {
>+        mozAutoDocUpdate upd(document, UPDATE_CONTENT_STATE, PR_TRUE);

Please use 

  MOZ_AUTO_DOC_UPDATE(document, UPDATE_CONTENT_STATE, PR_TRUE);

That will make sure people can't accidentally edit away the 'upd' part.  ;)

>+        mDefaultSubmitElement = aChild; // weak

Why is leaving an incorrect mDefaultSubmitElement OK when we have no document?  That seems wrong (and it should be easy to write tests to demonstrate that this leads to buggy behavior, no? we should add those tests to this bug so we make sure that all the relevant tests for this land in the regression tests).

>+nsHTMLFormElement::ResetDefaultSubmitElement(PRBool aNotify)

>+  if (aNotify && (oldDefault || mDefaultSubmitElement)) {

Can we assert here that oldDefault != mDefaultSubmitElement?  If not, should we check for that?

>+      mozAutoDocUpdate upd(document, UPDATE_CONTENT_STATE, PR_TRUE);

MOZ_AUTO_DOC_UPDATE

>+++ content/html/content/src/nsHTMLInputElement.cpp

>+          mozAutoDocUpdate upd(document, UPDATE_CONTENT_STATE, aNotify);

MOZ_AUTO_DOC_UPDATE

I'd like to see a patch without -w at some point too, btw, once things stabilize.

Do we relly want to notify if the "checked" attribute is changed on a control of types other than checkbox and radio?  I'd think not...

> nsHTMLInputElement::IntrinsicState() const

>+  } else if (mType == NS_FORM_INPUT_SUBMIT || mType == NS_FORM_INPUT_IMAGE) {

Why do you need this check?  You only do something interesting for NS_FORM_INPUT_IMAGE, so just check for that.

Looking at this function, I think we have interesting style resolution bugs when the type attribute of an HTML input changes; same thing for <button> elements.  Could you please file a followup bug on investigating that?  I think we want to fix them for 1.9...

Thanks for bringing this up to date!  Looking forward to the patch with the nits picked.
Comment 32 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-08-06 17:25:14 PDT
Er, just realized I should respond to comment 29....

> I think they are still needed for the case of the type attribute being removed.

UnsetAttr calls BeforeSetAttr and AfterSetAttr, no?  And those handle the type attr being removed by removing and reinserting the element in the form?

> Moved the part checking which element was the default submit element. 

No, that was it.  Thanks!

> The new element only needs to be notified on in the case where the type
> attribute was added

I think we'll have to generally handle type attribute changes in that followup bug I was talking about in my review comment.

> I'm trying to think if there's a case where that could cause the first branch 
> to be hit.

Parsing hits it, no?

> I changed this to notify, I think it has to.

Like I said, I don't buy this change.  We should talk about this; maybe catch me on irc?

Again, thank you!
Comment 33 jpl24 2006-08-14 19:53:49 PDT
Created attachment 233713 [details] [diff] [review]
patch_v4(-w Not for check in)
Comment 34 jpl24 2006-08-14 20:20:14 PDT
Created attachment 233715 [details] [diff] [review]
patch_v4

Not -w.

I also fixed two random whitespace/newline/parenthesis changes that crept in:

+    if (aNotify &&
+        (aName == nsHTMLAtoms::disabled && CanBeDisabled())) {

and 

 // nsHTMLFormElement
-
Comment 35 jpl24 2006-08-14 20:23:25 PDT
> Looking at this function, I think we have interesting style resolution bugs
> when the type attribute of an HTML input changes; same thing for <button>
> elements.  Could you please file a followup bug on investigating that?  I think
> we want to fix them for 1.9...

I tried a few test cases and couldn't find any bugs. bz, could you point me in the right direction?
Comment 36 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-08-14 21:40:13 PDT
Something like this should do it:

CSS:

span { color: green }
:default + span { color: red }

HTML:

<input type="checkbox" checked="checked">

JS:  Remove the type attribute from the input of a setTimeout.
Comment 37 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-08-14 21:40:49 PDT
Er, except use :checked instead of :default -- The :default code actually handles this case... ;)
Comment 38 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-08-15 20:16:20 PDT
Comment on attachment 233713 [details] [diff] [review]
patch_v4(-w Not for check in)

This looks great!  Thanks a ton for fixing this up!

r+sr=bzbarsky
Comment 39 jpl24 2006-08-15 22:17:03 PDT
Created attachment 233942 [details] [diff] [review]
bustage_fix

This might fix it?
Comment 40 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-08-15 22:57:36 PDT
Checked in that patch, plus a bustage fix.  jpl24 says he'll create some fully automated testcases that I could add to the regression tests.  Filed bug 348809 on the type-change issues.
Comment 41 jpl24 2006-08-19 15:13:30 PDT
Created attachment 234622 [details]
default_testacases

Test cases for the default pseudoclass. 

bz, these tests start both in numbering and in test coverage from the ones you wrote. I included a read me file that describes what the tests cover.
Comment 42 jpl24 2006-08-19 15:23:00 PDT
Note that dynamic test cases numbers 10 and 12 currently fail. I think this test case behavior is correct and caused by content state changed notification on the removed element coming too late, after the element is removed from the document. Follow up bug?
Comment 43 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-08-21 15:12:29 PDT
> Note that dynamic test cases numbers 10 and 12 currently fail.

Yeah, and with the patch for bug 348809 a few of the "add" testcases will fail too, because I removed a notification that really should not be needed.  The problem you're seeing here is bug 229915.

I checked in those testcases to the regression tests.

This bug is fixed.  Thank you!
Comment 44 Robert Sayre 2006-10-28 12:18:14 PDT
Comment on attachment 234622 [details]
default_testacases

I added the test cases in here to mochitest. There were two failing, so they are running as todo()

RCS file: /cvsroot/mozilla/testing/mochitest/tests/test_bug302186.html,v
done
Checking in tests/test_bug302186.html;
/cvsroot/mozilla/testing/mochitest/tests/test_bug302186.html,v  <--  test_bug302186.html
initial revision: 1.1
done
Comment 45 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-07-19 17:25:58 PDT
I just realized that the tests that jpl24 started from (the static/dynamicDefault tests at http://web.mit.edu/bzbarsky/www/testcases/css-selectors/) never made it into the tree.  I went ahead and landed them.

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