231081 - Checkboxes and radio buttons with dynamic selector :checked and adjacent combinator does not reresolve style

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Martijn Wargers (dead)]

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

Comment 1

[Martijn Wargers (dead)]

Attachment: Example of the problem with input:checked+label and input{display:none}

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch bz suggested

This doesn't fix the problem with radio inputs.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: What I really meant by comment 2 and 3 (diff -w; do not check in)

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?

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 9

[Boris Zbarsky [:bzbarsky]]

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 10

[Johnny Stenback (:jst)]

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.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 13

[Jesse Ruderman]

Attachment: reftests

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?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Clicking shouldn't be special here.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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?

Comment 16

[Jesse Ruderman]

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.

Comment 17

[Jesse Ruderman]

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.

Comment 18

[Jesse Ruderman]

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

Comment 19

[Jesse Ruderman]

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

Comment 20

[Boris Zbarsky [:bzbarsky]]

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