Closed Bug 1367975 Opened 7 years ago Closed 7 years ago

stylo: checking important rule change seems to be broken

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Bug 1367975 - Call RequestRestyle(Layer) when specified timing is changed for CSS animations.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Servo PR
41 bytes, text/x-github-pull-request
Details | Review 
In replace_rule_nodes, if update_rule_at_level() returns Some(), we consider that important rules were changed inside the function, but update_rule_at_level() returns even when no important rule changed [1].  We should only see the result inside .any_important() 'if' block.

[1] https://hg.mozilla.org/mozilla-central/file/5d6fe59a9a5d/servo/components/style/rule_tree/mod.rs#l329
Priority: -- → P2
Blocks: stylo-perf
Hiro, why is this a performance issue?
Flags: needinfo?(hikezoe)
That's because we create SequentialTask even if we don't need it.  When we have animations and if we change !important rule for the animating element, we need to create a SequentialTask to update the animation properties.  but on the current setup we create the SequentialTask there is no !important rule changes.

For example, if |element| has a CSS background color animation, whenever we change style for the element, we create a SequentialTask and update the background color animation.
Flags: needinfo?(hikezoe)
Dropping important rules case were not covered. Also there seems to be other cases that rely on this incorrect important rules changes.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c2b529e6d53aba5fb2815436b47d451413a2960&selectedJob=119820777
Some OMTA test cases rely on RequestRestyle(Standard) which caused by the important rule changes.

A typical failure case is here [1], it's changing animation direction from normal to reverse. I think those cases should be handled in UpdateEffectProperties().

Boris, did you see any similar problem when you implemented OMTA?  I think animation generation does not work properly.

[1] https://hg.mozilla.org/mozilla-central/file/44121dbcac6a/layout/style/test/test_animations_omta.html#l1010
Flags: needinfo?(boris.chiou)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> Some OMTA test cases rely on RequestRestyle(Standard) which caused by the
> important rule changes.
> 
> A typical failure case is here [1], it's changing animation direction from
> normal to reverse. I think those cases should be handled in
> UpdateEffectProperties().
> 
> Boris, did you see any similar problem when you implemented OMTA?  I think
> animation generation does not work properly.

Three similar problems:
1. (Bug 1334036) While working on OMTA, I have to make sure UpdateCascadeResults() is called when we update/remove/add the important ruless. yes, I think this is also related to UpdateEffectProperties() we called from SequentialTask.
2. (Bug 1361938) We don't update direction or play-state immediately in test_animations_omta.html, so the test is failed. However, I think you already fix it.
3. (Bug 1334036) Some changes to animations don't affect the computed style and yet still require the layer to be updated. Therefore, we also need to call AddLayerChangesForAnimation in ServoRestyleManager.

Maybe this is a case we didn't notice before. :(
Flags: needinfo?(boris.chiou)
Thank you Boris, I did totally forget that similar test cases failed on the decl block bug (bug 1361938).
I did make a mistake that the case, animation-direction change should be handled in UpdateAnimations. Anyway, with either ways we don't properly request restyle for layers for the case.
Brian, do you remember where we ensured to request restyle for layer when specified timing params are changed for CSS animations?
I thought it's AnimationEffectReadOnly::SetSpecifiedTiming(), but it seems not. Also looked at KeyframeEffectReadOnly::NotifyAnimationTimingUpdated(), but we request restyle there for throttled or standard.

I think we should request restyle layers for the case where the specified timing params are changed, just like we do it in KeyframeEffect::NotifySpecifiedTimingUpdated().  Am I missing something?
Flags: needinfo?(bbirtles)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> Brian, do you remember where we ensured to request restyle for layer when
> specified timing params are changed for CSS animations?
> I thought it's AnimationEffectReadOnly::SetSpecifiedTiming(), but it seems
> not. Also looked at KeyframeEffectReadOnly::NotifyAnimationTimingUpdated(),
> but we request restyle there for throttled or standard.

When that happens, I think we should be updating the animation generation and then updating layers in RestyleManager::AddLayerChangesForAnimation.
Flags: needinfo?(bbirtles)
Ok, in case of gecko we don't request restyle layer when animation-direction is changed.  We call nsIFrame::SchedulePaint() instead.

The call stack at that time;

