Bug 12520 - ARCH: General sibling selectors and multiple chained adjacent selectors don't work dynamically
: ARCH: General sibling selectors and multiple chained adjacent selectors don't...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: CSS
: 420+
: All All
: P2 Normal
Assigned To: Allan Sandfeld Jensen
: HasReduction
Depends on: 17680 18026 18027 9279 12998
Blocks: 93304
  Show dependency treegraph
 
Reported: 2007-01-31 19:49 PST by Darin Fisher (:fishd, Google)
Modified: 2014-11-02 16:30 PST (History)
20 users (show)

See Also:


Attachments
testcase (608 bytes, text/html)
2007-01-31 19:50 PST, Darin Fisher (:fishd, Google)
no flags Details
Simple patch (3.22 KB, patch)
2007-08-15 07:27 PDT, Allan Sandfeld Jensen
eric: review-
Details | Formatted Diff | Diff
test case (457 bytes, text/html)
2008-01-14 07:12 PST, Markus Wulftange
no flags Details
Test page to reproduce problem. (1.51 KB, text/html)
2010-04-18 20:46 PDT, Stephanie Hobson
no flags Details
Patch (8.18 KB, patch)
2012-03-08 07:59 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (9.64 KB, patch)
2012-08-16 02:52 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-02 (378.96 KB, application/zip)
2012-08-16 04:55 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from gce-cr-linux-04 (374.22 KB, application/zip)
2012-08-16 05:47 PDT, WebKit Review Bot
no flags Details
Patch (12.51 KB, patch)
2012-08-27 06:18 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (9.77 KB, patch)
2012-10-15 08:43 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (10.61 KB, patch)
2012-10-24 07:38 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Fisher (:fishd, Google) 2007-01-31 19:49:51 PST
Sibling selectors don't work dynamically

e.g.:

h2 + div {
  display: block;
}
h2.foo + div {
  display: none;
}

If I toggle the class attribute of h2, I don't see the display of the next sibling div change.

Testcase coming up.  Perhaps this is related to bug 12519.
Comment 1 Darin Fisher (:fishd, Google) 2007-01-31 19:50:56 PST
Created attachment 12843 [details]
testcase
Comment 2 Allan Sandfeld Jensen 2007-08-14 07:43:27 PDT
To fix this what is needed is a way to force a recalcStyle(Force) for not only all children, but also for all later siblings.

To really bad part is that anything that calls setChanged() should in theory trigger this subtree restyle if document()->usesSiblingRules() is set, just like document()->usesDescentRules() triggers restyle of children.

I have a solution in KHTML that instead creates a 1 to 1 multimap of dependencies, but that costs in memory. 
Comment 3 Allan Sandfeld Jensen 2007-08-15 07:27:31 PDT
Created attachment 15978 [details]
Simple patch

This is a direct patch of the problem without introducing new dependency data.
Comment 4 Maciej Stachowiak 2007-08-19 13:15:13 PDT
Comment on attachment 15978 [details]
Simple patch

Patch looks good to me. Tracking dependencies might be worth it as a next step if the speed hit here is large and the memory cost of tracking is not excessive.

I think it would be best to check this into feature-branch for now.

r=me
Comment 5 Eric Seidel 2007-10-07 02:06:20 PDT
Comment on attachment 15978 [details]
Simple patch

