Bug 12520 – ARCH: General sibling selectors and multiple chained adjacent selectors don't work dynamically

• Component: CSS
• Status: RESOLVED FIXED

[Darin Fisher (:fishd, Google)]

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.

[Darin Fisher (:fishd, Google)]

Created attachment 12843 [details]
testcase

[Allan Sandfeld Jensen]

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. 

[Allan Sandfeld Jensen]

Created attachment 15978 [details]
Simple patch

This is a direct patch of the problem without introducing new dependency data.

[Maciej Stachowiak]

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

[Eric Seidel (no email)]

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

[Allan Sandfeld Jensen]

The test also uses non-deterministic matching, so you need to apply the patch for #3428 first.

[Markus Wulftange]

Created attachment 18437 [details]
test case

I think the bug demonstrated in the attached test case is related to the same bug.

[Eric Seidel (no email)]

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;
    },

[Dave Hyatt]

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

[Robert Blaut]

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

[Dave Hyatt]

We still don't work right with the ~ selector or with multiple chained + selectors.  That's what is left now.

[Robert Blaut]

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

[Ryan]

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.

[Stephanie Hobson]

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.

[Allan Sandfeld Jensen]

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.

[Allan Sandfeld Jensen]

Created attachment 158759 [details]
Patch

[Allan Sandfeld Jensen]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Allan Sandfeld Jensen]

Created attachment 160706 [details]
Patch

Changed test-case to be text-dumped

[Allan Sandfeld Jensen]

Created attachment 168720 [details]
Patch

Rebased

[Build Bot]

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

[WebKit Review Bot]

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

[Allan Sandfeld Jensen]

Created attachment 170392 [details]
Patch

Now with reference test

[Andreas Kling]

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.

[Benjamin Poulain]

Closing: fixed a long time ago.

[Allan Sandfeld Jensen]

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.

[Benjamin Poulain]

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

[Allan Sandfeld Jensen]

Ah great. You probably want to close all the bugs this depends on as well then. They are all different cases of this bug.

[Philippe Wittenbergh]

Great work Benjamin! Thanks. All my test cases work correctly. I've marked bug 88219 as fixed as it now works fine.