Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
93633009:
Files.app: Fix the animation of progress items. (Closed)
|
Created:
7 years ago by hirono Modified:
7 years ago Reviewers:
mtomasz CC:
chromium-reviews, rginda+watch_chromium.org, mtomasz+watch_chromium.org, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFiles.app: Fix the animation of progress items.
Previously the animation of progress items is broken and sometimes we cannot
detect the end of animation. This causes to remain the completed progress item.
The root cause is the progress track's width becomes 100% before the animation
starts. The ProgressCenterPanel is passed the reference of ProgressItem, and
refers it in a callback function, where the item value can be updated.
This CL make the ProgressCenterPanel save the item value in a local variable and
refer the variable in a callback function.
BUG=329400
TEST=Create 5 file and 5 directory, delete them in debug build.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242076
Patch Set 1 : #
Total comments: 7
Patch Set 2 : Remove the TODO comments. #Patch Set 3 : Quick Fix. #Messages
Total messages: 11 (0 generated)
PTAL the CL? Thanks!
https://codereview.chromium.org/93633009/diff/20001/chrome/browser/resources/... File chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js (right): https://codereview.chromium.org/93633009/diff/20001/chrome/browser/resources/... chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js:70: * TODO(hirono): Stop to use CSS transition. It is hard to control a trigger CSS transitions are fast. I think we should use them for simpler code and performance. Well designed, are possible to control. https://codereview.chromium.org/93633009/diff/20001/chrome/browser/resources/... chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js:102: track.style.width = targetWidthRate + '%'; Can you please explain the race? What's the problem here? webkitAnimationEnd or webkitTransitionEnd isn't invoked? When does it happen? If track.style.width == item.progressRateInPercent?
Thanks! https://codereview.chromium.org/93633009/diff/20001/chrome/browser/resources/... File chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js (right): https://codereview.chromium.org/93633009/diff/20001/chrome/browser/resources/... chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js:70: * TODO(hirono): Stop to use CSS transition. It is hard to control a trigger On 2013/12/19 07:52:00, mtomasz wrote: > CSS transitions are fast. I think we should use them for simpler code and > performance. Well designed, are possible to control. My concerns are the following points: 1. Assigning 10% and then assigning 20% synchronously does not resulted in the transition 10% -> 20%. We need 1 asynchronous steps to trigger it. Ideally we'd like to assign the value asynchronously only if the value is assigned synchronously before. But there is no way to detect it. 2. If the current property value is assigned to the property, this does not trigger the transition end event. e.g. During the transition from 10% to 20%, if we assign 15% at the time when the animated property just 15%, this prevents the transition end event. This is hard to reproduce, but can be. https://codereview.chromium.org/93633009/diff/20001/chrome/browser/resources/... chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js:102: track.style.width = targetWidthRate + '%'; On 2013/12/19 07:52:00, mtomasz wrote: > Can you please explain the race? What's the problem here? webkitAnimationEnd or > webkitTransitionEnd isn't invoked? When does it happen? If track.style.width == > item.progressRateInPercent? Yes, the race is like this: 1. Request setTimeout with animation = false. In this point, item.progressRateInPercent != 100. 2. item.progressRateInPercent reaches 100. The element is marked 'pre-completed' waiting webkitTransitionEnd. setTimeout is requested with animation = true. 3. The first request is solved. In the previous code, the track width is obtained directly from the item. Hence, 100% width is set to the track with animation = false. 4. The second request is solved. But webkitTransitionEnd is not invoked because the track width does not change.
https://codereview.chromium.org/93633009/diff/20001/chrome/browser/resources/... File chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js (right): https://codereview.chromium.org/93633009/diff/20001/chrome/browser/resources/... chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js:70: * TODO(hirono): Stop to use CSS transition. It is hard to control a trigger On 2013/12/19 08:17:05, hirono wrote: > On 2013/12/19 07:52:00, mtomasz wrote: > > CSS transitions are fast. I think we should use them for simpler code and > > performance. Well designed, are possible to control. > > My concerns are the following points: > > 1. Assigning 10% and then assigning 20% synchronously does not resulted in the > transition 10% -> 20%. We need 1 asynchronous steps to trigger it. Ideally we'd > like to assign the value asynchronously only if the value is assigned > synchronously before. But there is no way to detect it. > > > 2. If the current property value is assigned to the property, this does not > trigger the transition end event. e.g. During the transition from 10% to 20%, if > we assign 15% at the time when the animated property just 15%, this prevents the > transition end event. This is hard to reproduce, but can be. (1) I think this is an expected behavior. The animation is started in the next iteration of the event loop. Therefore to enforce the animation, setTimer(0) can be used. (2) I don't think it works this way. As long as the style.width != previous style.width, then the animation will start for sure in the next event loop iteration. Even if the computed value is somewhere between. https://codereview.chromium.org/93633009/diff/20001/chrome/browser/resources/... chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js:84: var previousWidthRate = This previousWidthRate may be out of date. Eg. 1. In the beginning it is NaN. 2. Schedule setting width to 10% without animation. 3. It is still Nan, since the timer is not launched yet. 4. Schedule setting width to 80% without animation. 5. First timer is fired and sets 10% without animation. 6. Second timer is fired and sets 80% without animation. (but it should animate!) Shall we just wrap the entire code from #82 to #104 in a setTimeout(..., 0)? WDYT? https://codereview.chromium.org/93633009/diff/20001/chrome/browser/resources/... chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js:102: track.style.width = targetWidthRate + '%'; On 2013/12/19 08:17:05, hirono wrote: > On 2013/12/19 07:52:00, mtomasz wrote: > > Can you please explain the race? What's the problem here? webkitAnimationEnd > or > > webkitTransitionEnd isn't invoked? When does it happen? If track.style.width > == > > item.progressRateInPercent? > > Yes, the race is like this: > > 1. Request setTimeout with animation = false. In this point, > item.progressRateInPercent != 100. > 2. item.progressRateInPercent reaches 100. The element is marked 'pre-completed' > waiting webkitTransitionEnd. setTimeout is requested with animation = true. > 3. The first request is solved. In the previous code, the track width is > obtained directly from the item. Hence, 100% width is set to the track with > animation = false. > 4. The second request is solved. But webkitTransitionEnd is not invoked because > the track width does not change. Thanks!
> (2) I don't think it works this way. As long as the style.width != previous > style.width, then the animation will start for sure in the next event loop > iteration. Even if the computed value is somewhere between. I re-checked the behavior with steps timing-function. Exactly you are right. I have missed it about the behavior. > https://codereview.chromium.org/93633009/diff/20001/chrome/browser/resources/... > chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js:84: > var previousWidthRate = > This previousWidthRate may be out of date. Eg. > > 1. In the beginning it is NaN. > 2. Schedule setting width to 10% without animation. > 3. It is still Nan, since the timer is not launched yet. > 4. Schedule setting width to 80% without animation. > 5. First timer is fired and sets 10% without animation. > 6. Second timer is fired and sets 80% without animation. (but it should > animate!) I have noticed it. Basically I'm planning to introduce another class to manage the state of each progress item UI after that. But this CL is needed to merge, so I'd like to keep the CL tiny. Can we go with either way? 1) Keep the CL as it is. Dropping animation is not so critical issue. or 2) Add a quick fix for the problem.
On 2013/12/20 03:23:00, hirono wrote: > > (2) I don't think it works this way. As long as the style.width != previous > > style.width, then the animation will start for sure in the next event loop > > iteration. Even if the computed value is somewhere between. > > I re-checked the behavior with steps timing-function. Exactly you are right. > I have missed it about the behavior. > > > > https://codereview.chromium.org/93633009/diff/20001/chrome/browser/resources/... > > > chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js:84: > > var previousWidthRate = > > This previousWidthRate may be out of date. Eg. > > > > 1. In the beginning it is NaN. > > 2. Schedule setting width to 10% without animation. > > 3. It is still Nan, since the timer is not launched yet. > > 4. Schedule setting width to 80% without animation. > > 5. First timer is fired and sets 10% without animation. > > 6. Second timer is fired and sets 80% without animation. (but it should > > animate!) > > I have noticed it. > Basically I'm planning to introduce another class to manage the state of each > progress item UI after that. > But this CL is needed to merge, so I'd like to keep the CL tiny. > Can we go with either way? > 1) Keep the CL as it is. Dropping animation is not so critical issue. > or > 2) Add a quick fix for the problem. I'm fine with keeping this CL as small as possible, but wouldn't be moving this code (#82 to #104) to inside of that setTimeout() solving of the issues at once? The patch wouldn't be bigger.
On 2013/12/20 03:27:59, mtomasz wrote: > On 2013/12/20 03:23:00, hirono wrote: > > > (2) I don't think it works this way. As long as the style.width != previous > > > style.width, then the animation will start for sure in the next event loop > > > iteration. Even if the computed value is somewhere between. > > > > I re-checked the behavior with steps timing-function. Exactly you are right. > > I have missed it about the behavior. > > > > > > > > https://codereview.chromium.org/93633009/diff/20001/chrome/browser/resources/... > > > > > > chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js:84: > > > var previousWidthRate = > > > This previousWidthRate may be out of date. Eg. > > > > > > 1. In the beginning it is NaN. > > > 2. Schedule setting width to 10% without animation. > > > 3. It is still Nan, since the timer is not launched yet. > > > 4. Schedule setting width to 80% without animation. > > > 5. First timer is fired and sets 10% without animation. > > > 6. Second timer is fired and sets 80% without animation. (but it should > > > animate!) > > > > I have noticed it. > > Basically I'm planning to introduce another class to manage the state of each > > progress item UI after that. > > But this CL is needed to merge, so I'd like to keep the CL tiny. > > Can we go with either way? > > 1) Keep the CL as it is. Dropping animation is not so critical issue. > > or > > 2) Add a quick fix for the problem. > > I'm fine with keeping this CL as small as possible, but wouldn't be moving this > code (#82 to #104) to inside of that setTimeout() solving of the issues at once? > The patch wouldn't be bigger. Currently we assume the element gets 'pre-complete' attribute immediately after updateItemElement_ function is called. So we cannot just wrap the block with setTimeout until we fix the code around the assumption.
On 2013/12/20 03:34:02, hirono wrote: > On 2013/12/20 03:27:59, mtomasz wrote: > > On 2013/12/20 03:23:00, hirono wrote: > > > > (2) I don't think it works this way. As long as the style.width != > previous > > > > style.width, then the animation will start for sure in the next event loop > > > > iteration. Even if the computed value is somewhere between. > > > > > > I re-checked the behavior with steps timing-function. Exactly you are right. > > > I have missed it about the behavior. > > > > > > > > > > > > > https://codereview.chromium.org/93633009/diff/20001/chrome/browser/resources/... > > > > > > > > > > chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js:84: > > > > var previousWidthRate = > > > > This previousWidthRate may be out of date. Eg. > > > > > > > > 1. In the beginning it is NaN. > > > > 2. Schedule setting width to 10% without animation. > > > > 3. It is still Nan, since the timer is not launched yet. > > > > 4. Schedule setting width to 80% without animation. > > > > 5. First timer is fired and sets 10% without animation. > > > > 6. Second timer is fired and sets 80% without animation. (but it should > > > > animate!) > > > > > > I have noticed it. > > > Basically I'm planning to introduce another class to manage the state of > each > > > progress item UI after that. > > > But this CL is needed to merge, so I'd like to keep the CL tiny. > > > Can we go with either way? > > > 1) Keep the CL as it is. Dropping animation is not so critical issue. > > > or > > > 2) Add a quick fix for the problem. > > > > I'm fine with keeping this CL as small as possible, but wouldn't be moving > this > > code (#82 to #104) to inside of that setTimeout() solving of the issues at > once? > > The patch wouldn't be bigger. > > Currently we assume the element gets 'pre-complete' attribute immediately after > updateItemElement_ function is called. > So we cannot just wrap the block with setTimeout until we fix the code around > the assumption. I see. LGTM for the quick fix now if we need to merge, and let's do a complete fix later.
On 2013/12/20 03:36:01, mtomasz wrote: > On 2013/12/20 03:34:02, hirono wrote: > > On 2013/12/20 03:27:59, mtomasz wrote: > > > On 2013/12/20 03:23:00, hirono wrote: > > > > > (2) I don't think it works this way. As long as the style.width != > > previous > > > > > style.width, then the animation will start for sure in the next event > loop > > > > > iteration. Even if the computed value is somewhere between. > > > > > > > > I re-checked the behavior with steps timing-function. Exactly you are > right. > > > > I have missed it about the behavior. > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/93633009/diff/20001/chrome/browser/resources/... > > > > > > > > > > > > > > > chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js:84: > > > > > var previousWidthRate = > > > > > This previousWidthRate may be out of date. Eg. > > > > > > > > > > 1. In the beginning it is NaN. > > > > > 2. Schedule setting width to 10% without animation. > > > > > 3. It is still Nan, since the timer is not launched yet. > > > > > 4. Schedule setting width to 80% without animation. > > > > > 5. First timer is fired and sets 10% without animation. > > > > > 6. Second timer is fired and sets 80% without animation. (but it should > > > > > animate!) > > > > > > > > I have noticed it. > > > > Basically I'm planning to introduce another class to manage the state of > > each > > > > progress item UI after that. > > > > But this CL is needed to merge, so I'd like to keep the CL tiny. > > > > Can we go with either way? > > > > 1) Keep the CL as it is. Dropping animation is not so critical issue. > > > > or > > > > 2) Add a quick fix for the problem. > > > > > > I'm fine with keeping this CL as small as possible, but wouldn't be moving > > this > > > code (#82 to #104) to inside of that setTimeout() solving of the issues at > > once? > > > The patch wouldn't be bigger. > > > > Currently we assume the element gets 'pre-complete' attribute immediately > after > > updateItemElement_ function is called. > > So we cannot just wrap the block with setTimeout until we fix the code around > > the assumption. > > I see. LGTM for the quick fix now if we need to merge, and let's do a complete > fix later. Thanks! I removed the TODO comment of removing the CSS animation. In the new patch, we will keep the CSS animation.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hirono@chromium.org/93633009/80001
Message was sent while issue was closed.
Change committed as 242076 |
