Skip to content

Commit

Permalink
Correct handling of [0px,1px) values of perspective and perspective().
Browse files Browse the repository at this point in the history
This makes a number of fixes to handling small values of the perspective
CSS property and the perspective() transform function to match the
css-transforms-2 specification (the latest updates to which come from
the resolutions in w3c/csswg-drafts#413):

 * Accept zero values of the perspective property at parse time.  (They
   were already accepted for the perspective function.)

   Zero values are currently accepted by Gecko, but it treats them as
   the identity matrix (that is, as infinite perspective) rather than
   clamping to 1px.

 * Use -1.0 rather than 0.0 as the internal representation of
   perspective: none.

 * For rendering of both the perspective property and the perspective()
   transform function, treat values smaller than 1px as 1px.

 * For interpolation of the perspective() transform function,
   treat values smaller than 1px as 1px.  This is an additional
   clarification to the resolution that I proposed in
   w3c/csswg-drafts#6320.

 * When handling the perspective() transform function when finding the
   resolved value of the transform property (which is a matrix() or
   matrix3d() value), treat values smaller than 1px as 1px.  (Resolved
   values are the results of getComputedStyle().)  This is an additional
   clarification that I proposed in
   w3c/csswg-drafts#6346.

Note that interpolation and resolved values of the perspective property
since both interpolation and resolved values match the specified values.
In the case of interpolation that was resolved specifically in
w3c/csswg-drafts#3084.

It also substantially simplifies PerspectiveTransformOperation::Blend,
although I *believe* the only substantive change is the clamping of its
inputs to be 1px or larger.

Parts of this are somewhat risky, since previously transform:
perspective(0) was treated as the identity matrix and perspective: 0 was
a syntax error, whereas this makes both be treated as very substantial
transform (perspective from 1px away).  The old behavior of transform:
perspective(0) was interoperable across browsers.  The old behavior of
perspective: 0 was different in Gecko (where it was valid syntax, but
like transform: perspective(0) was treated as the identity matrix), but
the old behaviors across browsers still had in common that they all led
to the identity matrix (whether valid or invalid syntax), which is not
true of the new behavior.  The risk for handling of values in (0px, 1px)
is probably less substantial since those were already treated as extreme
transforms, and this makes them less extreme.

There are thus three possible less-risky alternatives, from more risk
(but less than this) to lowest risk:

 * Use this patch, but omit the changes to perspective: 0 and
   perspective(0) except for the change that makes perspective: 0 valid,
   but treat perspective: 0 as an identity transform like Gecko does.

 * Use this patch, but omit all the changes to perspective: 0px and
   perspective(0).

 * Change the behavior only when DBL_TRUE_MIN <= perspective < DBL_MIN,
   by treating perspective (property or function) as DBL_MIN in those
   cases.

However, it's worth trying this riskier alternative and following the
CSS Working Group's decision because that decision was made for good
reasons.  Taking this approach has two advantages:

 (1) It eliminates the only case where the valid values of a CSS
     property are an open range (a range exclusive of its endpoint),
     which creates difficulties for defining clamping of values to the
     valid range, which is important to CSS both for calc() and for
     animations (e.g., when the timing function result is outside of
     [0, 1]).

 (2) It eliminates a discontinuity in behavior at zero.  Discontinuities
     in behavior cause animations that cross the discontinuity to behave
     poorly.

Fixed: 1205161
Change-Id: Ie11a3d27d32e6ce16c39d670f6423a6710ba0971
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2924023
Commit-Queue: David Baron <dbaron@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#889344}
NOKEYCHECK=True
GitOrigin-RevId: 50b1cc46560ac75003b6c64db76fc14d23508735
  • Loading branch information
dbaron authored and Copybara-Service committed Jun 4, 2021
1 parent 8473725 commit 392fa86
Show file tree
Hide file tree
Showing 24 changed files with 131 additions and 279 deletions.
2 changes: 1 addition & 1 deletion blink/renderer/core/css/css_properties.json5
Original file line number Diff line number Diff line change
Expand Up @@ -3323,7 +3323,7 @@
interpolable: true,
field_group: "*",
field_template: "primitive",
default_value: "0.0",
default_value: "-1.0",
type_name: "float",
converter: "ConvertPerspective",
keywords: ["none"],
Expand Down
10 changes: 4 additions & 6 deletions blink/renderer/core/css/properties/longhands/longhands_custom.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5374,20 +5374,18 @@ const CSSValue* Perspective::ParseSingleValue(
if (range.Peek().Id() == CSSValueID::kNone)
return css_parsing_utils::ConsumeIdent(range);
CSSPrimitiveValue* parsed_value =
css_parsing_utils::ConsumeLength(range, context, kValueRangeAll);
css_parsing_utils::ConsumeLength(range, context, kValueRangeNonNegative);
bool use_legacy_parsing = localContext.UseAliasParsing();
if (!parsed_value && use_legacy_parsing) {
double perspective;
if (!css_parsing_utils::ConsumeNumberRaw(range, context, perspective))
if (!css_parsing_utils::ConsumeNumberRaw(range, context, perspective) ||
perspective < 0.0)
return nullptr;
context.Count(WebFeature::kUnitlessPerspectiveInPerspectiveProperty);
parsed_value = CSSNumericLiteralValue::Create(
perspective, CSSPrimitiveValue::UnitType::kPixels);
}
if (parsed_value &&
(parsed_value->IsCalculated() || parsed_value->GetDoubleValue() > 0))
return parsed_value;
return nullptr;
return parsed_value;
}

const CSSValue* Perspective::CSSValueFromComputedStyleInternal(
Expand Down
2 changes: 1 addition & 1 deletion blink/renderer/core/layout/layout_box.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1995,7 +1995,7 @@ bool LayoutBox::MapVisualRectToContainer(

TransformationMatrix perspective_matrix;
perspective_matrix.ApplyPerspective(
container_object->StyleRef().Perspective());
container_object->StyleRef().UsedPerspective());
perspective_matrix.ApplyTransformOrigin(perspective_origin.X(),
perspective_origin.Y(), 0);

Expand Down
2 changes: 1 addition & 1 deletion blink/renderer/core/layout/layout_object.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3339,7 +3339,7 @@ void LayoutObject::GetTransformFromContainer(

TransformationMatrix perspective_matrix;
perspective_matrix.ApplyPerspective(
container_object->StyleRef().Perspective());
container_object->StyleRef().UsedPerspective());
perspective_matrix.ApplyTransformOrigin(perspective_origin.X(),
perspective_origin.Y(), 0);

Expand Down
2 changes: 1 addition & 1 deletion blink/renderer/core/paint/paint_property_tree_builder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1917,7 +1917,7 @@ void FragmentPaintPropertyTreeBuilder::UpdatePerspective() {
// most transform nodes do.
TransformPaintPropertyNode::State state{
TransformPaintPropertyNode::TransformAndOrigin(
TransformationMatrix().ApplyPerspective(style.Perspective()),
TransformationMatrix().ApplyPerspective(style.UsedPerspective()),
PerspectiveOrigin(To<LayoutBox>(object_)) +
FloatSize(context_.current.paint_offset))};
state.flags.flattens_inherited_transform =
Expand Down
8 changes: 7 additions & 1 deletion blink/renderer/core/style/computed_style.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#ifndef THIRD_PARTY_BLINK_RENDERER_CORE_STYLE_COMPUTED_STYLE_H_
#define THIRD_PARTY_BLINK_RENDERER_CORE_STYLE_COMPUTED_STYLE_H_

#include <algorithm>
#include <memory>
#include "base/types/pass_key.h"
#include "third_party/blink/renderer/core/core_export.h"
Expand Down Expand Up @@ -1931,7 +1932,12 @@ class ComputedStyle : public ComputedStyleBase,
}

// Perspective utility functions.
bool HasPerspective() const { return Perspective() > 0; }
bool HasPerspective() const { return Perspective() >= 0; }

float UsedPerspective() const {
DCHECK(HasPerspective());
return std::max(1.0f, Perspective());
}

// Outline utility functions.
// HasOutline is insufficient to determine whether Node has an outline.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

#include "third_party/blink/renderer/platform/transforms/perspective_transform_operation.h"

#include <algorithm>
#include "third_party/blink/renderer/platform/geometry/blend.h"
#include "third_party/blink/renderer/platform/wtf/math_extras.h"

Expand All @@ -33,18 +34,15 @@ namespace blink {
scoped_refptr<TransformOperation> PerspectiveTransformOperation::Accumulate(
const TransformOperation& other) {
DCHECK(other.IsSameType(*this));
double other_p = To<PerspectiveTransformOperation>(other).p_;

if (p_ == 0 && other_p == 0)
return nullptr;
double other_p = To<PerspectiveTransformOperation>(other).UsedPerspective();
double p = UsedPerspective();

// We want to solve:
// -1/p + -1/p' == -1/p'', where we know p and p'.
//
// This can be rewritten as:
// p'' == (p * p') / (p + p')
double p = (p_ * other_p) / (p_ + other_p);
return PerspectiveTransformOperation::Create(p);
return PerspectiveTransformOperation::Create((p * other_p) / (p + other_p));
}

scoped_refptr<TransformOperation> PerspectiveTransformOperation::Blend(
Expand All @@ -54,34 +52,27 @@ scoped_refptr<TransformOperation> PerspectiveTransformOperation::Blend(
if (from && !from->IsSameType(*this))
return this;

// https://drafts.csswg.org/css-transforms-2/#interpolation-of-transform-functions
// says that we should run matrix decomposition and then run the rules for
// interpolation of matrices, but we know what those rules are going to
// yield, so just do that directly.
double from_p_inverse, to_p_inverse;
if (blend_to_identity) {
// FIXME: this seems wrong. https://bugs.webkit.org/show_bug.cgi?id=52700
double p = blink::Blend(p_, 1., progress);
return PerspectiveTransformOperation::Create(clampTo<int>(p, 0));
}

const PerspectiveTransformOperation* from_op =
static_cast<const PerspectiveTransformOperation*>(from);

TransformationMatrix from_t;
TransformationMatrix to_t;
from_t.ApplyPerspective(from_op ? from_op->p_ : 0);
to_t.ApplyPerspective(p_);
to_t.Blend(from_t, progress);

TransformationMatrix::DecomposedType decomp;
if (!to_t.Decompose(decomp)) {
// If we can't decompose, bail out of interpolation.
const PerspectiveTransformOperation* used_operation =
progress > 0.5 ? this : from_op;
return PerspectiveTransformOperation::Create(used_operation->Perspective());
}

if (decomp.perspective_z) {
double val = -1.0 / decomp.perspective_z;
return PerspectiveTransformOperation::Create(clampTo<double>(val, 0));
from_p_inverse = 1.0 / UsedPerspective();
to_p_inverse = 0.0;
} else {
if (from) {
const PerspectiveTransformOperation* from_op =
static_cast<const PerspectiveTransformOperation*>(from);
from_p_inverse = 1.0 / from_op->UsedPerspective();
} else {
from_p_inverse = 0.0;
}
to_p_inverse = 1.0 / UsedPerspective();
}
return PerspectiveTransformOperation::Create(0);
double p =
1.0 / std::max(0.0, blink::Blend(from_p_inverse, to_p_inverse, progress));
return PerspectiveTransformOperation::Create(clampTo<double>(p, 0));
}

scoped_refptr<TransformOperation> PerspectiveTransformOperation::Zoom(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#ifndef THIRD_PARTY_BLINK_RENDERER_PLATFORM_TRANSFORMS_PERSPECTIVE_TRANSFORM_OPERATION_H_
#define THIRD_PARTY_BLINK_RENDERER_PLATFORM_TRANSFORMS_PERSPECTIVE_TRANSFORM_OPERATION_H_

#include <algorithm>
#include "third_party/blink/renderer/platform/transforms/transform_operation.h"
#include "third_party/blink/renderer/platform/wtf/casting.h"

Expand All @@ -40,6 +41,8 @@ class PLATFORM_EXPORT PerspectiveTransformOperation final

double Perspective() const { return p_; }

double UsedPerspective() const { return std::max(1.0, p_); }

static bool IsMatchingOperationType(OperationType type) {
return type == kPerspective;
}
Expand All @@ -56,7 +59,7 @@ class PLATFORM_EXPORT PerspectiveTransformOperation final
}

void Apply(TransformationMatrix& transform, const FloatSize&) const override {
transform.ApplyPerspective(p_);
transform.ApplyPerspective(UsedPerspective());
}

scoped_refptr<TransformOperation> Accumulate(
Expand Down
2 changes: 0 additions & 2 deletions blink/web_tests/TestExpectations
Original file line number Diff line number Diff line change
Expand Up @@ -5446,11 +5446,9 @@ crbug.com/1008483 virtual/transform-interop/transforms/3d/point-mapping/3d-point

crbug.com/943503 external/wpt/css/css-transforms/transform3d-perspective-003.html [ Failure ]
crbug.com/943503 external/wpt/css/css-transforms/transform3d-perspective-004.html [ Failure ]
crbug.com/943503 external/wpt/css/css-transforms/transform3d-perspective-005.html [ Failure ]

crbug.com/943503 virtual/transform-interop/external/wpt/css/css-transforms/transform3d-perspective-003.html [ Pass ]
crbug.com/943503 virtual/transform-interop/external/wpt/css/css-transforms/transform3d-perspective-004.html [ Pass ]
crbug.com/943503 virtual/transform-interop/external/wpt/css/css-transforms/transform3d-perspective-005.html [ Pass ]

# Swiftshader issue.
crbug.com/1048149 external/wpt/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-innerwidth.html [ Crash Timeout ]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
from: '50px',
to: '100px'
}, [
{at: -20, is: 'none'}, // perspective does not accept 0 or negative values
{at: -20, is: '0px'},
{at: -0.3, is: '35px'},
{at: 0, is: '50px'},
{at: 0.3, is: '65px'},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@

player.currentTime = 5;
element.style.fontSize = '40px';
assert_equals(getComputedStyle(element).perspective, 'none');
assert_equals(getComputedStyle(element).perspective, '0px');

player.currentTime = 7.5;
assert_equals(getComputedStyle(element).perspective, 'none');
}, 'perspective clamped to none');
assert_equals(getComputedStyle(element).perspective, '0px');
}, 'perspective clamped to 0px');

test(function() {
container.style.perspective = 'none';
Expand Down
Loading

0 comments on commit 392fa86

Please sign in to comment.