Bug 66887 - CSS rules not being applied when a hidden field is inserted between an input[type=checkbox] and a label
Summary: CSS rules not being applied when a hidden field is inserted between an input[...
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL: http://www.justinhaygood.com/eyewonde...
Keywords:
Depends on: 70857
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-24 13:30 PDT by Justin Haygood
Modified: 2011-10-26 09:59 PDT (History)
4 users (show)

See Also:


Attachments
Proposed patch (9.07 KB, patch)
2011-09-06 14:55 PDT, Kulanthaivel Palanichamy
no flags Details | Formatted Diff | Diff
Rebased patch (9.11 KB, patch)
2011-09-07 11:03 PDT, Kulanthaivel Palanichamy
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Proposed patch (9.12 KB, patch)
2011-09-07 11:42 PDT, Kulanthaivel Palanichamy
hyatt: review-
hyatt: commit-queue-
Details | Formatted Diff | Diff
Proposed patch (14.24 KB, patch)
2011-09-07 15:03 PDT, Kulanthaivel Palanichamy
no flags Details | Formatted Diff | Diff
Proposed patch (14.82 KB, patch)
2011-09-09 11:43 PDT, Kulanthaivel Palanichamy
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Haygood 2011-08-24 13:30:22 PDT
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.
Comment 1 Justin Haygood 2011-08-24 13:32:52 PDT
This works in Internet Explorer 9 and Firefox 5. This is broken in Chrome 13 and Safari 5.1
Comment 2 Kulanthaivel Palanichamy 2011-09-06 14:55:29 PDT
Created attachment 106491 [details]
Proposed patch
Comment 3 Kulanthaivel Palanichamy 2011-09-07 11:03:58 PDT
Created attachment 106602 [details]
Rebased patch
Comment 4 Early Warning System Bot 2011-09-07 11:32:47 PDT
Comment on attachment 106602 [details]
Rebased patch

Attachment 106602 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9601739
Comment 5 Kulanthaivel Palanichamy 2011-09-07 11:42:42 PDT
Created attachment 106612 [details]
Proposed patch

Fixed the build error.
Comment 6 Dave Hyatt 2011-09-07 13:08:39 PDT
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.
Comment 7 Kulanthaivel Palanichamy 2011-09-07 15:03:23 PDT
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.
Comment 8 Dave Hyatt 2011-09-09 10:55:20 PDT
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.
Comment 9 Kulanthaivel Palanichamy 2011-09-09 11:43:49 PDT
Created attachment 106894 [details]
Proposed patch

Modified according to dhyatt's comments.
Comment 10 Dave Hyatt 2011-09-09 14:14:54 PDT
Comment on attachment 106894 [details]
Proposed patch

r=me
Comment 11 WebKit Review Bot 2011-09-09 16:21:55 PDT
Comment on attachment 106894 [details]
Proposed patch

Clearing flags on attachment: 106894

Committed r94887: <http://trac.webkit.org/changeset/94887>
Comment 12 WebKit Review Bot 2011-09-09 16:21:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Ian 'Hixie' Hickson 2011-10-25 15:18:39 PDT
I suspect this change is what made scrolling the HTML spec significantly slower.
Comment 14 Ian 'Hixie' Hickson 2011-10-25 15:31:01 PDT
Filed bug 70857 on the regression.
Comment 15 Dimitri Glazkov (Google) 2011-10-26 09:59:36 PDT
Reverted as http://trac.webkit.org/changeset/98492.