Bug 66887 – CSS rules not being applied when a hidden field is inserted between an input[type=checkbox] and a label

• Component: CSS
• Status: RESOLVED CONFIGURATION CHANGED

[Justin Haygood]

1. Open the attached URL (http://www.justinhaygood.com/eyewonder/css_test.htm)
2. Notice how the "broken" text does not cycle between red & white when clicked, but the "working" text does
3. The only difference is the hidden input field between the label and the corresponding checkbox
4. Changing the display state of the checkbox does not affect it

This issue was discovered by Ryan Caskey at EyeWonder, LLC. I am filing the ticket on his behalf.

[Justin Haygood]

This works in Internet Explorer 9 and Firefox 5. This is broken in Chrome 13 and Safari 5.1

[Kulanthaivel Palanichamy]

Created attachment 106491 [details]
Proposed patch

[Kulanthaivel Palanichamy]

Created attachment 106602 [details]
Rebased patch

[Early Warning System Bot]

Comment on attachment 106602 [details]
Rebased patch

Attachment 106602 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9601739

[Kulanthaivel Palanichamy]

Created attachment 106612 [details]
Proposed patch

Fixed the build error.

[Dave Hyatt]

Comment on attachment 106612 [details]
Proposed patch

This patch isn't quite right. Here are the issues I see:

(1) What's the point of keeping childrenAffectedByDirectAdjacentRules once you change it to track a bool in the children instead? You shouldn't need both flags.

(2) If you eliminate the parent flag, then what you're really patching is "forceCheckOfNextElementSibling". You can basically set that to true if the next sibling is affected by direct adjacent rules. Otherwise you don't have to do anything.

Let me know if I'm mistaken, but it sure looks like you don't need to keep around the other flag any longer with this change.

[Kulanthaivel Palanichamy]

Created attachment 106651 [details]
Proposed patch

More cleaner version.

This patch removes childrenAffectedByDirectAdjacentRules parent flag and forces style calculation for adjacent sibling if the current element is affected by direct adjacent rules irrespective of whether the adjacent sibling is affected by direct adjacent rules or not. This change is needed to make sure that the adjacent sibling's style is recalculated when there is no renderer attached to the adjacent sibling but it may be affected by direct adjacent rules at that moment.

[Dave Hyatt]

Comment on attachment 106651 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106651&action=review

> Source/WebCore/dom/Element.cpp:1157
> +        bool childAffectedByDirectAdjacentRules = element->renderStyle() ? element->renderStyle()->affectedByDirectAdjacentRules() : false;
> +        if (childAffectedByDirectAdjacentRules || forceCheckOfNextElementSibling || forceCheckOfAnyElementSibling)

Seems like you can kill forceCheckOfNextElementSibling, no? I don't think it serves any purpose any longer. It's causing you to check one node too many I believe.

[Kulanthaivel Palanichamy]

Created attachment 106894 [details]
Proposed patch

Modified according to dhyatt's comments.

[Dave Hyatt]

Comment on attachment 106894 [details]
Proposed patch

r=me

[WebKit Review Bot]

Comment on attachment 106894 [details]
Proposed patch

Clearing flags on attachment: 106894

Committed r94887: <http://trac.webkit.org/changeset/94887>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ian 'Hixie' Hickson]

I suspect this change is what made scrolling the HTML spec significantly slower.

[Ian 'Hixie' Hickson]

Filed bug 70857 on the regression.

[Dimitri Glazkov (Google)]

Reverted as http://trac.webkit.org/changeset/98492.

[Brent Fulgham]

The relevant website is no longer present.

[Brent Fulgham]

The test case provided in the patch still passes.

[Radar WebKit Bug Importer]

<rdar://problem/96913412>