Skip to content

Commit

Permalink
Undo scroll snaps in to_parent when NeedsSourceToParentUpdate.
Browse files Browse the repository at this point in the history 
When we scroll on impl thread, we do scroll snapping in transform trees
if the transform node has a non-integer screen space transform. However,
the main thread logic also do snapping. When updating source to parent
transforms on main thread, it is possible that the scroll snaps are used
to compute the to_parent transform.

It becomes a problem if the node is created due to a fixed pos layer,
the snapping will add a small translation to the to_parent transform and
result in the unstable position. This CL undoes the snapping in this
case. It is notable that scroll_parent might also be influenced by this
change.

BUG=584598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2183923002
Cr-Commit-Position: refs/heads/master@{#408699}
  • Loading branch information
sunxd authored and Commit bot committed Jul 29, 2016
1 parent 3463467 commit 8a9a609
Show file tree
Hide file tree
Showing 4 changed files with 142 additions and 7 deletions.
113 changes: 113 additions & 0 deletions cc/trees/layer_tree_host_common_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6839,6 +6839,119 @@ TEST_F(LayerTreeHostCommonTest,
scroller->DrawTransform().To2dTranslation());
}

TEST_F(LayerTreeHostCommonTest, ScrollSnappingWithScrollChild) {
// This test verifies that a scrolling child of a scrolling layer doesn't get
// snapped to integer coordinates.
//
// + root
// + container
// + scroller
// + scroll_child
//
scoped_refptr<Layer> root = Layer::Create();
scoped_refptr<Layer> container = Layer::Create();
scoped_refptr<Layer> scroller = Layer::Create();
scoped_refptr<Layer> scroll_child = Layer::Create();
root->AddChild(container);
root->AddChild(scroll_child);
container->AddChild(scroller);
host()->SetRootLayer(root);

scroller->SetScrollClipLayerId(container->id());
scroll_child->SetScrollParent(scroller.get());

gfx::Transform rotate;
rotate.RotateAboutYAxis(30);
root->SetBounds(gfx::Size(50, 50));
container->SetBounds(gfx::Size(50, 50));
scroller->SetBounds(gfx::Size(100, 100));
scroller->SetPosition(gfx::PointF(10.3f, 10.3f));
scroll_child->SetBounds(gfx::Size(10, 10));
scroll_child->SetTransform(rotate);

ExecuteCalculateDrawProperties(root.get());

host()->host_impl()->CreatePendingTree();
host()->CommitAndCreatePendingTree();
host()->host_impl()->ActivateSyncTree();
LayerTreeImpl* layer_tree_impl = host()->host_impl()->active_tree();

LayerImpl* root_impl = layer_tree_impl->LayerById(root->id());
LayerImpl* scroller_impl = layer_tree_impl->LayerById(scroller->id());
LayerImpl* scroll_child_impl = layer_tree_impl->LayerById(scroll_child->id());
gfx::Vector2dF scroll_delta(5.f, 9.f);
SetScrollOffsetDelta(scroller_impl, scroll_delta);

ExecuteCalculateDrawProperties(root_impl);

gfx::Vector2dF expected_scroller_screen_space_transform_translation(5.f, 1.f);
EXPECT_VECTOR2DF_EQ(expected_scroller_screen_space_transform_translation,
scroller_impl->ScreenSpaceTransform().To2dTranslation());

gfx::Transform expected_scroll_child_screen_space_transform;
expected_scroll_child_screen_space_transform.Translate(-5.3f, -9.3f);
expected_scroll_child_screen_space_transform.RotateAboutYAxis(30);
EXPECT_TRANSFORMATION_MATRIX_EQ(expected_scroll_child_screen_space_transform,
scroll_child_impl->ScreenSpaceTransform());
}

