RESOLVED FIXED171883
REGRESSION (r207372) Visibility property is not inherited when used in an animation 
https://bugs.webkit.org/show_bug.cgi?id=171883
Summary REGRESSION (r207372) Visibility property is not inherited when used in an ani...
Malte Ubl
Reported 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.
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-
DetailsFormatted DiffDiff
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+
DetailsFormatted DiffDiff
patch (8.24 KB, patch)
2017-05-10 09:10 PDT, Antti Koivisto 		no flags
DetailsFormatted DiffDiff
patch (8.50 KB, patch)
2017-05-10 14:58 PDT, Antti Koivisto 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2017-05-09 14:26:17 PDT
<rdar://problem/32086550>
Simon Fraser (smfr)
Comment 2 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.
Malte Ubl
Comment 3 2017-05-09 14:46:38 PDT
Created attachment 309545 [details] 10.2 does not repro. Animated GIF showing the behavior.
Simon Fraser (smfr)
Comment 4 2017-05-09 14:49:00 PDT
I was testing on Mac. It would be surprising if Mac and iOS behavior differed.
Malte Ubl
Comment 5 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.
Simon Fraser (smfr)
Comment 6 2017-05-09 15:00:51 PDT
This regressed with https://trac.webkit.org/changeset/207372/webkit
Simon Fraser (smfr)
Comment 7 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
Antti Koivisto
Comment 8 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.
Simon Fraser (smfr)
Comment 9 2017-05-09 16:23:51 PDT
This is possibly about the propagation of visibleContent bits in RenderLayers
Antti Koivisto
Comment 10 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.
Antti Koivisto
Comment 11 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.
Antti Koivisto
Comment 12 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.
Antti Koivisto
Comment 13 2017-05-10 05:22:28 PDT
Created attachment 309591 [details] patch
Build Bot
Comment 14 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
Build Bot
Comment 15 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
Antti Koivisto
Comment 16 2017-05-10 07:31:13 PDT
Created attachment 309603 [details] patch
Simon Fraser (smfr)
Comment 17 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?
Antti Koivisto
Comment 18 2017-05-10 09:10:12 PDT
Created attachment 309612 [details] patch
WebKit Commit Bot
Comment 19 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>
WebKit Commit Bot
Comment 20 2017-05-10 09:51:01 PDT
All reviewed patches have been landed. Closing bug.
Ryan Haddad
Comment 21 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>
Antti Koivisto
Comment 22 2017-05-10 14:58:33 PDT
Created attachment 309643 [details] patch updated SameSizeAsRenderElement struct to accommodate an additional bit
WebKit Commit Bot
Comment 23 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>
WebKit Commit Bot
Comment 24 2017-05-10 15:23:16 PDT
All reviewed patches have been landed. Closing bug.
Malte Ubl
Comment 25 2017-05-11 02:47:53 PDT
Thanks for the super quick fix!
Note You need to log in before you can comment on or make changes to this bug.