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: NEW
: WebKit
CSS
: 420+
: All All
: P2 Normal
Assigned To:
:
: HasReduction
: 9279 12998 17680 18026 18027
: 93304
  Show dependency treegraph
 
Reported: 2007-01-31 19:49 PST by
Modified: 2014-02-05 11:04 PST (History)


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 PST, Allan Sandfeld Jensen
eric: review-
Review Patch | 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 PST, Stephanie Hobson
no flags Details
Patch (8.18 KB, patch)
2012-03-08 07:59 PST, Allan Sandfeld Jensen
no flags Review Patch | Details | Formatted Diff | Diff
Patch (9.64 KB, patch)
2012-08-16 02:52 PST, Allan Sandfeld Jensen
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-02 (378.96 KB, application/zip)
2012-08-16 04:55 PST, 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 PST, WebKit Review Bot
no flags Details
Patch (12.51 KB, patch)
2012-08-27 06:18 PST, Allan Sandfeld Jensen
no flags Review Patch | Details | Formatted Diff | Diff
Patch (9.77 KB, patch)
2012-10-15 08:43 PST, Allan Sandfeld Jensen
no flags Review Patch | Details | Formatted Diff | Diff
Patch (10.61 KB, patch)
2012-10-24 07:38 PST, Allan Sandfeld Jensen
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 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 From 2007-01-31 19:50:56 PST -------
Created an attachment (id=12843) [details]
testcase
------- Comment #2 From 2007-08-14 07:43:27 PST -------
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 From 2007-08-15 07:27:31 PST -------
Created an attachment (id=15978) [details]
Simple patch

This is a direct patch of the problem without introducing new dependency data.
------- Comment #4 From 2007-08-19 13:15:13 PST -------
(From update of attachment 15978 [details])
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 From 2007-10-07 02:06:20 PST -------
(From update of attachment 15978 [details])
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 From 2007-10-07 03:00:55 PST -------
The test also uses non-deterministic matching, so you need to apply the patch for #3428 first.
------- Comment #7 From 2008-01-14 07:12:19 PST -------
Created an attachment (id=18437) [details]
test case

I think the bug demonstrated in the attached test case is related to the same bug.
------- Comment #8 From 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 From 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 From 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 From 2008-03-20 11:13:18 PST -------
We still don't work right with the ~ selector or with multiple chained + selectors.  That's what is left now.
------- Comment #12 From 2008-03-23 14:01:50 PST -------
(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 From 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 From 2010-04-18 20:46:28 PST -------
Created an attachment (id=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 From 2012-03-08 07:59:58 PST -------
Created an attachment (id=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 From 2012-08-16 02:52:07 PST -------
Created an attachment (id=158759) [details]
Patch
------- Comment #17 From 2012-08-16 02:56:50 PST -------
(In reply to comment #16)
> Created an attachment (id=158759) [details] [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 From 2012-08-16 04:55:13 PST -------
(From update of attachment 158759 [details])
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 From 2012-08-16 04:55:20 PST -------
Created an attachment (id=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 From 2012-08-16 05:47:34 PST -------
(From update of attachment 158759 [details])
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 From 2012-08-16 05:47:41 PST -------
Created an attachment (id=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 From 2012-08-27 06:18:41 PST -------
Created an attachment (id=160706) [details]
Patch

Changed test-case to be text-dumped
------- Comment #23 From 2012-10-15 08:43:36 PST -------
Created an attachment (id=168720) [details]
Patch

Rebased
------- Comment #24 From 2012-10-15 10:52:36 PST -------
(From update of attachment 168720 [details])
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 From 2012-10-15 15:43:32 PST -------
(From update of attachment 168720 [details])
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 From 2012-10-24 07:38:33 PST -------
Created an attachment (id=170392) [details]
Patch

Now with reference test
------- Comment #27 From 2014-02-05 11:04:58 PST -------
(From update of attachment 170392 [details])
Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.