Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2072643002:
MD Settings: animation interface (Closed)
|
Created:
4 years, 6 months ago by michaelpg Modified:
4 years, 5 months ago Reviewers:
Dan Beam CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, jlklein+watch-closure_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, vitalyp+closure_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, dbeam+watch-closure_chromium.org, shans, dstockwell Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: step 1 of modular, testable Settings transitions
Settings animations, Settings UI, and the routing/navigation are all
tightly coupled. The animations are complicated[1], fragile and hard
to test; as a result, they constantly become janky. It's also hard to
add polish or write new transitions.
Step 1: simple wrappers around web animations.
* settings.animation.Animation adds a |finished| Promise which Chrome hasn't
implemented natively yet.
Step 2: interface for grouping animations to start/stop together.
* settings.animation.AnimationGroup is an abstract animation or group of
animations which can succeed or be cancelled, immediately or asynchronously.
This is just a nice interface around patterns we are using throughout
material WebUI.
Example usage: http://pastebin.com/GZegDqLh
A router doesn't have to keep track of the state of animations:
http://pastebin.com/YwCzmXFv
Step 3 is to port existing animations by wrapping them in AnimationGroup,
removing the weird TransitionBehavior, and writing tests.
Step 4 is to rework the router to make use of AnimationGroup Promises.
Step 5 (optional nice-to-have) is to separate navigation from routing
so navigation events can be queued, canceled and reversed more smoothly.
BUG=589681
R=dbeam@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
[1] https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settings_page/main_page_behavior.js?q=main_page_behavior.js&sq=package:chromium&dr&l=43-78
Committed: https://crrev.com/239ecba6156f4e179b09107953638f7c8d8db09e
Cr-Commit-Position: refs/heads/master@{#404097}
Patch Set 1 #
Total comments: 11
Patch Set 2 : simplify #
Total comments: 4
Patch Set 3 : Transition constructor => AnimationGroup interface #Patch Set 4 : fixes #Patch Set 5 : rebase #
Total comments: 3
Patch Set 6 : Remove AnimationGroup #Patch Set 7 : rebase why not #
Dependent Patchsets: Messages
Total messages: 37 (17 generated)
Description was changed from
==========
MD Settings: step 1 of lightweight transition framework
Step 1 of modular, testable transitions in Settings is described below.
Step 2 is to port existing animations by wrapping them in Transitions
and write tests.
Step 3 is to rework the router to make use of Transition Promises.
Step 4 (optional) is to separate navigation from routing so navigation
events can be queued.
Currently, Settings animations (primarily main_page_behavior.js,
transition_behavior.js and settings_animated_pages.js), Settings UI,
and the routing/navigation are all tightly coupled. Animations are hard
to reason about and hard to test; as a result, they easily become janky.
Step 1 introduces simple wrappers around animations and transitions. A
Transition is an abstract state change in the page, which can either
succeed or be cancelled, immediately or asynchronously. Transitions
may compose web animations or other Transistions.
Transitions, like Promises, are chainable; they represent an attempt to
change from one known state to another. Example usage:
// Attempts to open a section and navigate to a different sub-page.
var openSection = new OpenSectionTransition(section);
var changeSubpage = openSection.finished.then(function() {
// After opening the section, navigate to another sub-page.
return new ChangeSubpageTransition(subpage);
}, function() {
// openSection was canceled due to another router event; do nothing.
});
A router doesn't have to keep track of the state of transitions:
function closeSection() {
// A section is opened or opening. Try to cancel
// the opening transition.
this.openSectionTransition.cancel();
this.closeSectionTransition =
this.openSectionTransition.finished.then(function() {
// The section already finished opening; close it.
return new CloseSectionTransition();
}, function() {
// The opening was successfully canceled; do nothing.
return null;
});
}
BUG=589681
R=dbeam@chromium.org
==========
to
==========
MD Settings: step 1 of lightweight transition framework
Step 1 of modular, testable transitions in Settings is described below.
Step 2 is to port existing animations by wrapping them in Transitions
and write tests.
Step 3 is to rework the router to make use of Transition Promises.
Step 4 (optional) is to separate navigation from routing so navigation
events can be queued.
Currently, Settings animations (primarily main_page_behavior.js,
transition_behavior.js and settings_animated_pages.js), Settings UI,
and the routing/navigation are all tightly coupled. Animations are hard
to reason about and hard to test; as a result, they easily become janky.
Step 1 introduces simple wrappers around animations and transitions. A
Transition is an abstract state change in the page, which can either
succeed or be cancelled, immediately or asynchronously. Transitions
may compose web animations or other Transistions.
Transitions, like Promises, are chainable; they represent an attempt to
change from one known state to another. Example usage:
// Attempts to open a section and navigate to a different sub-page.
var openSection = new OpenSectionTransition(section);
var changeSubpage = openSection.finished.then(function() {
// After opening the section, navigate to another sub-page.
return new ChangeSubpageTransition(subpage);
}, function() {
// openSection was canceled due to another router event; do nothing.
});
A router doesn't have to keep track of the state of transitions:
function closeSection() {
// A section is opened or opening. Try to cancel
// the opening transition.
this.openSectionTransition.cancel();
this.closeSectionTransition =
this.openSectionTransition.finished.then(function() {
// The section already finished opening; close it.
return new CloseSectionTransition();
}, function() {
// The opening was successfully canceled; do nothing.
return null;
});
}
BUG=589681
R=dbeam@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========
Description was changed from
==========
MD Settings: step 1 of lightweight transition framework
Step 1 of modular, testable transitions in Settings is described below.
Step 2 is to port existing animations by wrapping them in Transitions
and write tests.
Step 3 is to rework the router to make use of Transition Promises.
Step 4 (optional) is to separate navigation from routing so navigation
events can be queued.
Currently, Settings animations (primarily main_page_behavior.js,
transition_behavior.js and settings_animated_pages.js), Settings UI,
and the routing/navigation are all tightly coupled. Animations are hard
to reason about and hard to test; as a result, they easily become janky.
Step 1 introduces simple wrappers around animations and transitions. A
Transition is an abstract state change in the page, which can either
succeed or be cancelled, immediately or asynchronously. Transitions
may compose web animations or other Transistions.
Transitions, like Promises, are chainable; they represent an attempt to
change from one known state to another. Example usage:
// Attempts to open a section and navigate to a different sub-page.
var openSection = new OpenSectionTransition(section);
var changeSubpage = openSection.finished.then(function() {
// After opening the section, navigate to another sub-page.
return new ChangeSubpageTransition(subpage);
}, function() {
// openSection was canceled due to another router event; do nothing.
});
A router doesn't have to keep track of the state of transitions:
function closeSection() {
// A section is opened or opening. Try to cancel
// the opening transition.
this.openSectionTransition.cancel();
this.closeSectionTransition =
this.openSectionTransition.finished.then(function() {
// The section already finished opening; close it.
return new CloseSectionTransition();
}, function() {
// The opening was successfully canceled; do nothing.
return null;
});
}
BUG=589681
R=dbeam@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========
to
==========
MD Settings: step 1 of modular, testable Settings transitions
Settings animations, Settings UI, and the routing/navigation are all
tightly coupled. The animations are complicated, fragile and hard to
test; as a result, they easily become janky. It's also hard to add
polish or write new transitions.
Step 1: simple wrappers around animations and transitions. A Transition
is an abstract state change in the page, which can succeed or be cancelled,
immediately or asynchronously. Transitions may compose web animations or
other Transistions.
Transitions, like Promises, are chainable; they represent an attempt to
change from one known state to another. Example usage:
http://pastebin.com/GZegDqLh
A router doesn't have to keep track of the state of transitions:
http://pastebin.com/YwCzmXFv
Step 2 is to port existing animations by wrapping them in Transitions
and write tests.
Step 3 is to rework the router to make use of Transition Promises.
Step 4 (optional nice-to-have) is to separate navigation from routing
so navigation events can be queued, canceled and reversed more smoothly.
BUG=589681
R=dbeam@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========
Description was changed from ========== MD Settings: step 1 of modular, testable Settings transitions Settings animations, Settings UI, and the routing/navigation are all tightly coupled. The animations are complicated, fragile and hard to test; as a result, they easily become janky. It's also hard to add polish or write new transitions. Step 1: simple wrappers around animations and transitions. A Transition is an abstract state change in the page, which can succeed or be cancelled, immediately or asynchronously. Transitions may compose web animations or other Transistions. Transitions, like Promises, are chainable; they represent an attempt to change from one known state to another. Example usage: http://pastebin.com/GZegDqLh A router doesn't have to keep track of the state of transitions: http://pastebin.com/YwCzmXFv Step 2 is to port existing animations by wrapping them in Transitions and write tests. Step 3 is to rework the router to make use of Transition Promises. Step 4 (optional nice-to-have) is to separate navigation from routing so navigation events can be queued, canceled and reversed more smoothly. BUG=589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: step 1 of modular, testable Settings transitions Settings animations, Settings UI, and the routing/navigation are all tightly coupled. The animations are complicated, fragile and hard to test; as a result, they easily become janky. It's also hard to add polish or write new transitions. Step 1: simple wrappers around animations and transitions. A Transition is an abstract state change in the page, which can succeed or be cancelled, immediately or asynchronously. Transitions may compose web animations or other Transitions. Transitions, like Promises, are chainable; they represent an attempt to change from one known state to another. Example usage: http://pastebin.com/GZegDqLh A router doesn't have to keep track of the state of transitions: http://pastebin.com/YwCzmXFv Step 2 is to port existing animations by wrapping them in Transitions and write tests. Step 3 is to rework the router to make use of Transition Promises. Step 4 (optional nice-to-have) is to separate navigation from routing so navigation events can be queued, canceled and reversed more smoothly. BUG=589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: step 1 of modular, testable Settings transitions Settings animations, Settings UI, and the routing/navigation are all tightly coupled. The animations are complicated, fragile and hard to test; as a result, they easily become janky. It's also hard to add polish or write new transitions. Step 1: simple wrappers around animations and transitions. A Transition is an abstract state change in the page, which can succeed or be cancelled, immediately or asynchronously. Transitions may compose web animations or other Transitions. Transitions, like Promises, are chainable; they represent an attempt to change from one known state to another. Example usage: http://pastebin.com/GZegDqLh A router doesn't have to keep track of the state of transitions: http://pastebin.com/YwCzmXFv Step 2 is to port existing animations by wrapping them in Transitions and write tests. Step 3 is to rework the router to make use of Transition Promises. Step 4 (optional nice-to-have) is to separate navigation from routing so navigation events can be queued, canceled and reversed more smoothly. BUG=589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: step 1 of modular, testable Settings transitions Settings animations, Settings UI, and the routing/navigation are all tightly coupled. The animations are complicated[1], fragile and hard to test; as a result, they easily become janky. It's also hard to add polish or write new transitions. Step 1: simple wrappers around animations and transitions. A Transition is an abstract state change in the page, which can succeed or be cancelled, immediately or asynchronously. Transitions may compose web animations or other Transitions. Transitions, like Promises, are chainable; they represent an attempt to change from one known state to another. Example usage: http://pastebin.com/GZegDqLh A router doesn't have to keep track of the state of transitions: http://pastebin.com/YwCzmXFv Step 2 is to port existing animations by wrapping them in Transitions and write tests. Step 3 is to rework the router to make use of Transition Promises. Step 4 (optional nice-to-have) is to separate navigation from routing so navigation events can be queued, canceled and reversed more smoothly. BUG=589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation [1] https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin... ==========
+shans, dstockwell
On 2016/06/17 19:02:42, Dan Beam wrote: > +shans, dstockwell Hi guys. The TL;DR: is that this polyfills Animation.prototype.finished, and adds a "Transition" interface we can use to compose and test page transitions without importing all of neon-animations and web-animations-js. I'm totally open to feedback on the code or my example usage from the description.
https://codereview.chromium.org/2072643002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/animation/animation.js (right): https://codereview.chromium.org/2072643002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/animation/animation.js:45: resolve(self.animation_); why are we keeping |animation_| private but passing it to the callback on resolution? https://codereview.chromium.org/2072643002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/animation/animation.js:51: }); can you combine this code into: function dispatchAfter(e) { setTimeout(function() { self.dispatchEvent(e); self.animation_ = undefined; }); } https://codereview.chromium.org/2072643002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/animation/animation.js:54: reject(new afaict, we should not reject for a cancelled animation as this seems validly possible, and in the paradigm of promise, only stuff that would throw should reject https://codereview.chromium.org/2072643002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/animation/animation.js:58: **/( **/ -> */ https://codereview.chromium.org/2072643002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/animation/transition.js (right): https://codereview.chromium.org/2072643002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/animation/transition.js:59: function ImmediateTransition() {} i don't like this design. the name seems like an oxymoron (many people's definition of a transition involves a time period). you're creating a concrete class then disabling some functionality can we maybe make a {Step, State, Teleport, Style} have the Transition own 1-2 (end and maybe start?) instead?
https://codereview.chromium.org/2072643002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/animation/transition.js (right): https://codereview.chromium.org/2072643002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/animation/transition.js:59: function ImmediateTransition() {} On 2016/06/17 19:22:42, Dan Beam wrote: > i don't like this design. > > the name seems like an oxymoron (many people's definition of a transition > involves a time period). > > you're creating a concrete class then disabling some functionality > > can we maybe make a {Step, State, Teleport, Style} have the Transition own 1-2 > (end and maybe start?) instead? if you want to talk about this in a more active medium (chat, vc) possibly with other folks (i.e. neon-animation folks, web platform folks, etc.) we can if you want.
see usage in follow-up, coming soon to a CL near you https://codereview.chromium.org/2072643002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/animation/animation.js (right): https://codereview.chromium.org/2072643002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/animation/animation.js:45: resolve(self.animation_); On 2016/06/17 19:22:42, Dan Beam wrote: > why are we keeping |animation_| private but passing it to the callback on > resolution? removed from resolve https://codereview.chromium.org/2072643002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/animation/animation.js:51: }); On 2016/06/17 19:22:42, Dan Beam wrote: > can you combine this code into: > > function dispatchAfter(e) { > setTimeout(function() { > self.dispatchEvent(e); > self.animation_ = undefined; > }); > } Done. https://codereview.chromium.org/2072643002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/animation/animation.js:54: reject(new On 2016/06/17 19:22:42, Dan Beam wrote: > afaict, we should not reject for a cancelled animation as this seems validly > possible, and in the paradigm of promise, only stuff that would throw should > reject there is a TODO in the spec to make this "cancel" the promise if cancelable promises are ever added. until then, this is the behavior the spec prescribes, so it's the behavior consumers should expect. once the "finished" promise is implemented by Chrome we'll just delete this code; if we gave it different behavior, we'd have to keep it around. https://codereview.chromium.org/2072643002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/animation/animation.js:58: **/( On 2016/06/17 19:22:42, Dan Beam wrote: > **/ -> */ Done. https://codereview.chromium.org/2072643002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/animation/transition.js (right): https://codereview.chromium.org/2072643002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/animation/transition.js:59: function ImmediateTransition() {} On 2016/06/17 19:22:42, Dan Beam wrote: > i don't like this design. > > the name seems like an oxymoron (many people's definition of a transition > involves a time period). > > you're creating a concrete class then disabling some functionality > > can we maybe make a {Step, State, Teleport, Style} have the Transition own 1-2 > (end and maybe start?) instead? I've removed this, it hasn't been useful yet.
can you remove transition.js from this CL or explain why it's useful yet? i still don't fully understand the difference between animation and transition and looked briefly at the downstream CLs that depend on this issue and it didn't help clarify https://codereview.chromium.org/2072643002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/animation/transition.js (right): https://codereview.chromium.org/2072643002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/animation/transition.js:27: play: function() {}, shouldn't this also be assertNotReached() or something?
https://codereview.chromium.org/2072643002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/animation/animation.js (right): https://codereview.chromium.org/2072643002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/animation/animation.js:41: this.finished = new Promise(function(resolve, reject) { so the point of this class is just basically to add this |finished| promise, right?
ok, I read your jsbins. this makes slightly more sense to me now. can we rename Transition to something else. something uses "Chain", "Group", or "Timeline"? feel free to use a design doc instead of CLs to verify your design (less global search and replace when we disagree on naming ;)). also, if I come from a web-dev background I consider transition/animation direct mappings to @keyframes and transition:, which is mildly confusing.
Description was changed from ========== MD Settings: step 1 of modular, testable Settings transitions Settings animations, Settings UI, and the routing/navigation are all tightly coupled. The animations are complicated[1], fragile and hard to test; as a result, they easily become janky. It's also hard to add polish or write new transitions. Step 1: simple wrappers around animations and transitions. A Transition is an abstract state change in the page, which can succeed or be cancelled, immediately or asynchronously. Transitions may compose web animations or other Transitions. Transitions, like Promises, are chainable; they represent an attempt to change from one known state to another. Example usage: http://pastebin.com/GZegDqLh A router doesn't have to keep track of the state of transitions: http://pastebin.com/YwCzmXFv Step 2 is to port existing animations by wrapping them in Transitions and write tests. Step 3 is to rework the router to make use of Transition Promises. Step 4 (optional nice-to-have) is to separate navigation from routing so navigation events can be queued, canceled and reversed more smoothly. BUG=589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation [1] https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin... ========== to ========== MD Settings: step 1 of modular, testable Settings transitions Settings animations, Settings UI, and the routing/navigation are all tightly coupled. The animations are complicated[1], fragile and hard to test; as a result, they constantly become janky. It's also hard to add polish or write new transitions. Step 1: simple wrappers around animations and transitions. * settings.animation.Animation adds a |finished| Promise which Chrome hasn't implemented natiely yet. * settings.animation.AnimationGroup is an abstract animation or group of animations which can succeed or be cancelled, immediately or asynchronously. This is just a nice interface around patterns we are using throughout material WebUI. Example usage: http://pastebin.com/GZegDqLh A router doesn't have to keep track of the state of animations: http://pastebin.com/YwCzmXFv Step 2 is to port existing animations by wrapping them in AnimationGroup, removing the weird TransitionBehavior, and writing tests. Step 3 is to rework the router to make use of Transition Promises. Step 4 (optional nice-to-have) is to separate navigation from routing so navigation events can be queued, canceled and reversed more smoothly. BUG=589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation [1] https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin... ==========
Description was changed from ========== MD Settings: step 1 of modular, testable Settings transitions Settings animations, Settings UI, and the routing/navigation are all tightly coupled. The animations are complicated[1], fragile and hard to test; as a result, they constantly become janky. It's also hard to add polish or write new transitions. Step 1: simple wrappers around animations and transitions. * settings.animation.Animation adds a |finished| Promise which Chrome hasn't implemented natiely yet. * settings.animation.AnimationGroup is an abstract animation or group of animations which can succeed or be cancelled, immediately or asynchronously. This is just a nice interface around patterns we are using throughout material WebUI. Example usage: http://pastebin.com/GZegDqLh A router doesn't have to keep track of the state of animations: http://pastebin.com/YwCzmXFv Step 2 is to port existing animations by wrapping them in AnimationGroup, removing the weird TransitionBehavior, and writing tests. Step 3 is to rework the router to make use of Transition Promises. Step 4 (optional nice-to-have) is to separate navigation from routing so navigation events can be queued, canceled and reversed more smoothly. BUG=589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation [1] https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin... ========== to ========== MD Settings: step 1 of modular, testable Settings transitions Settings animations, Settings UI, and the routing/navigation are all tightly coupled. The animations are complicated[1], fragile and hard to test; as a result, they constantly become janky. It's also hard to add polish or write new transitions. Step 1: simple wrappers around animations and transitions. * settings.animation.Animation adds a |finished| Promise which Chrome hasn't implemented natiely yet. * settings.animation.AnimationGroup is an abstract animation or group of animations which can succeed or be cancelled, immediately or asynchronously. This is just a nice interface around patterns we are using throughout material WebUI. Example usage: http://pastebin.com/GZegDqLh A router doesn't have to keep track of the state of animations: http://pastebin.com/YwCzmXFv Step 2 is to port existing animations by wrapping them in AnimationGroup, removing the weird TransitionBehavior, and writing tests. Step 3 is to rework the router to make use of AnimationGroup Promises. Step 4 (optional nice-to-have) is to separate navigation from routing so navigation events can be queued, canceled and reversed more smoothly. BUG=589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation [1] https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin... ==========
https://codereview.chromium.org/2072643002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/animation/animation.js (right): https://codereview.chromium.org/2072643002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/animation/animation.js:41: this.finished = new Promise(function(resolve, reject) { On 2016/07/01 00:33:58, Dan Beam wrote: > so the point of this class is just basically to add this |finished| promise, > right? Yes, that's entirely the point. Once the platform adds |finished| we replace all new settings.animation.Animation(el, ...) with el.animate(...) web-animations-next-lite does something similar but ideally we remove that (crbug.com/584126) https://codereview.chromium.org/2072643002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/animation/transition.js (right): https://codereview.chromium.org/2072643002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/animation/transition.js:27: play: function() {}, On 2016/07/01 00:33:24, Dan Beam wrote: > shouldn't this also be assertNotReached() or something? Done.
On 2016/07/01 01:20:44, Dan Beam wrote: > ok, I read your jsbins. this makes slightly more sense to me now. > > can we rename Transition to something else. something uses "Chain", "Group", or > "Timeline"? I've renamed to AnimationGroup and made it an @interface. I also added more explicit comments explaining the desired usage and benefits. It's supposed to be an extremely simple interface, so if it still doesn't make sense, let's hammer it out in a hangout. > feel free to use a design doc instead of CLs to verify your design > (less global search and replace when we disagree on naming ;)). I don't mind search-and-replacing. I would prefer to land this now rather than by committee. If people want to bikeshed later, I'm fine with that. > > also, if I come from a web-dev background I consider transition/animation direct > mappings to @keyframes and transition:, which is mildly confusing. Not sure what to tell you. I could add some docs to our sites page about types of animations but they're well-documented externally.
i would still arguably move animation group into the next CL (i.e. when you actually use it), but none of this code seems to actually be used from anywhere but tests yet, so whatever you're going to use it Real Soon Now anyways lgtm, this looks like a great improvement! I do wonder, though, should the type "Animation" be used or referenced anywhere in "AnimationGroup"? is it intentional to omit this to allow more freedom for those implementing AnimationGroup? https://codereview.chromium.org/2072643002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/animation/animation_group.js (right): https://codereview.chromium.org/2072643002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/animation/animation_group.js:42: finished: null, can this really be null? https://codereview.chromium.org/2072643002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/animation_browsertest.js (right): https://codereview.chromium.org/2072643002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/animation_browsertest.js:41: expectFalse(true, 'Animation fired finish event before resolving promise'); don't we have a "fail()" instead of "expectFalse(true"?
Patchset #7 (id:120001) has been deleted
On 2016/07/01 23:22:24, Dan Beam wrote: > i would still arguably move animation group into the next CL (i.e. when you > actually use it), but none of this code seems to actually be used from anywhere > but tests yet, so whatever > > you're going to use it Real Soon Now anyways meh, I've removed it, let's see how we feel in the next CL https://codereview.chromium.org/2072643002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/animation_browsertest.js (right): https://codereview.chromium.org/2072643002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/animation_browsertest.js:41: expectFalse(true, 'Animation fired finish event before resolving promise'); On 2016/07/01 23:22:24, Dan Beam wrote: > don't we have a "fail()" instead of "expectFalse(true"? chai.assert.fail(null, null, message) is the closest I can find we use assertNotReached elsewhere in tests, so how's that?
Description was changed from ========== MD Settings: step 1 of modular, testable Settings transitions Settings animations, Settings UI, and the routing/navigation are all tightly coupled. The animations are complicated[1], fragile and hard to test; as a result, they constantly become janky. It's also hard to add polish or write new transitions. Step 1: simple wrappers around animations and transitions. * settings.animation.Animation adds a |finished| Promise which Chrome hasn't implemented natiely yet. * settings.animation.AnimationGroup is an abstract animation or group of animations which can succeed or be cancelled, immediately or asynchronously. This is just a nice interface around patterns we are using throughout material WebUI. Example usage: http://pastebin.com/GZegDqLh A router doesn't have to keep track of the state of animations: http://pastebin.com/YwCzmXFv Step 2 is to port existing animations by wrapping them in AnimationGroup, removing the weird TransitionBehavior, and writing tests. Step 3 is to rework the router to make use of AnimationGroup Promises. Step 4 (optional nice-to-have) is to separate navigation from routing so navigation events can be queued, canceled and reversed more smoothly. BUG=589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation [1] https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin... ========== to ========== MD Settings: step 1 of modular, testable Settings transitions Settings animations, Settings UI, and the routing/navigation are all tightly coupled. The animations are complicated[1], fragile and hard to test; as a result, they constantly become janky. It's also hard to add polish or write new transitions. Step 1: simple wrappers around web animations. * settings.animation.Animation adds a |finished| Promise which Chrome hasn't implemented natively yet. Step 2: interface for grouping animations to start/stop together. * settings.animation.AnimationGroup is an abstract animation or group of animations which can succeed or be cancelled, immediately or asynchronously. This is just a nice interface around patterns we are using throughout material WebUI. Example usage: http://pastebin.com/GZegDqLh A router doesn't have to keep track of the state of animations: http://pastebin.com/YwCzmXFv Step 3 is to port existing animations by wrapping them in AnimationGroup, removing the weird TransitionBehavior, and writing tests. Step 4 is to rework the router to make use of AnimationGroup Promises. Step 5 (optional nice-to-have) is to separate navigation from routing so navigation events can be queued, canceled and reversed more smoothly. BUG=589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation [1] https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin... ==========
The CQ bit was checked by michaelpg@chromium.org
The CQ bit was unchecked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2072643002/#ps140001 (title: "rebase why not")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by michaelpg@chromium.org
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== MD Settings: step 1 of modular, testable Settings transitions Settings animations, Settings UI, and the routing/navigation are all tightly coupled. The animations are complicated[1], fragile and hard to test; as a result, they constantly become janky. It's also hard to add polish or write new transitions. Step 1: simple wrappers around web animations. * settings.animation.Animation adds a |finished| Promise which Chrome hasn't implemented natively yet. Step 2: interface for grouping animations to start/stop together. * settings.animation.AnimationGroup is an abstract animation or group of animations which can succeed or be cancelled, immediately or asynchronously. This is just a nice interface around patterns we are using throughout material WebUI. Example usage: http://pastebin.com/GZegDqLh A router doesn't have to keep track of the state of animations: http://pastebin.com/YwCzmXFv Step 3 is to port existing animations by wrapping them in AnimationGroup, removing the weird TransitionBehavior, and writing tests. Step 4 is to rework the router to make use of AnimationGroup Promises. Step 5 (optional nice-to-have) is to separate navigation from routing so navigation events can be queued, canceled and reversed more smoothly. BUG=589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation [1] https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin... ========== to ========== MD Settings: step 1 of modular, testable Settings transitions Settings animations, Settings UI, and the routing/navigation are all tightly coupled. The animations are complicated[1], fragile and hard to test; as a result, they constantly become janky. It's also hard to add polish or write new transitions. Step 1: simple wrappers around web animations. * settings.animation.Animation adds a |finished| Promise which Chrome hasn't implemented natively yet. Step 2: interface for grouping animations to start/stop together. * settings.animation.AnimationGroup is an abstract animation or group of animations which can succeed or be cancelled, immediately or asynchronously. This is just a nice interface around patterns we are using throughout material WebUI. Example usage: http://pastebin.com/GZegDqLh A router doesn't have to keep track of the state of animations: http://pastebin.com/YwCzmXFv Step 3 is to port existing animations by wrapping them in AnimationGroup, removing the weird TransitionBehavior, and writing tests. Step 4 is to rework the router to make use of AnimationGroup Promises. Step 5 (optional nice-to-have) is to separate navigation from routing so navigation events can be queued, canceled and reversed more smoothly. BUG=589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation [1] https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== MD Settings: step 1 of modular, testable Settings transitions Settings animations, Settings UI, and the routing/navigation are all tightly coupled. The animations are complicated[1], fragile and hard to test; as a result, they constantly become janky. It's also hard to add polish or write new transitions. Step 1: simple wrappers around web animations. * settings.animation.Animation adds a |finished| Promise which Chrome hasn't implemented natively yet. Step 2: interface for grouping animations to start/stop together. * settings.animation.AnimationGroup is an abstract animation or group of animations which can succeed or be cancelled, immediately or asynchronously. This is just a nice interface around patterns we are using throughout material WebUI. Example usage: http://pastebin.com/GZegDqLh A router doesn't have to keep track of the state of animations: http://pastebin.com/YwCzmXFv Step 3 is to port existing animations by wrapping them in AnimationGroup, removing the weird TransitionBehavior, and writing tests. Step 4 is to rework the router to make use of AnimationGroup Promises. Step 5 (optional nice-to-have) is to separate navigation from routing so navigation events can be queued, canceled and reversed more smoothly. BUG=589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation [1] https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin... ========== to ========== MD Settings: step 1 of modular, testable Settings transitions Settings animations, Settings UI, and the routing/navigation are all tightly coupled. The animations are complicated[1], fragile and hard to test; as a result, they constantly become janky. It's also hard to add polish or write new transitions. Step 1: simple wrappers around web animations. * settings.animation.Animation adds a |finished| Promise which Chrome hasn't implemented natively yet. Step 2: interface for grouping animations to start/stop together. * settings.animation.AnimationGroup is an abstract animation or group of animations which can succeed or be cancelled, immediately or asynchronously. This is just a nice interface around patterns we are using throughout material WebUI. Example usage: http://pastebin.com/GZegDqLh A router doesn't have to keep track of the state of animations: http://pastebin.com/YwCzmXFv Step 3 is to port existing animations by wrapping them in AnimationGroup, removing the weird TransitionBehavior, and writing tests. Step 4 is to rework the router to make use of AnimationGroup Promises. Step 5 (optional nice-to-have) is to separate navigation from routing so navigation events can be queued, canceled and reversed more smoothly. BUG=589681 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation [1] https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin... Committed: https://crrev.com/239ecba6156f4e179b09107953638f7c8d8db09e Cr-Commit-Position: refs/heads/master@{#404097} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/239ecba6156f4e179b09107953638f7c8d8db09e Cr-Commit-Position: refs/heads/master@{#404097} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
