Bug 159450 – REGRESSION(r200769): animations are no longer overridden

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Dean Jackson]

https://trac.webkit.org/changeset/200769 has introduced a regression on AMP content.

Making a test case now.

[Radar WebKit Bug Importer]

<rdar://problem/27188180>

[Ryosuke Niwa]

The problem appears to the code change in MutableStyleProperties::setProperty.  We’re no longer overriding prefixing variant, and this manifests in 8s animation of the content being visiblity: hidden not be overriden.

Effectively, AMP has the following rules:
body {
    -webkit-animation: -amp-start 8s steps(1,end) 0s 1 normal both;
    animation: -amp-start 8s steps(1,end) 0s 1 normal both;
}

body {
    animation: none;
}

And animation: none is no longer taking effect so we’re delayin the display of contenets for 8s now.

[Ryosuke Niwa]

<rdar://problem/27120570>

[Ryosuke Niwa]

Created attachment 282841 [details]
Demo

[Dean Jackson]

<rdar://problem/27120570>

[Dean Jackson]

Created attachment 282843 [details]
test

[Ryosuke Niwa]

e.g. http://www.macrumors.com/2016/06/30/apple-tidal-acquisition-talks/amp/ and https://ampbyexample.com takes 8s to load :(

[Ryosuke Niwa]

Created attachment 282845 [details]
Reduction for AMP

[Ryosuke Niwa]

Hm... it looks like we might be confusing the style resolver here. Because "animation: none" doesn't set -webkit-animation-*, the earlier values of -webkit-animation-* are persisting.  We might need to teach the style resolver to ignore prefixed properties when unprefixed properties are specified.

[Dean Jackson]

Created attachment 282982 [details]
Patch

[WebKit Commit Bot]

Attachment 282982 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:9:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 282982 [details]
Patch

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

New failing tests:
transitions/transitions-parsing.html
animations/animation-shorthand-removed.html
fast/css/shorthand-omitted-initial-value-overrides-shorthand.html

[Build Bot]

Created attachment 282984 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282982 [details]
Patch

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

New failing tests:
animations/animation-shorthand-removed.html
fast/css/shorthand-omitted-initial-value-overrides-shorthand.html
transitions/transitions-parsing.html

[Build Bot]

Created attachment 282985 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282982 [details]
Patch

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

New failing tests:
transitions/transitions-parsing.html
animations/animation-shorthand-removed.html
fast/css/shorthand-omitted-initial-value-overrides-shorthand.html

[Build Bot]

Created attachment 282986 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282982 [details]
Patch

Attachment 282982 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1638884

New failing tests:
transitions/transitions-parsing.html
animations/animation-shorthand-removed.html
fast/css/shorthand-omitted-initial-value-overrides-shorthand.html

[Build Bot]

Created attachment 282987 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Dean Jackson]

Created attachment 283093 [details]
Patch

[Dean Jackson]

Committed r202950: <http://trac.webkit.org/changeset/202950>

[Antti Koivisto]

Comment on attachment 283093 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:3843
> +        // We can't use ShorthandScope here as we can already be inside one (e.g we are parsing CSSTransition).
> +        m_currentShorthand = prefixingVariantForPropertyId(m_currentShorthand);
> +        addProperty(prefixingVariant, WTFMove(value), important, implicit);
> +        m_currentShorthand = prefixingVariantForPropertyId(m_currentShorthand);

I don't understand this code. Why are we repeating the same operation before and after addProperty?

Also isn't the whole point of FooScopes that they can nest?

(In reply to comment #21)
> Committed r202950: <http://trac.webkit.org/changeset/202950>

This caused bug 159861.