Bug 171883 - REGRESSION (r207372) Visibility property is not inherited when used in an animation
Summary: REGRESSION (r207372) Visibility property is not inherited when used in an ani...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: Safari 10
Hardware: All All
: P2 Critical
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-05-09 14:04 PDT by Malte Ubl
Modified: 2017-05-11 02:47 PDT (History)
10 users (show)

See Also:


Attachments
10.2 does not repro. Animated GIF showing the behavior. (75.81 KB, image/gif)
2017-05-09 14:46 PDT, Malte Ubl
no flags Details
patch (8.02 KB, patch)
2017-05-10 05:22 PDT, Antti Koivisto
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (816.39 KB, application/zip)
2017-05-10 06:30 PDT, Build Bot
no flags Details
patch (8.18 KB, patch)
2017-05-10 07:31 PDT, Antti Koivisto
simon.fraser: review+
Details | Formatted Diff | Diff
patch (8.24 KB, patch)
2017-05-10 09:10 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (8.50 KB, patch)
2017-05-10 14:58 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Malte Ubl 2017-05-09 14:04:22 PDT
It appears that the latest Safari stable release broke animation of CSS visibility.

Previously (like in other browser) visibility was correctly inherited by children. This is no longer the case.

Compare http://output.jsbin.com/yirehac/quiet between Safari versions (and Chrome and Firefox).

Repro case
<!doctype html>
<html>
<head>
  <style>
    
  body{
    animation:test 8s steps(1,end) 0s 1 normal both
  }
  @keyframes test {
    from{visibility:hidden}
    to{visibility:visible}
  }    
  body {
    background: #cfd8dc;
  }
  div {
    background: white; 
    width: 360px;
    height: 360px;
  }
  </style>  
</head>
<body>Hello, world.
  <div>
    Why am I shown?
  </div>
</body>
</html>

This bug leads to FOUC on many AMP pages (which at this point is a significant portion of internet traffic). It cannot be worked around in AMP as the relevant CSS is part of the distributed pages and not centrally managed. We can update the future recommendation, but the current 2 billion pages have the problem.
Comment 1 Radar WebKit Bug Importer 2017-05-09 14:26:17 PDT
<rdar://problem/32086550>
Comment 2 Simon Fraser (smfr) 2017-05-09 14:42:38 PDT
Is there a Safari version in which this bug did not occur? I can reproduce back to r210000.
Comment 3 Malte Ubl 2017-05-09 14:46:38 PDT
Created attachment 309545 [details]
10.2 does not repro. Animated GIF showing the behavior.
Comment 4 Simon Fraser (smfr) 2017-05-09 14:49:00 PDT
I was testing on Mac. It would be surprising if Mac and iOS behavior differed.
Comment 5 Malte Ubl 2017-05-09 14:51:45 PDT
Unfortunately I don't have a way to test on an older version of desktop Safari. It does repro in current mobile Safari and it does not repro in iOS 10.2 mobile Safari.
Comment 6 Simon Fraser (smfr) 2017-05-09 15:00:51 PDT
This regressed with https://trac.webkit.org/changeset/207372/webkit
Comment 7 Simon Fraser (smfr) 2017-05-09 15:02:36 PDT
I did ask about inherit with this change:
https://bugs.webkit.org/show_bug.cgi?id=163452#c3
Comment 8 Antti Koivisto 2017-05-09 15:57:22 PDT
I don't see 'inherit' in this test so I'm not sure what the relevance of that is.
Comment 9 Simon Fraser (smfr) 2017-05-09 16:23:51 PDT
This is possibly about the propagation of visibleContent bits in RenderLayers
Comment 10 Antti Koivisto 2017-05-09 16:30:06 PDT
Yeah, something like that seems likely. There shouldn't be anything special about visibility inheritance from style perspective.
Comment 11 Antti Koivisto 2017-05-10 03:23:45 PDT
Other inherited properties have the same problem. You can replace 'visibility' with 'color' here and get the same wrong behavior.
Comment 12 Antti Koivisto 2017-05-10 03:46:20 PDT
The problem here is that our animation code is tied to renderers. We don't have renderers during the initial style resolution so animations are not applied yet. When constructing renderers we set their style to the initial animated style but this step can't implement inheritance.

Normally this is invisible as the first animation frame will immediately inherit the style correctly. However in this case the animation is discrete and the first frame is the same as the initial state. With r207372 we optimize the descendant style change away.

There are simple fixes for this specific case. However at some point we should fix the underlying issues too.
Comment 13 Antti Koivisto 2017-05-10 05:22:28 PDT
Created attachment 309591 [details]
patch
Comment 14 Build Bot 2017-05-10 06:30:55 PDT
Comment on attachment 309591 [details]
patch

Attachment 309591 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3711640

New failing tests:
animations/animation-initial-inheritance.html
Comment 15 Build Bot 2017-05-10 06:30:56 PDT
Created attachment 309599 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 16 Antti Koivisto 2017-05-10 07:31:13 PDT
Created attachment 309603 [details]
patch
Comment 17 Simon Fraser (smfr) 2017-05-10 08:16:24 PDT
Comment on attachment 309603 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=309603&action=review

> Source/WebCore/style/StyleTreeResolver.cpp:264
> +            // FIXME: We should compute animated style correctly during initial style resolution when we don't have renderers yet.

Could you file a bug for this and reference it here please?
Comment 18 Antti Koivisto 2017-05-10 09:10:12 PDT
Created attachment 309612 [details]
patch
Comment 19 WebKit Commit Bot 2017-05-10 09:50:59 PDT
Comment on attachment 309612 [details]
patch

Clearing flags on attachment: 309612

Committed r216591: <http://trac.webkit.org/changeset/216591>
Comment 20 WebKit Commit Bot 2017-05-10 09:51:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Ryan Haddad 2017-05-10 14:14:22 PDT
Reverted r216591 for reason:

This change broke an internal build.

Committed r216619: <http://trac.webkit.org/changeset/216619>
Comment 22 Antti Koivisto 2017-05-10 14:58:33 PDT
Created attachment 309643 [details]
patch

updated SameSizeAsRenderElement struct to accommodate an additional bit
Comment 23 WebKit Commit Bot 2017-05-10 15:23:15 PDT
Comment on attachment 309643 [details]
patch

Clearing flags on attachment: 309643

Committed r216631: <http://trac.webkit.org/changeset/216631>
Comment 24 WebKit Commit Bot 2017-05-10 15:23:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Malte Ubl 2017-05-11 02:47:53 PDT
Thanks for the super quick fix!