After applying your patch to feature branch, the test case still fails.  We can't land this until that's resolved. :(  Marking as r- until this is fixed for landing on feature-branch (or the test case is fixed).
Comment 6 Allan Sandfeld Jensen 2007-10-07 03:00:55 PDT
The test also uses non-deterministic matching, so you need to apply the patch for #3428 first.
Comment 7 Markus Wulftange 2008-01-14 07:12:19 PST
Created attachment 18437 [details]
test case

I think the bug demonstrated in the attached test case is related to the same bug.
Comment 8 Eric Seidel 2008-02-09 01:38:33 PST
I believe Acid3 test 42 is hitting this:

Test 42: expected: 1, got: 0 - rule did not start matching after change

    function () {
      // test 42: +, ~, >, and ' ' in dynamic situations
      selectorTest(function (doc, add, expect) {
        var div1 = doc.createElement('div');
        div1.id = "div1";
        doc.body.appendChild(div1);
        var div2 = doc.createElement('div');
        doc.body.appendChild(div2);
        var div3 = doc.createElement('div');
        doc.body.appendChild(div3);
        var div31 = doc.createElement('div');
        div3.appendChild(div31);
        var div311 = doc.createElement('div');
        div31.appendChild(div311);
        var div3111 = doc.createElement('div');
        div311.appendChild(div3111);
        var match = add("#div1 ~ div div + div > div");
        expect(div1, 0, "failure 1");
        expect(div2, 0, "failure 2");
        expect(div3, 0, "failure 3");
        expect(div31, 0, "failure 4");
        expect(div311, 0, "failure 5");
        expect(div3111, 0, "failure 6");
        var div310 = doc.createElement('div');
        div31.insertBefore(div310, div311);
        expect(div1, 0, "failure 7");
        expect(div2, 0, "failure 8");
        expect(div3, 0, "failure 9");
        expect(div31, 0, "failure 10");
        expect(div310, 0, "failure 11");
        expect(div311, 0, "failure 12");
        expect(div3111, match, "rule did not start matching after change");
      });
      selectorTest(function (doc, add, expect) {
        var div1 = doc.createElement('div');
        div1.id = "div1";
        doc.body.appendChild(div1);
        var div2 = doc.createElement('div');
        div1.appendChild(div2);
        var div3 = doc.createElement('div');
        div2.appendChild(div3);
        var div4 = doc.createElement('div');
        div3.appendChild(div4);
        var div5 = doc.createElement('div');
        div4.appendChild(div5);
        var div6 = doc.createElement('div');
        div5.appendChild(div6);
        var match = add("#div1 > div div > div");
        expect(div1, 0, "failure 14");
        expect(div2, 0, "failure 15");
        expect(div3, 0, "failure 16");
        expect(div4, match, "failure 17");
        expect(div5, match, "failure 18");
        expect(div6, match, "failure 19");
        var p34 = doc.createElement('p');
        div3.insertBefore(p34, div4);
        p34.insertBefore(div4, null);
        expect(div1, 0, "failure 20");
        expect(div2, 0, "failure 21");
        expect(div3, 0, "failure 22");
        expect(p34, 0, "failure 23");
        expect(div4, 0, "failure 24");
        expect(div5, match, "failure 25");
        expect(div6, match, "failure 26");
      });
      selectorTest(function (doc, add, expect) {
        var div1 = doc.createElement('div');
        div1.id = "div1";
        doc.body.appendChild(div1);
        var div2 = doc.createElement('div');
        div1.appendChild(div2);
        var div3 = doc.createElement('div');
        div2.appendChild(div3);
        var div4 = doc.createElement('div');
        div3.appendChild(div4);
        var div5 = doc.createElement('div');
        div4.appendChild(div5);
        var div6 = doc.createElement('div');
        div5.appendChild(div6);
        var match = add("#div1 > div div > div");
        expect(div1, 0, "failure 27");
        expect(div2, 0, "failure 28");
        expect(div3, 0, "failure 29");
        expect(div4, match, "failure 30");
        expect(div5, match, "failure 31");
        expect(div6, match, "failure 32");
        var p23 = doc.createElement('p');
        div2.insertBefore(p23, div3);
        p23.insertBefore(div3, null);
        expect(div1, 0, "failure 33");
        expect(div2, 0, "failure 34");
        expect(div3, 0, "failure 35");
        expect(p23, 0, "failure 36");
        expect(div4, match, "failure 37");
        expect(div5, match, "failure 38");
        expect(div6, match, "failure 39");
      });
      return 3;
    },

Comment 9 Dave Hyatt 2008-02-09 15:35:45 PST
Bug 17203 actually fixed test 42.  Turns out it was just testing DOM children changes, so it passes.  This bug should still be kept open for other kinds of changes though (like :hover or class changing).

Comment 10 Robert Blaut 2008-02-21 22:31:03 PST
(In reply to comment #9)
>  This bug should still be kept open for other kinds of
> changes though (like :hover or class changing).
> 

Added bug 9279 and bug 12998 as blocking bugs.
Comment 11 Dave Hyatt 2008-03-20 11:13:18 PDT
We still don't work right with the ~ selector or with multiple chained + selectors.  That's what is left now.

Comment 12 Robert Blaut 2008-03-23 14:01:50 PDT
(In reply to comment #11)
> We still don't work right with the ~ selector or with multiple chained +
> selectors.  That's what is left now.
> 

So it's better to split these cases on separate bugs and keeps this bug open as umbrella bug.
Comment 13 Ryan 2010-02-27 17:02:40 PST
The general sibling combinator in conjunction with checked pseudo-class isn't working e.g.

input:checked ~ div { ... }

This won't work and neither will chaining adjacent sibling combinators e.g.

input:checked + label + div { ... }

However using the checked pseudo class with just a single adjacent sibling combinator works when the checkbox/radio is checked/unchecked e.g.

input:checked + label { ... }

However #2, doing a general sibling combinator with the enabled or disabled pseudo-class works fine e.g.

input:enabled ~ div { ... }
input:disabled ~ div { ... }

Seems it doesn't update the styles on elements that are changed dynamically i.e. :checked or if an input is disabled through javascript.
Comment 14 Stephanie Hobson 2010-04-18 20:46:28 PDT
Created attachment 53653 [details]
Test page to reproduce problem.

Test page to reproduce problem, also includes a test case to reproduce Bug 26539 and I will attach it there too.
Comment 15 Allan Sandfeld Jensen 2012-03-08 07:59:58 PST
Created attachment 130823 [details]
Patch

Patch that tries propagate restyling beyond the first sibling if necessary, but only over elements that has been observed during styling to possibly propagate restyles to siblings.

This solves most inefficient cases, but can in some cases cause a performance degradation. Especially very simple double sibling combinator combinations such as "li + li" will mark all li as propagating restyles.

The patch is not up for review as it is not yet complete and needs test-cases, but comments are appreciated.
Comment 16 Allan Sandfeld Jensen 2012-08-16 02:52:07 PDT
Created attachment 158759 [details]
Patch
Comment 17 Allan Sandfeld Jensen 2012-08-16 02:56:50 PDT
(In reply to comment #16)
> Created an attachment (id=158759) [details]
> Patch

The patch has been updated. The flag for indirect adjacent sibling combinators have been removed (the parent flag is good enough for now), and the flag on the child-element is now used instead of the generic flag on the parent, making the choice whether the next element should be restyled or not more fine-grained.
Comment 18 WebKit Review Bot 2012-08-16 04:55:13 PDT
Comment on attachment 158759 [details]
Patch

Attachment 158759 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13509569

New failing tests:
fast/selectors/sibling-combinators-dynamic.html
Comment 19 WebKit Review Bot 2012-08-16 04:55:20 PDT
Created attachment 158776 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 20 WebKit Review Bot 2012-08-16 05:47:34 PDT
Comment on attachment 158759 [details]
Patch

Attachment 158759 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13513551

New failing tests:
fast/selectors/sibling-combinators-dynamic.html
Comment 21 WebKit Review Bot 2012-08-16 05:47:41 PDT
Created attachment 158787 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 22 Allan Sandfeld Jensen 2012-08-27 06:18:41 PDT
Created attachment 160706 [details]
Patch

Changed test-case to be text-dumped
Comment 23 Allan Sandfeld Jensen 2012-10-15 08:43:36 PDT
Created attachment 168720 [details]
Patch

Rebased
Comment 24 Build Bot 2012-10-15 10:52:36 PDT
Comment on attachment 168720 [details]
Patch

Attachment 168720 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14298756

New failing tests:
fast/selectors/sibling-combinators-dynamic.html
Comment 25 WebKit Review Bot 2012-10-15 15:43:32 PDT
Comment on attachment 168720 [details]
Patch

Attachment 168720 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14286784

New failing tests:
fast/selectors/sibling-combinators-dynamic.html
Comment 26 Allan Sandfeld Jensen 2012-10-24 07:38:33 PDT
Created attachment 170392 [details]
Patch

Now with reference test
Comment 27 Andreas Kling 2014-02-05 11:04:58 PST
Comment on attachment 170392 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.
Comment 28 Benjamin Poulain 2014-11-01 10:39:46 PDT
Closing: fixed a long time ago.
Comment 29 Allan Sandfeld Jensen 2014-11-01 18:54:51 PDT
I doubt very much it is solved. None of the issues it depends on have been solved, and there has been no work to fix the issue, probably because it doesn't really affect much real content.
Comment 30 Benjamin Poulain 2014-11-01 22:10:18 PDT
(In reply to comment #29)
> I doubt very much it is solved. None of the issues it depends on have been
> solved, and there has been no work to fix the issue, probably because it
> doesn't really affect much real content.

Try Nightly. I fixed this, and then some, a few months ago.

The new style resolution is also the basis of the extended :nth-child() and other new features.
Comment 31 Allan Sandfeld Jensen 2014-11-02 02:33:31 PST
Ah great. You probably want to close all the bugs this depends on as well then. They are all different cases of this bug.
Comment 32 Philippe Wittenbergh 2014-11-02 16:30:50 PST
Great work Benjamin! Thanks. All my test cases work correctly. I've marked bug 88219 as fixed as it now works fine.