Keyframe rule should be ignored if keyframe selector has invalid value

[Alexander Shalamov]

Whenever keyframe selector has invalid value (negative or more than 100%), keyframe rule should be ignored.

[Alexander Shalamov]

Created attachment 187297 [details]
Patch 1

Patch for initial review.

[Alexander Shalamov]

Created attachment 187299 [details]
Patch 2

- remove unnecessary rule.release() in CSSParser::createKeyframesRule

[Early Warning System Bot]

Comment on attachment 187299 [details]
Patch 2

Attachment 187299 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16427613

[Early Warning System Bot]

Comment on attachment 187299 [details]
Patch 2

Attachment 187299 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16427615

[WebKit Review Bot]

Comment on attachment 187299 [details]
Patch 2

Attachment 187299 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16408747

[EFL EWS Bot]

Comment on attachment 187299 [details]
Patch 2

Attachment 187299 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16434636

[Peter Beverloo (cr-android ews)]

Comment on attachment 187299 [details]
Patch 2

Attachment 187299 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16418649

[WebKit Review Bot]

Comment on attachment 187299 [details]
Patch 2

Attachment 187299 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16432666

[Build Bot]

Comment on attachment 187299 [details]
Patch 2

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

[Build Bot]

Comment on attachment 187299 [details]
Patch 2

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

[Alexander Shalamov]

Created attachment 187550 [details]
Patch 3

fix for linking issue

[Alexander Shalamov]

Created attachment 197744 [details]
Patch 4

rebased to master

[Simon Fraser (smfr)]

Comment on attachment 197744 [details]
Patch 4

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

> LayoutTests/animations/keyframe-rule-negative-selector-percentage.html:6
> +            -100% {

What about comma-separate keyframe selectors, like 10%, -20%? Please test those.

[Alexander Shalamov]

Created attachment 198324 [details]
Patch 5

Added comma separated keyframe selectors to layout test.

[Dean Jackson]

Comment on attachment 198324 [details]
Patch 5

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

Nearly there!

You might want to test -0% as well.

> Source/WebCore/ChangeLog:7
> +        Keyframe rule should be ignored if keyframe selector has invalid value
> +        https://bugs.webkit.org/show_bug.cgi?id=109271
> +
> +        Reviewed by NOBODY (OOPS!).
> +

Please outline what you changed. You updated the CSS grammar - it's important to describe why you did that.

> Source/WebCore/ChangeLog:26
> +        * css/CSSGrammar.y.in:
> +        * css/CSSParser.cpp:
> +        (WebCore::CSSParser::createKeyframesRule):
> +        (WebCore::CSSParser::updateSpecifiers):
> +        * css/StyleResolver.cpp:
> +        (WebCore::StyleResolver::keyframeStylesForAnimation):
> +        * css/WebKitCSSKeyframeRule.cpp:
> +        (WebCore::StyleKeyframe::keyText):
> +        (WebCore):
> +        (WebCore::StyleKeyframe::setKeyText):
> +        (WebCore::StyleKeyframe::setKeys):
> +        (WebCore::StyleKeyframe::parseKeyString):
> +        (WebCore::StyleKeyframe::reportMemoryUsage):
> +        * css/WebKitCSSKeyframeRule.h:
> +        (StyleKeyframe):
> +        (WebCore::StyleKeyframe::isValid):
> +        (WebCore::StyleKeyframe::getKeys):

And provide some details here too.

> Source/WebCore/css/CSSParser.cpp:11351
> +        if (keyframe->isValid()) {
> +            rule->parserAppendKeyframe(keyframe);
> +            continue;
> +        }
> +        return 0;

You should do this the other way around.

if (!keyframe->isValid())
  return 0;

rule->parserAppendKeyframe(keyframe);

> Source/WebCore/css/CSSParser.cpp:11630
> +        if (value && value->unit == CSSPrimitiveValue::CSS_NUMBER) {
> +            keyframeKeys.append(static_cast<float>(value->fValue / 100));
> +            continue;
> +        }
> +        keyframeKeys.clear();
> +        break;

Same here. Early break/return in the invalid case.

[Brent Fulgham]

I believe keyframe animation works properly now. Could you REOPEN if you believe a specific case is still not handled properly?