Bug 12520

Created attachment 12843 [details] testcase

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.

Created attachment 15978 [details] Simple patch This is a direct patch of the problem without introducing new dependency data.

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

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

Created attachment 18437 [details] test case I think the bug demonstrated in the attached test case is related to the same bug.

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

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

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

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

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

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.

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.

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.

Created attachment 158759 [details] Patch

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

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

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

Created attachment 160706 [details] Patch Changed test-case to be text-dumped

Created attachment 168720 [details] Patch Rebased

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

Created attachment 170392 [details] Patch Now with reference test

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.

Closing: fixed a long time ago.

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.

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

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

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