541590 - Default nsPresContext::mProcessingAnimationStyleChange to TRUE instead of FALSE

Status: RESOLVED WONTFIX

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: fix v1

The variable nsPresContext::mProcessingAnimationStyleChange tells us whether we're doing a normal restyle vs. a non-animation restyle.[1]

Currently, this flag defaults to the "normal" setting.

However, after bug 537139's patch, this means we have the wrong behavior when trying to resolve SMIL-animated properties in "display:none" content.  (because we don't have any frames, and our nsStyleContexts are temporary and unaffected by posted restyle events)

To fix this, we need to reverse this flag's default -- so that we assume, by default, that we're doing animation restyles.  Wherever we *want* a non-animation restyle (right now, just in nsCSSFrameConstructor::ProcessPendingRestyles), we can temporarily toggle this flag off.

Patch attached.  I'll put it through tryserver and then request review if all is well.

[1] "Animation restyles" see transition-animated and SMIL-animated style, and will not trigger new transitions (after bug 537139 is fixed).  "Non-animation restyles" ignore transition-animated and SMIL-animated style (again, after bug 537139 is fixed), and can trigger new transitions.

Comment 1

[Daniel Holbert [:dholbert]]

(NOTE: Since I'm already tweaking the nsPresContext initializer list, I also removed some redundant initializations of members to PR_FALSE -- see the comment about "nsPresContext::operator new()" in the patch context.)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v2

First version failed a few transitions reftests and mochitests.  Needed to toggle the mProcessingAnimationStyleChange flag when attributes change, to make sure we request a *non*-animation restyle for those.

I also needed and mark the "to_none_test" test-bundle as passing, in test_transitions.html.  Note that the "from_none_test" and "always_none_test" (which actually are identical right now, I think?) still fail, so I'm leaving those ones as "todo" for now.

Comment 3

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

Hmmm.  The need for those changes makes me wonder if we really need two booleans; one that says whether we're in the middle of any restyle processing, and another that says, if we are, whether it's for animation or not.

Comment 4

[Daniel Holbert [:dholbert]]

So then, assuming we use the two-boolean strategy from comment 3, I think we'd need the following changes:

  - When PostRestyleEvent calls PostRestyleEventCommon, it'd now use both bools combined as its |aForAnimation| argument -- it'd pass (isInRestyleProcessing && isProcessingAnimationStyleChange).  This would handle the cases addressed by fix v2.

  - In bug 537139's patch, we'd want to always apply SMIL override style, except when we're doing non-animation-restyle-processing. That is:
  if (!isInRestyleProcessing || isAnimation) {
     // Apply override style
  }
(In particular, we'd need to apply the override style when we're not doing processing, in order to handle the 'display:none' case described in comment 0.)

Does that make sense?

Comment 5

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

Yes, though we might want to look through callers of PostRestyleEvent to see if others should be more careful and pass some other piece of information through.

Comment 6

[Daniel Holbert [:dholbert]]

Actually, I think the simplest solution is:
 - Leave the mProcessingAnimationStyleChange default as-is (i.e. WONTFIX this bug)

 - For bug 537139:
   * Add a second bool for "IsProcessingRestyles()" on the presContext.  (This bool only becomes true for the duration of nsCSSFrameConstructor::ProcessPendingRestyles.)
   * Check that bool in nsHTMLCSSStyleSheet::RulesMatching(), to satisfy the second half of comment 4.

The first half of comment 4 would still be satisfied, because "(isInRestyleProcessing && isProcessingAnimationStyleChange)" is the same as just "(isProcessingAnimationStyleChange)", as long as mIsProcessingAnimationStyleChange only ever becomes true during restyle processing (the situation we have right now on trunk).

Comment 7

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

Yeah, that sounds good.

Comment 8

[Daniel Holbert [:dholbert]]

Ok -- marking this as WONTFIX, per comment 6-7.  (I've posted an updated patch on bug 537139, too.)