Devtools: Replace split widget rAF animation with web animation (issue 1156953002 by samli@chromium.org)

0 views
Skip to first unread message

sa...@chromium.org

unread,
May 25, 2015, 9:57:03 PM5/25/15
to pfel...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, yurys...@chromium.org
Reviewers: pfeldman,

Description:
Devtools: Replace split widget rAF animation with web animation

The current split widget animation overflows content over other widgets.
This change replaces the animation implementation with a native web
animation.

Please review this at https://codereview.chromium.org/1156953002/

Base URL: svn://svn.chromium.org/blink/trunk

Affected files (+11, -50 lines):
M Source/devtools/front_end/ui/SplitWidget.js
M Source/devtools/front_end/ui/splitWidget.css


Index: Source/devtools/front_end/ui/SplitWidget.js
diff --git a/Source/devtools/front_end/ui/SplitWidget.js
b/Source/devtools/front_end/ui/SplitWidget.js
index
592e2a1227e47dd1c39aa0fc2c032daf8c9cc28a..a779861bbb9116854535f76600e01029bb4ade68
100644
--- a/Source/devtools/front_end/ui/SplitWidget.js
+++ b/Source/devtools/front_end/ui/SplitWidget.js
@@ -508,7 +508,6 @@ WebInspector.SplitWidget.prototype = {
*/
_animate: function(reverse, callback)
{
- var animationTime = 50;
this._animationCallback = callback;

var animatedMarginPropertyName;
@@ -518,52 +517,14 @@ WebInspector.SplitWidget.prototype = {
animatedMarginPropertyName =
this._secondIsSidebar ? "margin-bottom" : "margin-top";

var marginFrom = reverse ? "0" : "-" +
WebInspector.zoomManager.dipToCSS(this._sidebarSizeDIP) + "px";
- var marginTo = reverse ? "-" +
WebInspector.zoomManager.dipToCSS(this._sidebarSizeDIP) + "px" : "0";
-
- // This order of things is important.
- // 1. Resize main element early and force layout.
- this.contentElement.style.setProperty(animatedMarginPropertyName,
marginFrom);
- if (!reverse) {
- suppressUnused(this._mainElement.offsetWidth);
- suppressUnused(this._sidebarElement.offsetWidth);
- }
-
- // 2. Issue onresize to the sidebar element, its size won't change.
- if (!reverse)
- this._sidebarWidget.doResize();
-
- // 3. Configure and run animation
- this.contentElement.style.setProperty("transition",
animatedMarginPropertyName + " " + animationTime + "ms linear");
+ var marginTo = reverse ? "-" +
WebInspector.zoomManager.dipToCSS(this._sidebarSizeDIP) + "px" : "0";

- var boundAnimationFrame;
- var startTime;
- /**
- * @this {WebInspector.SplitWidget}
- */
- function animationFrame()
- {
- delete this._animationFrameHandle;
-
- if (!startTime) {
- // Kick animation on first frame.
-
this.contentElement.style.setProperty(animatedMarginPropertyName, marginTo);
- startTime = window.performance.now();
- } else if (window.performance.now() < startTime +
animationTime) {
- // Process regular animation frame.
- if (this._mainWidget)
- this._mainWidget.doResize();
- } else {
- // Complete animation.
- this._cancelAnimation();
- if (this._mainWidget)
- this._mainWidget.doResize();
-
this.dispatchEventToListeners(WebInspector.SplitWidget.Events.SidebarSizeChanged,
this.sidebarSize());
- return;
- }
- this._animationFrameHandle =
this.contentElement.window().requestAnimationFrame(boundAnimationFrame);
- }
- boundAnimationFrame = animationFrame.bind(this);
- this._animationFrameHandle =
this.contentElement.window().requestAnimationFrame(boundAnimationFrame);
+ this.contentElement.style.setProperty(animatedMarginPropertyName,
marginTo);
+ var keyframes = [{}, {}];
+ keyframes[0][animatedMarginPropertyName] = marginFrom;
+ keyframes[1][animatedMarginPropertyName] = marginTo;
+ var player = this.contentElement.animate(keyframes, { duration:
150, easing: "cubic-bezier(0, 0, 0.2, 1)" });
+ player.onfinish = this._cancelAnimation.bind(this);
},

_cancelAnimation: function()
@@ -574,10 +535,6 @@ WebInspector.SplitWidget.prototype = {
this.contentElement.style.removeProperty("margin-left");
this.contentElement.style.removeProperty("transition");

- if (this._animationFrameHandle) {
-
this.contentElement.window().cancelAnimationFrame(this._animationFrameHandle);
- delete this._animationFrameHandle;
- }
if (this._animationCallback) {
this._animationCallback();
delete this._animationCallback;
Index: Source/devtools/front_end/ui/splitWidget.css
diff --git a/Source/devtools/front_end/ui/splitWidget.css
b/Source/devtools/front_end/ui/splitWidget.css
index
113b8b787967ca2439ee894ca91f626f9148183b..817013fc0424431f589e0a500763834cac4843af
100644
--- a/Source/devtools/front_end/ui/splitWidget.css
+++ b/Source/devtools/front_end/ui/splitWidget.css
@@ -26,6 +26,10 @@
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/

+:host {
+ overflow: hidden;
+}
+
.shadow-split-widget {
display: flex;
overflow: hidden;


pfel...@chromium.org

unread,
May 26, 2015, 2:01:14 AM5/26/15
to sa...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, yurys...@chromium.org

https://codereview.chromium.org/1156953002/diff/20001/Source/devtools/front_end/ui/SplitWidget.js
File Source/devtools/front_end/ui/SplitWidget.js (left):

https://codereview.chromium.org/1156953002/diff/20001/Source/devtools/front_end/ui/SplitWidget.js#oldcode554
Source/devtools/front_end/ui/SplitWidget.js:554:
this._mainWidget.doResize();
It might be important to make this call.

https://codereview.chromium.org/1156953002/diff/20001/Source/devtools/front_end/ui/SplitWidget.js#oldcode559
Source/devtools/front_end/ui/SplitWidget.js:559:
this._mainWidget.doResize();
It is definitely important to make this call.

https://codereview.chromium.org/1156953002/

sa...@chromium.org

unread,
May 26, 2015, 3:22:43 AM5/26/15
to pfel...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, yurys...@chromium.org
ptal. Latest change uses raf simply to make the mainWidget.doResize() call.
I
think the animation looks better and the code simpler.


https://codereview.chromium.org/1156953002/diff/20001/Source/devtools/front_end/ui/SplitWidget.js
File Source/devtools/front_end/ui/SplitWidget.js (left):

https://codereview.chromium.org/1156953002/diff/20001/Source/devtools/front_end/ui/SplitWidget.js#oldcode554
Source/devtools/front_end/ui/SplitWidget.js:554:
this._mainWidget.doResize();
On 2015/05/26 06:01:14, pfeldman wrote:
> It might be important to make this call.

Done.

https://codereview.chromium.org/1156953002/diff/20001/Source/devtools/front_end/ui/SplitWidget.js#oldcode559
Source/devtools/front_end/ui/SplitWidget.js:559:
this._mainWidget.doResize();
On 2015/05/26 06:01:14, pfeldman wrote:
> It is definitely important to make this call.

In some cases, this is handled in the callback(). Even without any of
these calls, the viewport resized correctly when the animation finished.
Not all animate calls have a callback though.

https://codereview.chromium.org/1156953002/

dgo...@chromium.org

unread,
May 26, 2015, 7:46:32 AM5/26/15
to sa...@chromium.org, pfel...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, yurys...@chromium.org

https://codereview.chromium.org/1156953002/diff/40001/Source/devtools/front_end/ui/SplitWidget.js
File Source/devtools/front_end/ui/SplitWidget.js (left):

https://codereview.chromium.org/1156953002/diff/40001/Source/devtools/front_end/ui/SplitWidget.js#oldcode560
Source/devtools/front_end/ui/SplitWidget.js:560:
this.dispatchEventToListeners(WebInspector.SplitWidget.Events.SidebarSizeChanged,
this.sidebarSize());
This one is gone.

https://codereview.chromium.org/1156953002/diff/40001/Source/devtools/front_end/ui/SplitWidget.js
File Source/devtools/front_end/ui/SplitWidget.js (right):

https://codereview.chromium.org/1156953002/diff/40001/Source/devtools/front_end/ui/SplitWidget.js#newcode538
Source/devtools/front_end/ui/SplitWidget.js:538: var player =
this.contentElement.animate(keyframes, { duration: 150, easing:
"cubic-bezier(0, 0, 0.2, 1)" });
Note that duration was 50 before. Accidental change?

https://codereview.chromium.org/1156953002/

sa...@chromium.org

unread,
May 26, 2015, 9:41:27 PM5/26/15
to dgo...@chromium.org, pfel...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, yurys...@chromium.org

https://codereview.chromium.org/1156953002/diff/40001/Source/devtools/front_end/ui/SplitWidget.js
File Source/devtools/front_end/ui/SplitWidget.js (left):

https://codereview.chromium.org/1156953002/diff/40001/Source/devtools/front_end/ui/SplitWidget.js#oldcode560
Source/devtools/front_end/ui/SplitWidget.js:560:
this.dispatchEventToListeners(WebInspector.SplitWidget.Events.SidebarSizeChanged,
this.sidebarSize());
On 2015/05/26 11:46:32, dgozman wrote:
> This one is gone.

Done.

https://codereview.chromium.org/1156953002/diff/40001/Source/devtools/front_end/ui/SplitWidget.js
File Source/devtools/front_end/ui/SplitWidget.js (right):

https://codereview.chromium.org/1156953002/diff/40001/Source/devtools/front_end/ui/SplitWidget.js#newcode538
Source/devtools/front_end/ui/SplitWidget.js:538: var player =
this.contentElement.animate(keyframes, { duration: 150, easing:
"cubic-bezier(0, 0, 0.2, 1)" });
On 2015/05/26 11:46:32, dgozman wrote:
> Note that duration was 50 before. Accidental change?

Intentional. 100-150ms allows for user perception to see and understand
the transition being made, while still being really fast compared to the
100ms window to respond to user input.

50ms means you have about 3 frames to work with, which usually means it
looks janky/flickering. Especially true given that hiding a widget is
quite a big state transition.

The curve will still make this look really speedy but smooth. It's the
standard material "leaving" easing.

https://codereview.chromium.org/1156953002/

dgo...@chromium.org

unread,
May 28, 2015, 7:43:53 AM5/28/15
to sa...@chromium.org, pfel...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, yurys...@chromium.org

https://codereview.chromium.org/1156953002/diff/60001/Source/devtools/front_end/ui/SplitWidget.js
File Source/devtools/front_end/ui/SplitWidget.js (left):

https://codereview.chromium.org/1156953002/diff/60001/Source/devtools/front_end/ui/SplitWidget.js#oldcode533
Source/devtools/front_end/ui/SplitWidget.js:533:
this._sidebarWidget.doResize();
This seems valuable.

https://codereview.chromium.org/1156953002/diff/60001/Source/devtools/front_end/ui/SplitWidget.js
File Source/devtools/front_end/ui/SplitWidget.js (right):

https://codereview.chromium.org/1156953002/diff/60001/Source/devtools/front_end/ui/SplitWidget.js#newcode559
Source/devtools/front_end/ui/SplitWidget.js:559:
this._mainWidget.doResize();
This causes a lot of extra resizes, since |_cancelAnimation| is called
from multiple places. Same goes for the event.

https://codereview.chromium.org/1156953002/

sa...@chromium.org

unread,
Jun 2, 2015, 1:14:57 AM6/2/15
to dgo...@chromium.org, pfel...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, yurys...@chromium.org
PTAL

https://codereview.chromium.org/1156953002/

dgo...@chromium.org

unread,
Jun 2, 2015, 5:36:54 AM6/2/15
to sa...@chromium.org, pfel...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, yurys...@chromium.org
Could you please explain how is this better? Code does not look much
cleaner.
Did you measure performance or something?


https://codereview.chromium.org/1156953002/diff/80001/Source/devtools/front_end/ui/SplitWidget.js
File Source/devtools/front_end/ui/SplitWidget.js (left):

https://codereview.chromium.org/1156953002/diff/80001/Source/devtools/front_end/ui/SplitWidget.js#oldcode527
Source/devtools/front_end/ui/SplitWidget.js:527:
suppressUnused(this._mainElement.offsetWidth);
I wonder why this is not needed anymore?

https://codereview.chromium.org/1156953002/diff/80001/Source/devtools/front_end/ui/SplitWidget.js
File Source/devtools/front_end/ui/SplitWidget.js (right):

https://codereview.chromium.org/1156953002/diff/80001/Source/devtools/front_end/ui/SplitWidget.js#newcode517
Source/devtools/front_end/ui/SplitWidget.js:517:
this._mainWidget.doResize();
Looks like this call will be duplicated in animationFinished at the end.

https://codereview.chromium.org/1156953002/

paul...@chromium.org

unread,
Jun 2, 2015, 5:47:09 AM6/2/15
to sa...@chromium.org, dgo...@chromium.org, pfel...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, yurys...@chromium.org
Can we animate a transform so we don't have to layout on every frame?

https://codereview.chromium.org/1156953002/

dgo...@chromium.org

unread,
Jun 2, 2015, 5:50:23 AM6/2/15
to sa...@chromium.org, pfel...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, yurys...@chromium.org
On 2015/06/02 09:47:09, paulirish wrote:
> Can we animate a transform so we don't have to layout on every frame?

We have to layout because of things like CodeMirror or FlameChart redraw
themselves when size changes.

https://codereview.chromium.org/1156953002/

sa...@chromium.org

unread,
Jun 4, 2015, 11:06:53 PM6/4/15
to dgo...@chromium.org, pfel...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, sergey...@chromium.org, yurys...@chromium.org
Its hard to make a fair comparison about performance here. To me, it looks
much
better and is also more semantic.


https://codereview.chromium.org/1156953002/diff/80001/Source/devtools/front_end/ui/SplitWidget.js
File Source/devtools/front_end/ui/SplitWidget.js (left):

https://codereview.chromium.org/1156953002/diff/80001/Source/devtools/front_end/ui/SplitWidget.js#oldcode527
Source/devtools/front_end/ui/SplitWidget.js:527:
suppressUnused(this._mainElement.offsetWidth);
On 2015/06/02 at 09:36:54, dgozman wrote:
> I wonder why this is not needed anymore?

Not sure why it would be.

https://codereview.chromium.org/1156953002/diff/80001/Source/devtools/front_end/ui/SplitWidget.js#oldcode527
Source/devtools/front_end/ui/SplitWidget.js:527:
suppressUnused(this._mainElement.offsetWidth);
On 2015/06/02 at 09:36:54, dgozman wrote:
> I wonder why this is not needed anymore?

I don't know why it was needed in the first place.

https://codereview.chromium.org/1156953002/diff/80001/Source/devtools/front_end/ui/SplitWidget.js
File Source/devtools/front_end/ui/SplitWidget.js (right):

https://codereview.chromium.org/1156953002/diff/80001/Source/devtools/front_end/ui/SplitWidget.js#newcode517
Source/devtools/front_end/ui/SplitWidget.js:517:
this._mainWidget.doResize();
On 2015/06/02 at 09:36:54, dgozman wrote:
> Looks like this call will be duplicated in animationFinished at the
end.

Fixed.

https://codereview.chromium.org/1156953002/diff/80001/Source/devtools/front_end/ui/SplitWidget.js#newcode517
Source/devtools/front_end/ui/SplitWidget.js:517:
this._mainWidget.doResize();
On 2015/06/02 at 09:36:54, dgozman wrote:
> Looks like this call will be duplicated in animationFinished at the
end.

Done.

https://codereview.chromium.org/1156953002/
Reply all
Reply to author
Forward
0 new messages