Bug 54373 - CSS '+' combinator with more than 2 combinations doesn't work
Summary: CSS '+' combinator with more than 2 combinations doesn't work
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Leo Yang
URL: http://www.w3.org/Style/CSS/Test/CSS2...
Keywords: InRadar
Depends on:
Blocks: 93304
  Show dependency treegraph
Reported: 2011-02-13 21:14 PST by Leo Yang
Modified: 2016-06-08 00:39 PDT (History)
18 users (show)

See Also:

Patch (6.00 KB, patch)
2011-02-13 21:42 PST, Leo Yang
no flags Details | Formatted Diff | Diff
Patch (12.56 KB, patch)
2011-02-16 01:03 PST, Leo Yang
hyatt: review-
hyatt: commit-queue-
Details | Formatted Diff | Diff
Revised patch (21.21 KB, patch)
2011-03-11 00:51 PST, Leo Yang
no flags Details | Formatted Diff | Diff
Revised patch - rebased and test case reworked (20.69 KB, patch)
2011-05-23 20:42 PDT, Leo Yang
no flags Details | Formatted Diff | Diff
Simpler test case using checkbox and :checked (561 bytes, text/html)
2012-06-20 08:33 PDT, Dan Moore
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Leo Yang 2011-02-13 21:14:52 PST
The test case specified by URL field doesn't work.
Comment 1 Leo Yang 2011-02-13 21:42:56 PST
Created attachment 82287 [details]
Comment 2 Leo Yang 2011-02-16 01:03:11 PST
Created attachment 82601 [details]
Comment 3 Dave Hyatt 2011-03-08 13:33:44 PST
Comment on attachment 82601 [details]

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

Good first start.  Don't forget you need to patch checkForSiblingStyleChanges as well.  Will need a dynamic test to cover that code also.

> Source/WebCore/css/CSSStyleSelector.cpp:2243
> +                if (e->parentNode() && e->parentNode()->isElementNode()) {
> +                    RenderStyle* parentStyle = elementStyle ? elementParentStyle : e->parentNode()->renderStyle();
> +                    if (parentStyle)
> +                        parentStyle->setChildrenAffectedByDirectAdjacentRules();

If you're going to put a bit on the children, then you don't need to set a bit on the parent any longer.  Just remove this code.

> Source/WebCore/dom/Element.cpp:1101
> +        if (hasDirectAdjacentRules && n->isElementNode() && n->renderStyle() && n->renderStyle()->affectedByDirectAdjacentRules())

You need to be smarter about this.  You're looking for runs of nodes that have the affected bit set.  For example if a node n has changed, then you want to recalc all the elements that follow n that are affected by direct adjacent rules.  You don't need to do this if n hasn't actually changed.  You're doing too much recalc as a result.

> Source/WebCore/rendering/style/RenderStyle.h:136
> -    unsigned m_childIndex : 21; // Plenty of bits to cache an index.
> +    bool m_affectedByDirectAdjacentRules: 1;
> +    unsigned m_childIndex : 20; // Plenty of bits to cache an index.

Should be able to leave this alone if you just remove the childrenaffected bit.
Comment 4 Leo Yang 2011-03-11 00:51:04 PST
Created attachment 85442 [details]
Revised patch
Comment 5 Eric Seidel (no email) 2011-05-23 14:47:27 PDT
Comment on attachment 85442 [details]
Revised patch

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

> LayoutTests/fast/css/dynamic-multiple-sibling-selector-after-dom-change.html:3
> +<html>
> +    <head>
> +        <style type="text/css">

I wouldn't bother indeneting.

> LayoutTests/fast/css/dynamic-multiple-sibling-selector-after-dom-change.html:28
> +                if (window.layoutTestController)
> +                    layoutTestController.notifyDone();

You don't need to waitUntilDone if you're using onload.  The dump happens right after onload.
Comment 6 Leo Yang 2011-05-23 20:42:25 PDT
Created attachment 94553 [details]
Revised patch - rebased and test case reworked
Comment 7 Leo Yang 2011-06-06 21:09:16 PDT
(In reply to comment #6)
> Created an attachment (id=94553) [details]
> Revised patch - rebased and test case reworked

Could someone review this patch?
Comment 8 Leo Yang 2012-01-07 04:14:40 PST
Ping for review.
Comment 9 Eric Seidel (no email) 2012-01-30 15:17:24 PST
Comment on attachment 94553 [details]
Revised patch - rebased and test case reworked

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

> Source/WebCore/dom/Element.cpp:-1145
> -    // FIXME: This check is good enough for :hover + foo, but it is not good enough for :hover + foo + bar.
> -    // For now we will just worry about the common case, since it's a lot trickier to get the second case right
> -    // without doing way too much re-resolution.

The question for this change is if we got the second case right, w/o doing too much re-resolution.  Whoever wrote this fixme (presumably hyatt) would need to answer that for this bug.
Comment 10 Eric Seidel (no email) 2012-01-30 15:18:14 PST
This was posted for review 8 months ago.  I'll email hyatt and see if he's willing to do a second round of review.
Comment 11 Mike McNally 2012-06-13 16:43:04 PDT
I don't know whether this is related, but this jsfiddle:


demonstrates some clearly buggy behavior. According to the web inspector, the browser recognizes the applicability of the ":checked" rule to the <div>, but the "display: none" setting is not applied. If, in the inspector, the rule is disabled and re-enabled, then the element is hidden.

Chrome and Safari both behave more or less the same way. I'm testing with Chrome 19.0.1084.52 (Linux) and Safari 5.1.4 (7534.54.16) (Windows XP).
Comment 12 Thiago Marcos P. Santos 2012-06-19 07:55:05 PDT
BTW, accordingly to the tool I'm proposing at bug 89470 (in fact it is just a refactoring of "webkit-patch patches-to-review"), this bug has the oldest attachment pending review at the WebKit project. Right now it is 392 days old.
Comment 13 Dan Moore 2012-06-20 08:33:38 PDT
Created attachment 148574 [details]
Simpler test case using checkbox and :checked
Comment 14 Allan Sandfeld Jensen 2012-08-22 15:30:41 PDT
I believe this is a duplicate of bug #12520
Comment 15 Leo Yang 2012-09-12 11:50:45 PDT
Comment on attachment 94553 [details]
Revised patch - rebased and test case reworked

Cancel review request since this patch doesn't apply on the tip of tree.
Comment 16 Radar WebKit Bug Importer 2012-09-12 11:51:17 PDT
Comment 17 Aldonio 2012-09-26 17:33:59 PDT
Is this the correct place to write workarounds for fellow developers?

I managed to avoid this bug using a general sibling selector to write an unnecessary rule, of course, the results depends on context.

Taking the simpler test case as an example:

/* Buggy rule */
input:checked + .unchecked + .checked
	display: inline;

/* Unnecessary rule */
input:checked ~ .checked
	display: none;
Comment 18 Chris Rebert 2016-06-08 00:06:35 PDT
All 3 of the testcases seem to be working fine for me in Safari 9.1.
Comment 19 Antti Koivisto 2016-06-08 00:39:56 PDT
I believe Benjamin fixed this a while ago.