Skip to content

Commit

Permalink
Revert "VT: Remove containment requirement."
Browse files Browse the repository at this point in the history
This reverts commit e554cf3.

Reason for revert: Decided against this feature for now.

Original change's description:
> VT: Remove containment requirement.
>
> This patch removes the containment requirement from view-transitions.
>
> This is to align with proposed resolution
>  w3c/csswg-drafts#7882
>
> R=​khushalsagar@chromium.org, bokan@chromium.org
>
> Fixed: 1409491
> Change-Id: Iad0eb54c8d2de503f209a58a9f438e586fcd6a36
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4188811
> Reviewed-by: David Bokan <bokan@chromium.org>
> Reviewed-by: Khushal Sagar <khushalsagar@chromium.org>
> Commit-Queue: Vladimir Levin <vmpstr@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1096187}

Change-Id: Id0b58230eb372a96aa1f1dff2e7d84e2f297219f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4192788
Commit-Queue: Vladimir Levin <vmpstr@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1096273}
  • Loading branch information
vmpstr authored and Chromium LUCI CQ committed Jan 24, 2023
1 parent a0c3f50 commit f2820f7
Show file tree
Hide file tree
Showing 36 changed files with 252 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@
namespace blink {
namespace {

const char* kContainmentNotSatisfied =
"Aborting transition. Element must contain paint or layout for "
"view-transition-name : ";
const char* kDuplicateTagBaseError =
"Unexpected duplicate view-transition-name: ";

Expand All @@ -63,6 +66,11 @@ const String& AnimationUAStyles() {
return kAnimationUAStyles;
}

bool SatisfiesContainment(const LayoutObject& object) {
return object.ShouldApplyPaintContainment() ||
object.ShouldApplyLayoutContainment();
}

absl::optional<String> ComputeInsetDifference(PhysicalRect reference_rect,
const LayoutRect& target_rect,
float device_pixel_ratio) {
Expand Down Expand Up @@ -339,6 +347,25 @@ bool ViewTransitionStyleTracker::FlattenAndVerifyElements(
VectorOf<Element>& elements,
VectorOf<AtomicString>& transition_names,
absl::optional<RootData>& root_data) {
for (const auto& element : ViewTransitionSupplement::From(*document_)
->ElementsWithViewTransitionName()) {
DCHECK(element->ComputedStyleRef().ViewTransitionName());

// Ignore elements which are not rendered.
if (!element->GetLayoutObject())
continue;

// Skip the transition if containment is not satisfied.
if (!element->IsDocumentElement() &&
!SatisfiesContainment(*element->GetLayoutObject())) {
StringBuilder message;
message.Append(kContainmentNotSatisfied);
message.Append(element->ComputedStyleRef().ViewTransitionName());
AddConsoleError(message.ReleaseString());
return false;
}
}

// We need to flatten the data first, and sort it by ordering which reflects
// the setElement ordering.
struct FlatData : public GarbageCollected<FlatData> {
Expand Down Expand Up @@ -786,9 +813,11 @@ bool ViewTransitionStyleTracker::RunPostPrePaintSteps() {

DCHECK_NE(element_data->target_element, document_->documentElement());
auto* layout_object = element_data->target_element->GetLayoutObject();
// TODO(khushalsagar): Verify that skipping a transition when things become
// display none is aligned with spec.
if (!layout_object) {
if (!layout_object || !SatisfiesContainment(*layout_object)) {
StringBuilder message;
message.Append(kContainmentNotSatisfied);
message.Append(entry.key);
AddConsoleError(message.ReleaseString());
return false;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
background: blue;
}
.shared {
contain: layout;
width: 100px;
height: 100px;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
}
.shared {
view-transition-name: shared;
contain: layout;
width: 100px;
height: 100px;
}
Expand All @@ -33,6 +34,7 @@
width: 10px;
height: 10px;
background: red;
contain: layout;
}

::view-transition-group(hidden) { animation-duration: 300s; }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
background: green;
}
.shared {
contain: layout;
width: 100px;
height: 100px;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
}
.shared {
view-transition-name: shared;
contain: layout;
width: 100px;
height: 100px;
}
Expand All @@ -34,6 +35,7 @@
width: 10px;
height: 10px;
background: red;
contain: layout;
}

::view-transition-group(hidden) { animation-duration: 300s; }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

<style>
div {
contain: layout;
position: absolute;
top: 50px;
width: 100px;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

<script src="/common/reftest-wait.js"></script>
<style>
div { contain: layout; }
#one {
background: green;
width: 100px;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
padding: 0;

view-transition-name: dialog;
contain: layout;
}

#target::backdrop {
Expand All @@ -25,6 +26,7 @@
background: grey;

view-transition-name: backdrop;
contain: layout;
}

.hidden {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
padding: 0;

view-transition-name: dialog;
contain: layout;
}

#target::backdrop {
Expand All @@ -25,6 +26,7 @@
background: grey;

view-transition-name: backdrop;
contain: layout;
}

.hidden {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,14 @@
height: 10px;
view-transition-name: hidden;
background: green;
contain: layout;
}

.target {
width: 100px;
height: 100px;
background: lightblue;
contain: layout;
view-transition-name: target;
}
.inner {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@
contain: paint;
border: 1px solid black;
}
.source {
contain: layout;
}
#target {
background: red;
position: absolute;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
width: 10px;
height: 10px;
view-transition-name: hidden;
contain: layout;
}

html::view-transition-group(target) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
width: 10px;
height: 10px;
view-transition-name: hidden;
contain: layout;
}

html::view-transition-group(target) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
<!DOCTYPE html>
<html>
<title>View transitions: transition skipped if no containment on new element after animation started</title>
<link rel="help" href="https://www.w3.org/TR/css-view-transitions-1/">
<link rel="author" href="mailto:khushalsagar@chromium.org">

<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>

<style>
div {
width: 100px;
height: 100px;
background: blue;
view-transition-name: target;
contain: paint;
}

html::view-transition-group(target) {
animation-duration: 300s;
}
</style>

<div id=first></div>

<script>
promise_test(async t => {
assert_implements(document.startViewTransition, "Missing document.startViewTransition");
return new Promise(async (resolve, reject) => {
let transition = document.startViewTransition();
await transition.updateCallbackDone;
await transition.ready;

transition.finished.then(resolve, reject);
first.style.contain = "none";
});
}, "new element becoming uncontained should skip the transition");
</script>
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[no-containment-on-new-element-mid-transition.html]
[new element becoming uncontained should skip the transition]
expected: FAIL
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
<!DOCTYPE html>
<html>
<title>View transitions: transition skipped if no containment on new element</title>
<link rel="help" href="https://www.w3.org/TR/css-view-transitions-1/">
<link rel="author" href="mailto:khushalsagar@chromium.org">

<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>

<style>
div {
width: 100px;
height: 100px;
background: blue;
view-transition-name: target;
contain: paint;
}
</style>

<div id=first></div>

<script>
promise_test(async t => {
assert_implements(document.startViewTransition, "Missing document.startViewTransition");
return new Promise(async (resolve, reject) => {
let transition = document.startViewTransition(() => {
first.style.contain = "none";
});

let readyRejected = false;
transition.ready.then(reject, () => { readyRejected = true; });

let updateCallbackDone = false;
transition.updateCallbackDone.then(() => { updateCallbackDone = true; }, reject);
transition.finished.then(() => {
assert_true(readyRejected, "ready not rejected");
assert_true(updateCallbackDone, "dom not updated");

if (window.getComputedStyle(first).contain == "none")
resolve();
else
reject("dom update callback did not run");

}, reject);
});
}, "uncontained new element should skip the transition");
</script>
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[no-containment-on-new-element.html]
[uncontained new element should skip the transition]
expected: FAIL
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
<!DOCTYPE html>
<html>
<title>View transitions: transition skipped if no containment on old element</title>
<link rel="help" href="https://www.w3.org/TR/css-view-transitions-1/">
<link rel="author" href="mailto:khushalsagar@chromium.org">

<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>

<style>
div {
width: 100px;
height: 100px;
background: blue;
view-transition-name: target;
}
</style>

<div id=first></div>

<script>
promise_test(async t => {
assert_implements(document.startViewTransition, "Missing document.startViewTransition");
return new Promise(async (resolve, reject) => {
let transition = document.startViewTransition(() => {
first.style.contain = "paint";
});

let readyRejected = false;
transition.ready.then(reject, () => { readyRejected = true; });

let updateCallbackDone = false;
transition.updateCallbackDone.then(() => { updateCallbackDone = true; }, reject);
transition.finished.then(() => {
assert_true(readyRejected, "ready not rejected");
assert_true(updateCallbackDone, "dom not updated");

if (window.getComputedStyle(first).contain == "paint")
resolve();
else
reject("dom update callback did not run");

}, reject);
});
}, "uncontained old element should skip the transition");
</script>
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[no-containment-on-old-element.html]
[uncontained old element should skip the transition]
expected: FAIL
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
width: 10px;
height: 10px;
view-transition-name: hidden;
contain: layout;
}

html::view-transition-group(target) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
width: 10px;
height: 10px;
view-transition-name: hidden;
contain: layout;
}

html::view-transition-group(target) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@
color: red;
}

div {
contain: layout;
}
</style>
<div id="target"></div>
<div id="target2"></div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@
color: red;
}

#target {
contain: layout;
}

</style>
<div id="target"></div>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@
color: red;
}

#target {
contain: layout;
}

</style>
<div id="target"></div>

Expand Down

0 comments on commit f2820f7

Please sign in to comment.