Closed Bug 1382136 Opened 7 years ago Closed 7 years ago

stylo: make 'content' animatable

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

People

(Reporter: daisuke, Assigned: daisuke)

References

Details

Attachments

(5 files, 3 obsolete files)

Bug 1382136 - Part 3: remove test fail annotations from meta in wpt.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1382136 - Part 4: add a reftest for 'content' animation on pseudo element.
59 bytes, text/x-review-board-request
hiro
: review+
Details
pseudo-cssanimation.html
308 bytes, text/html
Details
pseudo-style-fix-in-reframe
2.23 KB, text/plain
Details
Servo PR 18085
41 bytes, text/x-github-pull-request
Details | Review 
'content' property should animate discretely.
Oh, got crash in reftest. ( layout/style/test/test_counter_style.html )
Will investigate.
(In reply to Daisuke Akatsuka (:daisuke) from comment #7)
> Oh, got crash in reftest. ( layout/style/test/test_counter_style.html )

Not reftest. Correctly it is mochitest.
Hi Cameron,
I'm sorry to bother you.

I'd like to get your advice about patch 3.
https://reviewboard.mozilla.org/r/161906/diff/1#index_header

I got an advice that we should not set restyle damage for parent in here at least from Hiro on IRC. 
How should we implement?
Flags: needinfo?(cam)
It seems wrong that we have to do this.  We should normally generate a ReconstructFrame when calling CalcStyleDifference on the pseudo's old and new style.  Does that not work in some case?
Flags: needinfo?(cam) → needinfo?(dakatsuka)
(In reply to Cameron McCormack (:heycam) from comment #10)
> It seems wrong that we have to do this.  We should normally generate a
> ReconstructFrame when calling CalcStyleDifference on the pseudo's old and
> new style.  Does that not work in some case?

Thanks Cameron.

Yes, we have the case when we can not get nsStyleContext by existing_style_for_restyle_damage in here[1].
I attached a test case.
When run the attached test by "mach run pseudo-cssanimation.html", we get ReconstructFrame hint two times during animation. One is at the animation starting, another one is when the content changes to "to".
However, after that, if we reload the page, we could not get the first hint. Because the nsStyleContext is null at the animation starting since the frame for pseudo element is not constructed yet ( The context get from frame in here[2] ). 
We could get the hint correctly at the second timing which 'content' changes however.
So should we find a cause why the behavior was changed? Or, how to get nsStyleContext, or the way to calculating the diff?

[1] https://searchfox.org/mozilla-central/source/servo/components/style/matching.rs#770
[2] https://searchfox.org/mozilla-central/source/layout/style/ServoBindings.cpp#389
Flags: needinfo?(dakatsuka) → needinfo?(cam)
Emilio should be able to answer your question more quickly than me.
Flags: needinfo?(cam) → needinfo?(emilio+bugs)
(In reply to Cameron McCormack (:heycam) from comment #10)
> It seems wrong that we have to do this.  We should normally generate a
> ReconstructFrame when calling CalcStyleDifference on the pseudo's old and
> new style.  Does that not work in some case?

It's not only wrong, but doing it from there is racy too.

(In reply to Daisuke Akatsuka (:daisuke) from comment #12)
> (In reply to Cameron McCormack (:heycam) from comment #10)
> > It seems wrong that we have to do this.  We should normally generate a
> > ReconstructFrame when calling CalcStyleDifference on the pseudo's old and
> > new style.  Does that not work in some case?
> 
> Yes, we have the case when we can not get nsStyleContext by
> existing_style_for_restyle_damage in here[1].
> I attached a test case.
> When run the attached test by "mach run pseudo-cssanimation.html", we get
> ReconstructFrame hint two times during animation. One is at the animation
> starting, another one is when the content changes to "to".
> However, after that, if we reload the page, we could not get the first hint.
> Because the nsStyleContext is null at the animation starting since the frame
> for pseudo element is not constructed yet ( The context get from frame in
> here[2] ). 
> We could get the hint correctly at the second timing which 'content' changes
> however.
> So should we find a cause why the behavior was changed? Or, how to get
> nsStyleContext, or the way to calculating the diff?

So I suspect a lot that you're hitting this path[1] when processing the pseudo-element, may that be true?

If it is, we really need to get that fixed. If it's not, it'd be nice to know which paths are taken when, and what's the value of the traversal_flags.for_animation_only() on each one.

[1]: https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/servo/components/style/matching.rs#836
Flags: needinfo?(emilio+bugs)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #14)
> (In reply to Daisuke Akatsuka (:daisuke) from comment #12)
> > Yes, we have the case when we can not get nsStyleContext by
> > existing_style_for_restyle_damage in here[1].
> > I attached a test case.
> > When run the attached test by "mach run pseudo-cssanimation.html", we get
> > ReconstructFrame hint two times during animation. One is at the animation
> > starting, another one is when the content changes to "to".
> > However, after that, if we reload the page, we could not get the first hint.
> > Because the nsStyleContext is null at the animation starting since the frame
> > for pseudo element is not constructed yet ( The context get from frame in
> > here[2] ). 
> > We could get the hint correctly at the second timing which 'content' changes
> > however.
> > So should we find a cause why the behavior was changed? Or, how to get
> > nsStyleContext, or the way to calculating the diff?
> 
> So I suspect a lot that you're hitting this path[1] when processing the
> pseudo-element, may that be true?
> 
> If it is, we really need to get that fixed. If it's not, it'd be nice to
> know which paths are taken when, and what's the value of the
> traversal_flags.for_animation_only() on each one.
> 
> [1]:
> https://searchfox.org/mozilla-central/rev/
> 09c065976fd4f18d4ad764d7cb4bbc684bf56714/servo/components/style/matching.
> rs#836

Thank you very much, Emilio!
Yes, passed through the path in case of we could not get ReconstructFrame hint.
So, can we include the result of 'content' change to needs_reconstruction flag?
Then, although we need to update the parent style, where should we do? In finish_restyle?
Flags: needinfo?(emilio+bugs)
(In reply to Daisuke Akatsuka (:daisuke) from comment #15)
> Thank you very much, Emilio!
> Yes, passed through the path in case of we could not get ReconstructFrame
> hint.
> So, can we include the result of 'content' change to needs_reconstruction
> flag?

We should either unconditionally return reconstruct(), or properly call CalcStyleDifference. Cam and Hiro are more familiar on why that hack is there. Seems like a big wallpaper to me.

> Then, although we need to update the parent style, where should we do? In
> finish_restyle?

We don't (and can't, because it'd be racy) update the parent style from there...

Which is unfortunate, I guess. In general, we only run animations for pseudos that "exist". The way we handle changing content right now is reframing, which effectively means "destroy the pseudo-element, and generate another new one".

I _think_ there's no particular reason changing content from "foo" to "bar" should trigger a reframe of the whole parent element though, seems like reconstructing the inner frames of the pseudo should just work...

I don't have an immediate idea to make that work properly, which isn't either:

 * Process ::before and ::after at the same time as the parent during the traversal. This is slightly tricky and requires you to skip them on the traversal iterator. This would allow you to update the style on the parent, but that'd be still a hack and will break other places where we currently assume it doesn't contain animations.

 * Process animation rules on the parent too. We can't really do this properly because we post animation restyles for the pseudo-element itself, which makes sense. Also, this will do a lot of duplicated work, which isn't fun.

 * Don't reconstruct frames for the parent of the pseudo if the content property goes from something that isn't none -> something, or something -> none. I suspect this is the best way to go. The only thing we really want to do in that case is "reframe the children" (of the pseudo-element), and I suspect we reframe the whole thing just because nobody felt the need to do the fast thing (because in practice it's quite uncommon). I _really_ think this is the less hacky and better solution. 

Maybe Cam or Hiro have other ideas. Also, maybe check with bz or dbaron on whether my last point is sane, but I think it is.

Finally, just a curious question without having read the patches, what is it supposed to happen when animating to / from content: none? Also, do other browsers support this?
Flags: needinfo?(emilio+bugs)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #16)
>  * Don't reconstruct frames for the parent of the pseudo if the content
> property goes from something that isn't none -> something, or something ->
> none. I suspect this is the best way to go. The only thing we really want to
> do in that case is "reframe the children" (of the pseudo-element), and I
> suspect we reframe the whole thing just because nobody felt the need to do
> the fast thing (because in practice it's quite uncommon). I _really_ think
> this is the less hacky and better solution. 

Actually, I guess even this wouldn't really work (well, would work with this particular case, but not in the general case).

I bet right now we lose animation information from the pseudo if we, for example, change the pseudo's "position" property, which would cause it to reframe.

I guess the best thing to do is preserving the pseudo-element across reframes if needed, which doesn't sound particularly insane now that the pseudos are referenced from content properties (we'd just need to make those properties own the pseudo instead of the frame properties), but I remember trying to do that in bug 1355351 and failing because of different stuff like bug 1359656 and similar stuff mentioned in bug 1355351.

Maybe I could try to do that, but that's a fair amount of work.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #16)

> Finally, just a curious question without having read the patches, what is it
> supposed to happen when animating to / from content: none? Also, do other
> browsers support this?

I tested using this code[1] for Firefox release 54.0.1, Chrome canary 62.0.3172.0 and Safari preview 36.
The result is following:
                        |      Firefox        |      Chrome         |   Safari    |
-----------------------------------------------------------------------------------
| none in from keyframe |  invisible -> 'to'  |     invisible       | not animate |
| none in to keyframe   | 'from' -> invisible | 'from' -> invisible | not animate |
| specified content     |    same to above    |   same to adobe     | not animate |
| not specified content |    same to above    |     invisible       | not animate |
-----------------------------------------------------------------------------------

I think, we expect Firefox behavior which regardless of whether the content properties are specified from the beginning, content can animate. Also, if set 'none' to keyframe, should gone the content.
[1] https://jsbin.com/qedevel/edit?html,css,output
(In reply to Daisuke Akatsuka (:daisuke) from comment #18)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #16)
> 
> > Finally, just a curious question without having read the patches, what is it
> > supposed to happen when animating to / from content: none? Also, do other
> > browsers support this?
> 
> I tested using this code[1] for Firefox release 54.0.1, Chrome canary
> 62.0.3172.0 and Safari preview 36.
> The result is following:
>                         |      Firefox        |      Chrome         |  
> Safari    |
> -----------------------------------------------------------------------------
> ------
> | none in from keyframe |  invisible -> 'to'  |     invisible       | not
> animate |
> | none in to keyframe   | 'from' -> invisible | 'from' -> invisible | not
> animate |
> | specified content     |    same to above    |   same to adobe     | not
> animate |
> | not specified content |    same to above    |     invisible       | not
> animate |
> -----------------------------------------------------------------------------
> ------
> 
> I think, we expect Firefox behavior which regardless of whether the content
> properties are specified from the beginning, content can animate. Also, if
> set 'none' to keyframe, should gone the content.
> [1] https://jsbin.com/qedevel/edit?html,css,output

It seems to me that the Firefox behavior relies on the fact that firefox creates CSS animations on pseudo element even if content property is none.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #19)
> It seems to me that the Firefox behavior relies on the fact that firefox
> creates CSS animations on pseudo element even if content property is none.

Indeed, and we agreed that's a bug, last time I checked, right?
Hmm... We're getting _so_ confused.

The page has some CSS that reads like:

.on-loading-complete .mod-load-indicator--mask{opacity:0;visibility:hidden;transition:300ms 500ms}.mod-flexbox{display:-webkit-flex;display:-ms-flexbox;display:flex;-webkit-flex-direction:row;-moz-flex-direction:row;-ms-flex-direction:row;flex-direction:row;-webkit-flex-wrap:wrap;-ms-flex-wrap:wrap;flex-wrap:wrap;margin-left:-10px;margin-right:-10px}

We're styling the |.on-loading-complete .mod-load-indicator--mask|, but somehow applying the .mod-flexbox rule... wtf.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #21)
> Hmm... We're getting _so_ confused.
> 
> The page has some CSS that reads like:
> 
> .on-loading-complete
> .mod-load-indicator--mask{opacity:0;visibility:hidden;transition:300ms
> 500ms}.mod-flexbox{display:-webkit-flex;display:-ms-flexbox;display:flex;-
> webkit-flex-direction:row;-moz-flex-direction:row;-ms-flex-direction:row;
> flex-direction:row;-webkit-flex-wrap:wrap;-ms-flex-wrap:wrap;flex-wrap:wrap;
> margin-left:-10px;margin-right:-10px}
> 
> We're styling the |.on-loading-complete .mod-load-indicator--mask|, but
> somehow applying the .mod-flexbox rule... wtf.

Whoops, wrong bug.
Priority: -- → P2
I did dig into this in detail.  As far as I can tell our reframe machinery mostly works fine for the content property animations (Though I agree Emilio's view, we shouldn't do reframe when content property changes (other than none)).  A problem is we create child elements for content property with *pseudo style owned by the parent* [1].  (Note that, unfortunately, the pseudo style does not contain animation styles at all, whereas the other pseudo style which contains animation styles was cleared at this moment because of reframe!)  I have no idea how to solve this situation since I don't have a good sense of frame construction.

[1] https://hg.mozilla.org/mozilla-central/file/a4a448ba7f18/layout/base/nsCSSFrameConstructor.cpp#l1972
I think this should work. Also I noticed, we should check the pseudo element has script animations as well.
Comment on attachment 8890702 [details]
Bug 1382136 - Part 3: remove test fail annotations from meta in wpt.

https://reviewboard.mozilla.org/r/161908/#review172000
Attachment #8890702 - Flags: review?(hikezoe) → review+
Comment on attachment 8890703 [details]
Bug 1382136 - Part 4: add a reftest for 'content' animation on pseudo element.

https://reviewboard.mozilla.org/r/161910/#review172002

These reftest should use reftest-wait or set animation-play-state: paused.  Also 1s is too short for reftest we should use 100s instead.
Attachment #8890703 - Flags: review?(hikezoe)
Comment on attachment 8890699 [details]
Bug 1382136 - Part 1: Implement clone_content method.

https://reviewboard.mozilla.org/r/161902/#review172004

Clearing review request since this has been already landed with list-style-type animatable patches.
Attachment #8890699 - Flags: review?(hikezoe)
Comment on attachment 8890701 [details]
Bug 1382136 - Part 3: address for peudo element.

https://reviewboard.mozilla.org/r/161906/#review172006

As per discusstion in this bug.
Attachment #8890701 - Flags: review?(hikezoe) → review-
Comment on attachment 8890700 [details]
Bug 1382136 - Part 2: Make content animatable.

https://reviewboard.mozilla.org/r/161904/#review172010

This needs to be revised based on current trunk.
Attachment #8890700 - Flags: review?(hikezoe)
Depends on: 1389440
(In reply to Hiroyuki Ikezoe (:hiro) from comment #29)
> Comment on attachment 8890700 [details]
> Bug 1382136 - Part 2: make content property animatable.
> 
> https://reviewboard.mozilla.org/r/161904/#review172010
> 
> This needs to be revised based on current trunk.

Split out this part in bug 1389440.
Attachment #8890701 - Attachment is obsolete: true
Thank you very much, Hiro.
Your landed patch works perfectly.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7863b0e256b032f691161be22cdfcca552e8094c
Comment on attachment 8890703 [details]
Bug 1382136 - Part 4: add a reftest for 'content' animation on pseudo element.

https://reviewboard.mozilla.org/r/161910/#review173182

I think we can use the same content value for the reference file to use opposite from and to values respectively. I.e. from: '' and to: 'content', and from: 'content' and to: ''.
Attachment #8890703 - Flags: review?(hikezoe) → review+
Comment on attachment 8890700 [details]
Bug 1382136 - Part 2: Make content animatable.

https://reviewboard.mozilla.org/r/161904/#review173184
Attachment #8890700 - Flags: review?(hikezoe) → review+
Comment on attachment 8890699 [details]
Bug 1382136 - Part 1: Implement clone_content method.

https://reviewboard.mozilla.org/r/161902/#review173186

::: commit-message-756aa:3
(Diff revision 2)
> +clone_content is needed to animate CSS Transitions, and to get computed
> +inherited value if keyframe has 'inherit'.

Why is this comment necessary for content property?  We didn't leave such comment ever.

::: servo/components/style/properties/gecko.mako.rs:5618
(Diff revision 2)
> +        unsafe fn string_from_char_array(chars: &*mut u16) -> String {
> +            let data = *chars as *const u16;
> +            let mut length = 0;
> +            let mut iter = data;
> +            while *iter != 0 {
> +                length += 1;
> +                iter = iter.offset(1);
> +            }
> +            let char_vec = ::std::slice::from_raw_parts(data, length as usize);
> +            String::from_utf16(char_vec).unwrap()
> +        }

This is pretty similar to string_from_ns_string_buffer in media_queries.rs. We should unify them.

::: servo/components/style/properties/gecko.mako.rs:5659
(Diff revision 2)
> +                        let gecko_function =
> +                            unsafe { &**gecko_content.mContent.mCounters.as_ref() };
> +                        let ident = gecko_function.mIdent.to_string();
> +                        let style =
> +                            CounterStyleOrNone::from_gecko_value(&gecko_function.mCounterStyle);

These code should be factored out as a closure so that we can reuse it for Counters type.
Attachment #8890699 - Flags: review?(hikezoe) → review+
Attached file Servo PR 18085 — 

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8890699 -
        Attachment is obsolete: true

    
  

        Attachment #8890700 -
        Attachment is obsolete: true

    
    

    
  
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 41c9d0932804 -d 91e40a0271ce: rebasing 413547:41c9d0932804 "Bug 1382136 - Part 3: remove test fail annotations from meta in wpt. r=hiro"
merging testing/web-platform/meta/web-animations/animation-model/animation-types/accumulation-per-property.html.ini
merging testing/web-platform/meta/web-animations/animation-model/animation-types/addition-per-property.html.ini
warning: conflicts while merging testing/web-platform/meta/web-animations/animation-model/animation-types/accumulation-per-property.html.ini! (edit, then use 'hg resolve --mark')
warning: conflicts while merging testing/web-platform/meta/web-animations/animation-model/animation-types/addition-per-property.html.ini! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d2a4cd0ea444
Part 3: remove test fail annotations from meta in wpt. r=hiro
https://hg.mozilla.org/integration/autoland/rev/6b7f728e5684
Part 4: add a reftest for 'content' animation on pseudo element. r=hiro

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/d2a4cd0ea444
https://hg.mozilla.org/mozilla-central/rev/6b7f728e5684
Status: NEW → 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: