Bug 108751 – WebKit shouldn't accept "none, none" in transition shorthand property.

• Component: CSS
• Status: RESOLVED FIXED

[Syoichi Tsuyuhara]

According to CSS Transitions...
>If there is more than one <single-transition> in the shorthand, and any of the transitions has ‘none’ as the <single-transition-property>, then the declaration is invalid.
http://dev.w3.org/csswg/css3-transitions/#transition-shorthand-property

On Windows 7 Home Premium SP1 64bit, Firefox 18.0.1 and Opera 12.13 correspond to this.
But Chromium 26.0.1401.0 (180245) accepts "none, none" in transition property.
http://jsfiddle.net/syoichi/qTcCR/

[Alexis Menard (darktears)]

Hi,

Thanks for the report. It has been fixed by http://trac.webkit.org/changeset/140560 and it is covered by a regression test : http://trac.webkit.org/changeset/140560/trunk/LayoutTests/transitions/transitions-parsing.html

Thanks for the report.

*** This bug has been marked as a duplicate of bug 105428 ***

[Syoichi Tsuyuhara]

Oh, sorry. I may not have made myself clear enough.
I report a bug of transition 'shorthand' property, not transition-property property.
Please confirm this code, and comment #0's spec.

>(function (body) {
>  body.style.transition = 'none, none';
>  body.insertAdjacentHTML('AfterBegin', body.style.transition === '');
>}(document.body));
http://jsfiddle.net/syoichi/qTcCR/

[Alexis Menard (darktears)]

I will investigate.

[Alexis Menard (darktears)]

Created attachment 188577 [details]
Patch

[WebKit Review Bot]

Comment on attachment 188577 [details]
Patch

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

New failing tests:
fast/css/transform-inline-style.html
fast/css/transform-inline-style-remove.html

[Alexis Menard (darktears)]

Created attachment 188585 [details]
Patch

[Dean Jackson]

Comment on attachment 188585 [details]
Patch

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

r+ but I think we need to clarify the names before landing.

> Source/WebCore/ChangeLog:23
> +        (WebCore::AnimationParseContext::hasFirstAnimationCommitted):

I suggest "hasCommittedFirstAnimation" (getter)

> Source/WebCore/ChangeLog:28
> +        (WebCore::AnimationParseContext::markParsingAnimationPropertyFinished):
> +        In the shorthand as soon as a keyword has been found then the parsing
> +        is 'finished', if any other animation/transition declaration part of
> +        the shorthand are with a keyword then it's invalid.
> +        (WebCore::AnimationParseContext::canParseMoreAnimationProperty):

This one is a tricky name, because it is so similar to the one below, but is only used within shorthands :(

How about "canParseAnotherAnimationPropertyKeywordInShorthand" (getter) and "animationPropertyKeywordParsedInShorthand" (setter).
Although I don't like that this has flipped the logic from your existing code, and also that the getter returns the opposite of the setter (if that makes sense).

> Source/WebCore/ChangeLog:30
> +        (WebCore::AnimationParseContext::hasAnimationPropertyKeyword):
> +        (WebCore::AnimationParseContext::commitAnimationPropertyKeyword):

How about "hasSeenAnimationPropertyKeyword" (getter) and "sawAnimationPropertyKeyword" (setter)?

[Dean Jackson]

Comment on attachment 188585 [details]
Patch

Even my suggestions are going against the style guide somewhat.

Also, why not just use a struct for the context, without any methods?

[Alexis Menard (darktears)]

Created attachment 188601 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 188601 [details]
Patch for landing

Rejecting attachment 188601 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Alexis Menard (darktears)]

Committed r143019: <http://trac.webkit.org/changeset/143019>

[Syoichi Tsuyuhara]

I confirmed that this bug is fixed on Chromium 27.0.1414.0 (182962).
Thanks!