#0  nsIFrame::SchedulePaint (this=0x7fffd1ae2cf8, aType=nsIFrame::PAINT_DEFAULT) at /home/ikezoe/gecko/layout/generic/nsFrame.cpp:6682
#1  0x00007fffe3a73c14 in mozilla::DoApplyRenderingChangeToTree (aFrame=0x7fffd1ae2cf8, aChange=nsChangeHint_UpdateOpacityLayer)
    at /home/ikezoe/gecko/layout/base/RestyleManager.cpp:1109
#2  0x00007fffe3a740a3 in mozilla::ApplyRenderingChangeToTree (aPresShell=0x7fffd19da000, aFrame=0x7fffd1ae2cf8, aChange=nsChangeHint_UpdateOpacityLayer)
    at /home/ikezoe/gecko/layout/base/RestyleManager.cpp:1193
#3  0x00007fffe3a755eb in mozilla::RestyleManager::ProcessRestyledFrames (this=0x7fffbe495500, aChangeList=...) at /home/ikezoe/gecko/layout/base/RestyleManager.cpp:1608
#4  0x00007fffe3a44ac0 in mozilla::GeckoRestyleManager::ComputeAndProcessStyleChange (this=0x7fffbe495500, aFrame=0x7fffd1ae2cf8, aMinChange=nsChangeHint_Empty, aRestyleTracker=...,-
    aRestyleHint=eRestyle_StyleAttribute, aRestyleHintData=...) at /home/ikezoe/gecko/layout/base/GeckoRestyleManager.cpp:3557
#5  0x00007fffe3a38084 in mozilla::GeckoRestyleManager::RestyleElement (this=0x7fffbe495500, aElement=0x7fffbdc4f9d0, aPrimaryFrame=0x7fffd1ae2cf8, aMinHint=nsChangeHint_Empty,-
    aRestyleTracker=..., aRestyleHint=eRestyle_StyleAttribute, aRestyleHintData=...) at /home/ikezoe/gecko/layout/base/GeckoRestyleManager.cpp:229
#6  0x00007fffe3a87012 in mozilla::RestyleTracker::ProcessOneRestyle

In GeckoRestyleManager::ComputeAndProcessStyleChange (frame#4), we call ElementRestyler::ComputeStyleChangeFor(), as a result, we get nsChangeHint_UpdateOpacityLayer, it leads to the SchedulePaint.

Also, a surprise thing is that we call SchedulePaint even if we change unrelated style attribute, e.g. color.  This might be the cause of bug 1307341.
Anyway, I am leaving the gecko problem here, and call RequestRestyle(Layer) in AnimationEffectReadOnly::SetSpecifiedTiming().
It looks like bug 1293806 regressed this.
(In reply to Brian Birtles (:birtles, away 31 July~7 Aug) from comment #11)
> It looks like bug 1293806 regressed this.

Oh right, my fault.. I did search RequestRestyle() instead.
Priority: P2 → --
Comment on attachment 8892691 [details]
Bug 1367975 - Make replace_rules_internal return true only if important rules changed.

https://reviewboard.mozilla.org/r/163676/#review169114

::: servo/components/style/matching.rs:718
(Diff revision 1)
>              match new_node {
>                  Some(n) => {
>                      *path = n;
> -                    level.is_important()
> +                    important_rules_changed
>                  },
>                  None => false,

Due to my next comment, this should become:

```
if let Some(n) = new_node {
    *path = n;
}
important_rules_changed
```

::: servo/components/style/rule_tree/mod.rs:363
(Diff revision 1)
>                      debug!("Picking the fast path in rule replacement");
>                      return None;
>                  }
>              }
> +            if level.is_important() {
> +                *important_rules_changed = true;

I think this should happen before the early-return above for correctness. I think it doesn't matter for us in this case, but the fast path in rule replacement is still a replacement :)
Attachment #8892691 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8892690 [details]
Bug 1367975 - Call RequestRestyle(Layer) when specified timing is changed for CSS animations.

https://reviewboard.mozilla.org/r/163674/#review169180
Attachment #8892690 - Flags: review?(bbirtles) → review+
Attached file Servo PR — 
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED

    
    

    
      
  

    
  

        Attachment #8892691 -
        Attachment is obsolete: true

    
    

    
  
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b47507e113e3
Call RequestRestyle(Layer) when specified timing is changed for CSS animations. r=birtles

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/b47507e113e3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago

          status-firefox57:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: