Bug 108751 - WebKit shouldn't accept "none, none" in transition shorthand property.
Summary: WebKit shouldn't accept "none, none" in transition shorthand property.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexis Menard (darktears)
URL: http://jsfiddle.net/syoichi/qTcCR/
Keywords:
Depends on:
Blocks: 93136
  Show dependency treegraph
 
Reported: 2013-02-02 05:52 PST by Syoichi Tsuyuhara
Modified: 2013-02-16 04:35 PST (History)
6 users (show)

See Also:


Attachments
Patch (17.82 KB, patch)
2013-02-15 08:19 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (22.23 KB, patch)
2013-02-15 09:19 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch for landing (22.37 KB, patch)
2013-02-15 10:43 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Syoichi Tsuyuhara 2013-02-02 05:52:17 PST
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/
Comment 1 Alexis Menard (darktears) 2013-02-04 03:18:33 PST
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 ***
Comment 2 Syoichi Tsuyuhara 2013-02-04 05:02:11 PST
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/
Comment 3 Alexis Menard (darktears) 2013-02-04 05:05:57 PST
I will investigate.
Comment 4 Alexis Menard (darktears) 2013-02-15 08:19:46 PST
Created attachment 188577 [details]
Patch
Comment 5 WebKit Review Bot 2013-02-15 09:08:32 PST
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
Comment 6 Alexis Menard (darktears) 2013-02-15 09:19:33 PST
Created attachment 188585 [details]
Patch
Comment 7 Dean Jackson 2013-02-15 10:23:40 PST
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)?
Comment 8 Dean Jackson 2013-02-15 10:26:10 PST
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?
Comment 9 Alexis Menard (darktears) 2013-02-15 10:43:15 PST
Created attachment 188601 [details]
Patch for landing
Comment 10 WebKit Review Bot 2013-02-15 10:58:26 PST
Comment on attachment 188601 [details]
Patch for landing

Rejecting attachment 188601 [details] from commit-queue.

alexis@webkit.org 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.
Comment 11 Alexis Menard (darktears) 2013-02-15 11:08:15 PST
Committed r143019: <http://trac.webkit.org/changeset/143019>
Comment 12 Syoichi Tsuyuhara 2013-02-16 04:35:15 PST
I confirmed that this bug is fixed on Chromium 27.0.1414.0 (182962).
Thanks!