Bug 52683 – perspective() transform function should take lengths

• Component: CSS
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

The perspective() transform function currently takes a unitless number. It should instead take a Length.

[Simon Fraser (smfr)]

Created attachment 79391 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/anim..." exit_code: 1

Source/WebCore/platform/graphics/transforms/PerspectiveTransformOperation.h:44:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/css/CSSParser.cpp:5319:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 10 files


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

[WebKit Review Bot]

Attachment 79391 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7594202

[Simon Fraser (smfr)]

Created attachment 79394 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/anim..." exit_code: 1

Source/WebCore/platform/graphics/transforms/PerspectiveTransformOperation.h:44:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/css/CSSParser.cpp:5319:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 10 files


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

[WebKit Review Bot]

Attachment 79391 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7633060

[Dean Jackson]

Comment on attachment 79394 [details]
Patch

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

> LayoutTests/transforms/3d/general/3dtransform-values.html:47
> +      { 'transform' : 'perspective(banana)', 'result' : 'none' }, // unit must be length

Test for bare perspective() ?

{ 'transform' : 'perspective()', 'result' : 'none' }

Although maybe it's already there, just not in this diff.

> Source/WebCore/ChangeLog:9
> +        be a Length, rather than a bare number.

Add a comment to say that bare numbers are still accepted (and are evaluated as px, as $DEITY intended)?

> Source/WebCore/css/CSSStyleSelector.cpp:7057
> +                    // This is a quirk that should go away when 3d transforms are finalized.

I think you should say it is deprecated, but in honesty I'm not sure we can ever make it go away. I don't see what the deal is with interpreting a bare number as px. In this case there is no possible confusion - the problem is with properties like line-height in which a bare number is a multiplier, not a length.

> Source/WebCore/platform/graphics/transforms/PerspectiveTransformOperation.cpp:59
> +    toT.applyPerspective(toP.calcFloatValue(1));

This is a question to educate me rather than a comment, but why do you pass 1 in as the maxvalue for calcFloatValue? If it is a percentage, then why clamp it? (and what does a percentage used in perspective mean?).

[Simon Fraser (smfr)]

(In reply to comment #7)
> (From update of attachment 79394 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79394&action=review
> 
> > LayoutTests/transforms/3d/general/3dtransform-values.html:47
> > +      { 'transform' : 'perspective(banana)', 'result' : 'none' }, // unit must be length
> 
> Test for bare perspective() ?

It's handled, but I could add a test, yeah.

> > Source/WebCore/ChangeLog:9
> > +        be a Length, rather than a bare number.
> 
> Add a comment to say that bare numbers are still accepted (and are evaluated as px, as $DEITY intended)?

Sure.

> > Source/WebCore/css/CSSStyleSelector.cpp:7057
> > +                    // This is a quirk that should go away when 3d transforms are finalized.
> 
> I think you should say it is deprecated, but in honesty I'm not sure we can ever make it go away. I don't see what the deal is with interpreting a bare number as px. In this case there is no possible confusion - the problem is with properties like line-height in which a bare number is a multiplier, not a length.

It should match translate() etc, for which I think we require units.

> > Source/WebCore/platform/graphics/transforms/PerspectiveTransformOperation.cpp:59
> > +    toT.applyPerspective(toP.calcFloatValue(1));
> 
> This is a question to educate me rather than a comment, but why do you pass 1 in as the maxvalue for calcFloatValue? If it is a percentage, then why clamp it? (and what does a percentage used in perspective mean?).

I believe the argument is unused for non-percentage units.

[Dean Jackson]

(In reply to comment #8)

> > > Source/WebCore/css/CSSStyleSelector.cpp:7057
> > > +                    // This is a quirk that should go away when 3d transforms are finalized.
> > 
> > I think you should say it is deprecated, but in honesty I'm not sure we can ever make it go away. I don't see what the deal is with interpreting a bare number as px. In this case there is no possible confusion - the problem is with properties like line-height in which a bare number is a multiplier, not a length.
> 
> It should match translate() etc, for which I think we require units.

Obviously I wouldn't mind if translate didn't require units :)

> 
> > > Source/WebCore/platform/graphics/transforms/PerspectiveTransformOperation.cpp:59
> > > +    toT.applyPerspective(toP.calcFloatValue(1));
> > 
> > This is a question to educate me rather than a comment, but why do you pass 1 in as the maxvalue for calcFloatValue? If it is a percentage, then why clamp it? (and what does a percentage used in perspective mean?).
> 
> I believe the argument is unused for non-percentage units.

That's true, but why clamp it if I put perspective(120%) ? Maybe I misunderstand.

[Simon Fraser (smfr)]

(In reply to comment #9)
> (In reply to comment #8)
> 
> > > > Source/WebCore/css/CSSStyleSelector.cpp:7057
> > > > +                    // This is a quirk that should go away when 3d transforms are finalized.
> > > 
> > > I think you should say it is deprecated, but in honesty I'm not sure we can ever make it go away. I don't see what the deal is with interpreting a bare number as px. In this case there is no possible confusion - the problem is with properties like line-height in which a bare number is a multiplier, not a length.
> > 
> > It should match translate() etc, for which I think we require units.
> 
> Obviously I wouldn't mind if translate didn't require units :)

We have to use units in CSS. This ain't SVG!

> > > > Source/WebCore/platform/graphics/transforms/PerspectiveTransformOperation.cpp:59
> > > > +    toT.applyPerspective(toP.calcFloatValue(1));
> > > 
> > > This is a question to educate me rather than a comment, but why do you pass 1 in as the maxvalue for calcFloatValue? If it is a percentage, then why clamp it? (and what does a percentage used in perspective mean?).
> > 
> > I believe the argument is unused for non-percentage units.
> 
> That's true, but why clamp it if I put perspective(120%) ? Maybe I misunderstand.

Percentages are not allowed in perspective(), only lengths.

[Chris Marrin]

Comment on attachment 79394 [details]
Patch

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

r=me after possibly clarifying a couple of lines and fixing the style issues.

>>> Source/WebCore/css/CSSStyleSelector.cpp:7057
>>> +                    // This is a quirk that should go away when 3d transforms are finalized.
>> 
>> I think you should say it is deprecated, but in honesty I'm not sure we can ever make it go away. I don't see what the deal is with interpreting a bare number as px. In this case there is no possible confusion - the problem is with properties like line-height in which a bare number is a multiplier, not a length.
> 
> It should match translate() etc, for which I think we require units.

Also, you should say what the quirk is (which I think is that we still accept bare naked numbers in perspective())

> Source/WebCore/css/CSSStyleSelector.cpp:7060
> +                    val = min<double>(val, numeric_limits<int>::max());

Wow, this looks icky. You really have to use templates here? It sure hides what you're actually doing in all that noise.

> Source/WebCore/platform/graphics/transforms/PerspectiveTransformOperation.cpp:38
> +    return static_cast<int>(max<double>(min<double>(val, numeric_limits<int>::max()), 0));

Again, holy obfuscation Batman, what is this actually doing? Maybe at least split this into 3 lines: max, min and cast?

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/76568