Bug 54373

Summary:

CSS '+' combinator with more than 2 combinations doesn't work

Product:

WebKit

Reporter:

Leo Yang <leo.yang>

Component:

CSS

Assignee:

Leo Yang <leo.yang>

Status:

RESOLVED WORKSFORME

Severity:

Normal

CC:

aldomx, allan.jensen, bb-webkit.org, bdakin, benjamin, dan, dbates, eric, hyatt, kling, koivisto, m5, mitz, priyajeet.hora, tmpsantos, tonikitoo, webkit-bug-importer, webkit

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

OS:

All

URL:

http://www.w3.org/Style/CSS/Test/CSS2.1/20100127/html4/dynamic-sibling-combinator-001.htm

Bug Depends on:

Bug Blocks:

93304

Attachments:

Description	Flags
Patch	none
Patch	hyatt: review-, hyatt: commit-queue-
Revised patch	none
Revised patch - rebased and test case reworked	none
Simpler test case using checkbox and :checked	none

Leo Yang

Reported 2011-02-13 21:14:52 PST

The test case specified by URL field doesn't work.

Attachments
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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Leo Yang

Comment 1 2011-02-13 21:42:56 PST

Created attachment 82287 [details] Patch

Leo Yang

Comment 2 2011-02-16 01:03:11 PST

Created attachment 82601 [details] Patch

Dave Hyatt

Comment 3 2011-03-08 13:33:44 PST

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.

Leo Yang

Comment 4 2011-03-11 00:51:04 PST

Created attachment 85442 [details] Revised patch

Eric Seidel (no email)

Comment 5 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.

Leo Yang

Comment 6 2011-05-23 20:42:25 PDT

Created attachment 94553 [details] Revised patch - rebased and test case reworked

Leo Yang

Comment 7 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?

Leo Yang

Comment 8 2012-01-07 04:14:40 PST

Ping for review.

Eric Seidel (no email)

Comment 9 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.

Eric Seidel (no email)

Comment 10 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.

Mike McNally

Comment 11 2012-06-13 16:43:04 PDT

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).

Thiago Marcos P. Santos

Comment 12 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.

Dan Moore

Comment 13 2012-06-20 08:33:38 PDT

Created attachment 148574 [details] Simpler test case using checkbox and :checked

Allan Sandfeld Jensen

Comment 14 2012-08-22 15:30:41 PDT

I believe this is a duplicate of bug #12520

Leo Yang

Comment 15 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.

Radar WebKit Bug Importer

Comment 16 2012-09-12 11:51:17 PDT

<rdar://problem/12286306>

Aldonio

Comment 17 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; }

Chris Rebert

Comment 18 2016-06-08 00:06:35 PDT

All 3 of the testcases seem to be working fine for me in Safari 9.1.

Antti Koivisto

Comment 19 2016-06-08 00:39:56 PDT

I believe Benjamin fixed this a while ago.

Note You need to log in before you can comment on or make changes to this bug.