TEST_F(LayerTreeHostCommonTest, ScrollSnappingWithFixedPosChild) {
// This test verifies that a fixed pos child of a scrolling layer doesn't get
// snapped to integer coordinates.
//
// + root
// + container
// + scroller
// + fixed_pos
//
scoped_refptr<Layer> root = Layer::Create();
scoped_refptr<Layer> container = Layer::Create();
scoped_refptr<Layer> scroller = Layer::Create();
scoped_refptr<Layer> fixed_pos = Layer::Create();

scroller->SetIsContainerForFixedPositionLayers(true);

root->AddChild(container);
container->AddChild(scroller);
scroller->AddChild(fixed_pos);
host()->SetRootLayer(root);

LayerPositionConstraint fixed_position;
fixed_position.set_is_fixed_position(true);
scroller->SetScrollClipLayerId(container->id());
fixed_pos->SetPositionConstraint(fixed_position);

root->SetBounds(gfx::Size(50, 50));
container->SetBounds(gfx::Size(50, 50));
scroller->SetBounds(gfx::Size(100, 100));
scroller->SetPosition(gfx::PointF(10.3f, 10.3f));
fixed_pos->SetBounds(gfx::Size(10, 10));

ExecuteCalculateDrawProperties(root.get());

host()->host_impl()->CreatePendingTree();
host()->CommitAndCreatePendingTree();
host()->host_impl()->ActivateSyncTree();
LayerTreeImpl* layer_tree_impl = host()->host_impl()->active_tree();

LayerImpl* root_impl = layer_tree_impl->LayerById(root->id());
LayerImpl* scroller_impl = layer_tree_impl->LayerById(scroller->id());
LayerImpl* fixed_pos_impl = layer_tree_impl->LayerById(fixed_pos->id());
gfx::Vector2dF scroll_delta(5.f, 9.f);
SetScrollOffsetDelta(scroller_impl, scroll_delta);

ExecuteCalculateDrawProperties(root_impl);

gfx::Vector2dF expected_scroller_screen_space_transform_translation(5.f, 1.f);
EXPECT_VECTOR2DF_EQ(expected_scroller_screen_space_transform_translation,
scroller_impl->ScreenSpaceTransform().To2dTranslation());

gfx::Vector2dF expected_fixed_pos_screen_space_transform_translation(10.3f,
10.3f);
EXPECT_VECTOR2DF_EQ(expected_fixed_pos_screen_space_transform_translation,
fixed_pos_impl->ScreenSpaceTransform().To2dTranslation());
}

