The test case specified by URL field doesn't work.
Created attachment 82287 [details] Patch
Created attachment 82601 [details] Patch
Comment on attachment 82601 [details] Patch 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.
Created attachment 85442 [details] Revised patch
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.
Created attachment 94553 [details] Revised patch - rebased and test case reworked
(In reply to comment #6) > Created an attachment (id=94553) [details] > Revised patch - rebased and test case reworked Could someone review this patch?
Ping for review.
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.
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.
I don't know whether this is related, but this jsfiddle: http://jsfiddle.net/gSPqX/1/ 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).
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.
Created attachment 148574 [details] Simpler test case using checkbox and :checked
I believe this is a duplicate of bug #12520
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.
<rdar://problem/12286306>
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; }
All 3 of the testcases seem to be working fine for me in Safari 9.1.
I believe Benjamin fixed this a while ago.