1338982 - stylo: NODE_HAS_SLOW_SELECTOR_LATER_SIBLINGS is not set correctly

Status: RESOLVED DUPLICATE of bug 1345950

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Cameron McCormack (:heycam)]

Attachment: test

Here is a test case that fails due to not setting NODE_HAS_SLOW_SELECTOR_LATER_SIBLINGS.  It also fails in Servo.

Comment 2

[Cameron McCormack (:heycam)]

Attachment: test 2

Comment 3

[Cameron McCormack (:heycam)]

Bobby, is there any fundamental reason why we can't allow RESTYLE_LATER_SIBLINGS to be stored in an element's RestyleData?  Currently we allow snapshot expansion to produce RESTYLE_LATER_SIBLINGS, which we then notice and return back up to preprocess_children to expand out into (RESTYLE_SELF | RESTYLE_DESCENDANTS) on the later siblings.  But we don't allow an eRestyleHint_LaterSiblings to be posted explicitly, which RestyleManager wants to do to handle DOM mutations.  We could make RestyleManager post restyles for the later siblings manually, but that seems wasteful.

I guess it would make it hard for has_current_styles() to tell, though.

Comment 4

[Cameron McCormack (:heycam)]

Although I guess that is already not accurate if a previous sibling has a snapshot that would expand to include RESTYLE_LATER_SIBLINGS?

Comment 5

[Cameron McCormack (:heycam)]

Attachment: WIP v0.1

This fixes the "test 2" attachment.

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8836643 [details] [diff] [review]
WIP v0.1

Review of attachment 8836643 [details] [diff] [review]:
-----------------------------------------------------------------

I think we should move this into StoredRestyleHint. Also, probably worth separating the selectors change into a separate commit and getting Simon or emilio to review it (I don't have the codepaths paged in enough to be sure that this is the right place for the check).

Comment 7

[Bobby Holley (:bholley)]

Emilio, is this fixed with your recent latersiblings work?

Comment 8

[Emilio Cobos Álvarez (:emilio)]

There's no chance the test case number 1 works on stylo because we don't properly implement :empty.

That being said, it doesn't work on servo too. I'll check if the patch fixes it on servo.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

So, right now only the selectors change is needed to fix that. The change doesn't fix the first test case, but that's just because it doesn't handle the HAS_EMPTY_SELECTOR flag.

Now, the fun stuff. Thinking whether that was the correct place to put the flag, I just realized that all the flag handling introduced in bug 1336646 is broken. Sorry, should've catched that during review :(

Mainly, the way we're doing it right now is that we get this `flags` outparam and apply it to the _original_ element we're matching again, but these selector flags can go arbitrarily around the DOM, for example:

.foo + bar baz baz baz {
    color: red;
}

The flag for the slow selector should be inserted in the element we're trying to match the first `bar` again, which may be an arbitrary ancestor or sibling, etc. of the element we're matching against.

The best idea I have right now is using an outparam Vec<(Element, Flags)>, but that sounds quite terrible :(.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

I guess the same is true for relations, but since they're only used for style sharing during the same restyle the worst thing that may happen is that we count more relations than needed.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

The fix for the relations thing is straight-forward, but no so much for the element flags. I plan to fix the relations once I have the style cache working in sequential mode, because of test coverage.

Comment 12

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> So, right now only the selectors change is needed to fix that. The change
> doesn't fix the first test case, but that's just because it doesn't handle
> the HAS_EMPTY_SELECTOR flag.
> 
> Now, the fun stuff. Thinking whether that was the correct place to put the
> flag, I just realized that all the flag handling introduced in bug 1336646
> is broken. Sorry, should've catched that during review :(
> 
> Mainly, the way we're doing it right now is that we get this `flags`
> outparam and apply it to the _original_ element we're matching again, but
> these selector flags can go arbitrarily around the DOM, for example:
> 
> .foo + bar baz baz baz {
>     color: red;
> }
> 
> The flag for the slow selector should be inserted in the element we're
> trying to match the first `bar` again, which may be an arbitrary ancestor or
> sibling, etc. of the element we're matching against.

I don't think the selector above would require any slow selectors flags (the later siblings restyle hint should take care of that). But in general I see your point. :-(


I've filed bug 1345950 for this issue.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #12) 
> > .foo + bar baz baz baz {
> >     color: red;
> > }
> > 
> > The flag for the slow selector should be inserted in the element we're
> > trying to match the first `bar` again, which may be an arbitrary ancestor or
> > sibling, etc. of the element we're matching against.
> 
> I don't think the selector above would require any slow selectors flags (the
> later siblings restyle hint should take care of that). But in general I see
> your point. :-(

How not? That would need the HAS_SLOW_SELECTOR_LATER_SIBLINGS flag, otherwise we don't post the LaterSiblings hint.

Comment 14

[Bobby Holley (:bholley)]

Yeah you're right - I was mixing it up with state/attr changes.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Self-reminder to look at the different tests skipped at https://hg.mozilla.org/integration/autoland/rev/ee95a66bea56e9b7044abab6f12b12257cb6b4b0, which this should fix.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

With the recent :empty/:any/etc support and the recent flags patch, we now pass test_bug534804.html and test_bug73586.html, which test this extensively, so I believe we can close this.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6c10860d02ad
Re-enable no longer timing-dependent tests. r=emilio

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6c10860d02ad