Bug 171883

Summary:

REGRESSION (r207372) Visibility property is not inherited when used in an animation

Product:

WebKit

Reporter:

Malte Ubl <malte>

Component:

CSS

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Critical

CC:

buildbot, commit-queue, dino, koivisto, manian, rbyers, rniwa, ryanhaddad, simon.fraser, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

Safari 10

Hardware:

All

OS:

All

Attachments:

Description	Flags
10.2 does not repro. Animated GIF showing the behavior.	none
patch	buildbot: commit-queue-
Archive of layout-test-results from ews103 for mac-elcapitan	none
patch	simon.fraser: review+
patch	none
patch	none

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-	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
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.