522563 - ReResolveStyleContext should give continuations the same style rather than rerunning selector matching

Status: RESOLVED FIXED

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

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Right now ReResolveStyleContext reruns selector matching for each continuation.  We should instead make it give continuations the same style as their previous continuation.

This code should also assert that they have the same style context before the reresolution.  (i.e., before the reresolve, compare to next-continuation; during reresolve, get new style from prev-continuation).

We should also comment that in the future we may want to make both of these checks conditional on having the same parent style context, since we'd need that if we ever make :first-letter/:first-line be continuations rather than separate frames.  (Though, actually, I should check that we don't already do this for :first-line.)


This bug is being filed as a followup from bug 435441 comment 87:

>>+nsTransitionManager::StyleContextChanged(nsIContent *aElement,
>
>>+  // FIXME: When we have multiple continuations, we actually repeat this
>>+  // for each one, and if we have transitions we create separate cover
>>+  // rules for each one.  However, since we're attaching the transition
>>+  // data to the element, during the animation we create the same style
>>+  // rule, so it's not too horrible.
>
>I'm not sure I follow.... We'd create a bunch of separate cover rules, agreed. 
>Won't we then end up giving the different continuations different style
>contexts (pointing to these different cover rules)?  That doesn't sound so
>great...  Am I missing something?  I guess this explosion of style contexts is
>just temporary until we process the RestyleForAnimation restyle, but still.... 
>I guess this is ok for now, but we should consider a followup to prevent this.
>
>As a separate note, do we really redo selector matching for every continuation
>when doing ReResolveStyleContext?  It looks like we do, that sucks, and we
>should fix it.  Please file a followup bug?

and its response in bug 435441 comment 89:

>Yes, it's temporary until we restyle for animation immediately
>afterwards, and it would be fixed by better handling of continuations in
>ReResolveStyleContext (which I was actually somewhat surprised we didn't
>have).  So I think the thing to do is file that followup bug and
>otherwise leave it.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And when we do this we should remove the comments in StyleContextChanged metioned above (and perhaps replace them with assertions).

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> We should also comment that in the future we may want to make both of these
> checks conditional on having the same parent style context, since we'd need
> that if we ever make :first-letter/:first-line be continuations rather than
> separate frames.  (Though, actually, I should check that we don't already do
> this for :first-line.)

I actually needed to refresh my memory on how we do :first-letter and :first-line:  in both cases (although not reliably for :first-letter, I think), we use continuations both for the letter/line frames and for their descendants.  The letter and line frames distinguish whether they have the style or not by using different pseudo-types.

This means that this patch needs to distinguish both by parent style context and by pseudo-type.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

I couldn't see an obvious way to have an assertion in nsTransitionManager, since StyleContextChanged doesn't get a frame passed to it.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And this passed try server unit tests, modulo hitting bug 522819 on Windows.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 406773 [details] [diff] [review]
patch

That's actually... surpisingly painless. r=bzbarsky

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/886ab0434035