class AnimationScaleFactorTrackingLayerImpl : public LayerImpl {
public:
static std::unique_ptr<AnimationScaleFactorTrackingLayerImpl> Create(
Expand Down
22 changes: 22 additions & 0 deletions cc/trees/property_tree.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,28 @@ void TransformTree::UpdateLocalTransform(TransformNode* node) {
if (NeedsSourceToParentUpdate(node)) {
gfx::Transform to_parent;
ComputeTranslation(node->source_node_id, node->parent_id, &to_parent);
gfx::Vector2dF unsnapping;
TransformNode* current;
TransformNode* parent_node;
for (current = Node(node->source_node_id); current->id > node->parent_id;
current = parent(current)) {
unsnapping.Subtract(current->scroll_snap);
}
for (parent_node = Node(node->parent_id);
parent_node->id > node->source_node_id;
parent_node = parent(parent_node)) {
unsnapping.Add(parent_node->scroll_snap);
}
// If a node NeedsSourceToParentUpdate, the node is either a fixed position
// node or a scroll child.
// If the node has a fixed position, the parent of the node is an ancestor
// of source node, current->id should be equal to node->parent_id.
// Otherwise, the node's source node is always an ancestor of the node owned
// by the scroll parent, so parent_node->id should be equal to
// node->source_node_id.
DCHECK(current->id == node->parent_id ||
parent_node->id == node->source_node_id);
to_parent.Translate(unsnapping.x(), unsnapping.y());
node->source_to_parent = to_parent.To2dTranslation();
}

Expand Down
7 changes: 0 additions & 7 deletions cc/trees/property_tree_builder.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ struct DataForRecursion {
uint32_t main_thread_scrolling_reasons;
bool scroll_tree_parent_created_by_uninheritable_criteria;
const gfx::Transform* device_transform;
gfx::Vector2dF scroll_snap;
gfx::Transform compound_transform_since_render_target;
bool axis_align_since_render_target;
SkColor safe_opaque_background_color;
Expand Down Expand Up @@ -540,7 +539,6 @@ bool AddTransformNodeIfNeeded(
->offset_to_transform_parent();
source_index =
data_from_ancestor.transform_tree_parent->transform_tree_index();
source_offset -= data_from_ancestor.scroll_snap;
}
}

Expand All @@ -559,9 +557,6 @@ bool AddTransformNodeIfNeeded(
}
data_for_children->transform_tree_parent = layer;

if (IsContainerForFixedPositionLayers(layer) || is_fixed)
data_for_children->scroll_snap = gfx::Vector2dF();

if (!requires_node) {
data_for_children->should_flatten |= ShouldFlattenTransform(layer);
gfx::Vector2dF local_offset = layer->position().OffsetFromOrigin() +
Expand Down Expand Up @@ -694,8 +689,6 @@ bool AddTransformNodeIfNeeded(
// Flattening (if needed) will be handled by |node|.
layer->set_should_flatten_transform_from_property_tree(false);

data_for_children->scroll_snap += node->scroll_snap;

node->owner_id = layer->id();

return true;
Expand Down
7 changes: 7 additions & 0 deletions cc/trees/property_tree_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1071,11 +1071,13 @@ class PropertyTreeTestNonIntegerTranslationTest : public PropertyTreeTest {

int parent = tree.Insert(TransformNode(), 0);
tree.SetTargetId(parent, parent);
tree.Node(parent)->source_node_id = 0;
tree.Node(parent)->local.Translate(1.5f, 1.5f);

int child = tree.Insert(TransformNode(), parent);
tree.SetTargetId(child, parent);
tree.Node(child)->local.Translate(1, 1);
tree.Node(child)->source_node_id = parent;
tree.set_needs_update(true);
SetupTransformTreeForTest(&tree);
draw_property_utils::ComputeTransforms(&tree);
Expand All @@ -1086,6 +1088,8 @@ class PropertyTreeTestNonIntegerTranslationTest : public PropertyTreeTest {

tree.Node(parent)->local.Translate(0.5f, 0.5f);
tree.Node(child)->local.Translate(0.5f, 0.5f);
tree.Node(parent)->needs_local_transform_update = true;
tree.Node(child)->needs_local_transform_update = true;
tree.set_needs_update(true);
SetupTransformTreeForTest(&tree);
draw_property_utils::ComputeTransforms(&tree);
Expand All @@ -1095,6 +1099,7 @@ class PropertyTreeTestNonIntegerTranslationTest : public PropertyTreeTest {
tree.Node(child)->node_and_ancestors_have_only_integer_translation);

tree.Node(child)->local.Translate(0.5f, 0.5f);
tree.Node(child)->needs_local_transform_update = true;
tree.SetTargetId(child, child);
tree.set_needs_update(true);
SetupTransformTreeForTest(&tree);
Expand Down Expand Up @@ -1124,13 +1129,15 @@ class PropertyTreeTestSingularTransformSnapTest : public PropertyTreeTest {
effect_tree.Node(effect_parent)->has_render_surface = true;
tree.SetTargetId(parent, parent);
tree.Node(parent)->scrolls = true;
tree.Node(parent)->source_node_id = 0;

int child = tree.Insert(TransformNode(), parent);
TransformNode* child_node = tree.Node(child);
tree.SetTargetId(child, parent);
child_node->scrolls = true;
child_node->local.Scale3d(6.0f, 6.0f, 0.0f);
child_node->local.Translate(1.3f, 1.3f);
child_node->source_node_id = parent;
tree.set_needs_update(true);

SetupTransformTreeForTest(&tree);
Expand Down

0 comments on commit 8a9a609

Please sign in to comment.