User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031216 Firebird/0.7+ Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031216 Firebird/0.7+ From: http://bugzilla.mozilla.org/show_bug.cgi?id=15608#c60 See testcase that I'm attaching. Using this snippet in html: <input type="checkbox" name="test" id="c1"><label for="c1">check #c1</label> and this style: input{display:none;} input:checked+label{background-color:green;} you can still check the checkbox and the label becomes green, but unchecking the checkbox (which works with display:none), the label stays green. Checking/unchecking a checkbox to the left of the original does reresolve the style in the right way. Reproducible: Always Steps to Reproduce: 1. Press button: toggle display radio/checkbox buttons -> inputs have display:none 2. check/uncheck the checkboxes 3. Actual Results: See Details Expected Results: Reresolve the style of the labels in the right way. An easy workaround for this problem is to use: input {visibility:hidden;position:absolute;} This works and has the same effect as display:none

The bug seems to be in the code at http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsHTMLInputElement.cpp#1122

So the problem seems to be that this is a misuse of the nsIDocument/nsIDocumentObserver apis, pure and simple -- BeginUpdate and EndUpdate are not getting called around the ContentStatesChanged call. The right fix would be, imo: 1) Add an aNotify arg to SetCheckedInternal 2) Change callers to pass TRUE or FALSE as needed (eg DoneCreatingElement should end up passing FALSE). 3) If aNotify is true, BeginUpdate, do the content state change, and EndUpdate. Steps 1 and 2 are really optional, but will keep us from flushing the content sink on every single checkbox that has the checked attr set. David, let me know whether you have time to deal with this; if not, I could do it, I think.

This doesn't fix the problem with radio inputs.

Of course, the problem is that nsHTMLInputElement::SetCheckedInternal has to do *more* when there's no frame...

I apologize for the unclear comment. Here's a patch to illustrate what I was talking about (-w for discussion; once we decide what the right thing to do is I'll make a patch that can actually go in). The reason this bug happens is that ContentStatesChanged doesn't get called on the input when there is no frame. As a result, the restyling doesn't happen. The fix for that is to make sure to call ContentStatesChanged no matter whether we have a frame (that's what this patch does, and it fixes the bug for me). That's what the code looked like when we had bug 134560 -- it called ContentStatesChanged on the node (which had no frame _yet_) and the frame constructor saw there was no frame and called RecreateFramesForContent which created the frame; then control unwound back to the content sink, which called ContentAppended on the node, which caused the frame constructor to create another frame for it. Hence my comment about adding a BeginUpdate() call -- that makes the sink call ContentAppended on whatever it currently has that's not been "officially" appended yet before control returns to the caller of BeginUpdate(). The thing is, we no longer call RecreateFramesForContent from content state notification code -- we call MaybeRecreateFramesForContent. This doesn't construct a frame unless the node is in the undisplayed map, and nodes that have never been notified for are not in the undisplayed map. So even if I remove the BeginUpdate/EndUpdate calls here I don't get bug 134560 with the testcase in that bug. I could probably produce bug 134560 with a more complicated setup in the current world, eg if the checked state of the checkbox caused a reframe of the parent, and not all of the parent's kids had had ContentAppended called on them yet (probably need to use our IB stuff and selectors that affect preceding siblings, or just selectors that affect the parent). So to summarize: 1) To fix this bug we need to call ContentStatesChanged no matter whether we have the frame. 2) I don't _think_ that will regress bug 134560 immediately. 3) To make sure we don't regress bug 134560 we should call BeginUpdate/EndUpdate 4) To prevent this being a performance problem and to prevent the content sink from confusing itself, we should not do any of this when the code is being called from the content sink. Does that make sense?

I'd come to pretty much the same patch when I wrote comment 5. I probably need to split pretty much all of SetChecked into DoSetChecked and add an aNotify parameter to DoSetChecked and a bunch of other functions (including SetCheckedInternal).

Comment on attachment 139251 [details] [diff] [review] patch >Index: src/nsHTMLInputElement.cpp >+nsHTMLInputElement::RadioSetChecked(PRBool aNotify) > rv = NS_STATIC_CAST(nsHTMLInputElement*, > NS_STATIC_CAST(nsIDOMHTMLInputElement*, currentlySelected) >- )->SetCheckedInternal(PR_FALSE); >+ )->SetCheckedInternal(PR_FALSE, PR_TRUE); Why PR_TRUE there instead of aNotify? I guess we need to make sure that any radios that are already in the doc that this affects get notified properly, right? Add a comment to that effect? r=bzbarsky with that.

Comment on attachment 139251 [details] [diff] [review] patch - In nsHTMLInputElement::MaybeSubmitForm(): + if (mDocument && aNotify) { + mDocument->BeginUpdate(UPDATE_CONTENT_STATE); mDocument->ContentStatesChanged(this, nsnull, NS_EVENT_STATE_CHECKED); + mDocument->EndUpdate(UPDATE_CONTENT_STATE); } Wanna use mozAutoDocUpdate(mDocument, UPDATE_CONTENT_STATE, aNotify) here in stead of writing it out? - In nsHTMLOptionElement::SetSelectedInternal(): + if (aNotify && mDocument) { + mDocument->BeginUpdate(UPDATE_CONTENT_STATE); mDocument->ContentStatesChanged(this, nsnull, NS_EVENT_STATE_CHECKED); + mDocument->EndUpdate(UPDATE_CONTENT_STATE); + } Same here. sr=jst either way.

Fix checked in to trunk, 2004-01-20 20:28 -0800.

Comment on attachment 139251 [details] [diff] [review] patch >-nsHTMLInputElement::AddedToRadioGroup() >+nsHTMLInputElement::AddedToRadioGroup(PRBool aNotify) > { >+ if (aNotify) >+ aNotify = GET_BOOLBIT(mBitField, BF_PARSER_CREATING) != 0; bzbarsky poinst out this should have been == 0, or just a !. He even commented it since as thought it was written that way.

These pass on trunk. The display:none parts fail in "Firebird 2004-01-17-02-trunk", but not in exactly the same way as the original testcase. I guess clicking is special?

Clicking shouldn't be special here.

One difference between clicking and script when checking multiple things is that when clicking we'll process restyles between clicks. Does adding flushes between the scripted sets change behavior?

The difference between clicking and js-setting persists even if I add flushes or only toggle one checkbox at a time. Clicking can turn items yellow (but not white); js-setting does not change the color at all. That's all with Firebird 2004-01-17-02-trunk. Trunk does fine.

The tests pass in Firebird 2004-01-21-02-trunk (that's four days after the other build I tested), so I think they're at least testing the same bug :) Using cb.click() instead of cb.checked=!cb.checked doesn't affect the result in the older build.

Should I worry about this, or should I just check in the tests? :)

Checked in tests: http://hg.mozilla.org/mozilla-central/rev/e467c9f03ada

Oh, display:none.... No color change at all is sorta what I'd expect there for those builds.