Skip to content

Commit

Permalink
Files app: Remove thumbnail animation when updating its image
Browse files Browse the repository at this point in the history
This CL removes the animation when updating the thumbnail image. This
animation is very quick and hardly noticed and some code paths weren't
even using the animation.

This fixes 2 issues:
1. Old images weren't removed when there animation wasn't used.
2. The event listener for 'animationend' wasn't removed.

No change in functionality for the user, so no new tests added.

Test: Manually checked that file list and grid display and update their thumbnails.
Bug: 1056443
Change-Id: I8a6bd050c2bbb358387934ff9371c075a6703508
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2076749
Commit-Queue: Noel Gordon <noel@chromium.org>
Auto-Submit: Luciano Pacheco <lucmult@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#744972}
  • Loading branch information
Luciano Pacheco authored and Commit Bot committed Feb 27, 2020
1 parent cd9c40f commit 0b3cd1d
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 61 deletions.
17 changes: 0 additions & 17 deletions ui/file_manager/file_manager/foreground/css/file_manager.css
Original file line number Diff line number Diff line change
Expand Up @@ -1758,10 +1758,6 @@ body.check-select .thumbnail-grid .thumbnail-item[selected] .checkmark.active {
width: 100%;
}

.thumbnail-grid .img-container > .thumbnail.animate {
animation: fadeIn 250ms linear;
}

.thumbnail-grid .shield {
background-color: rgba(160, 160, 160, 50%);
bottom: 0;
Expand Down Expand Up @@ -2002,15 +1998,6 @@ li[renaming=''] .badge {
justify-content: flex-end;
}

@keyframes fadeIn {
from {
opacity: 0;
}
to {
opacity: 1;
}
}

@keyframes fadeOut {
from {
opacity: 1;
Expand Down Expand Up @@ -2169,10 +2156,6 @@ body.check-select #list-container li[selected] .detail-checkmark {
width: 100%;
}

#list-container list li .detail-thumbnail > .thumbnail.animate {
animation: fadeIn 220ms ease;
}

body.check-select #list-container list li[selected] .detail-thumbnail
> .thumbnail {
/* Fade out after checkmark fades in. */
Expand Down
30 changes: 8 additions & 22 deletions ui/file_manager/file_manager/foreground/js/ui/file_grid.js
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ class FileGrid extends cr.ui.Grid {
FileGrid.setThumbnailImage_(
assertInstanceof(box, HTMLDivElement), entry,
assert(event.dataUrl), assert(event.width), assert(event.height),
/* should animate */ true, mimeType);
mimeType);
}
}
listItem.classList.toggle('thumbnail-loaded', !!event.dataUrl);
Expand Down Expand Up @@ -691,8 +691,7 @@ class FileGrid extends cr.ui.Grid {
.contentMimeType;
FileGrid.setThumbnailImage_(
box, entry, thumbnailData.dataUrl, (thumbnailData.width || 0),
(thumbnailData.height || 0),
/* should not animate */ false, mimeType);
(thumbnailData.height || 0), mimeType);
li.classList.toggle('thumbnail-loaded', true);
} else {
FileGrid.setGenericThumbnail_(box, entry);
Expand Down Expand Up @@ -761,15 +760,10 @@ class FileGrid extends cr.ui.Grid {
* @param {string} dataUrl Data url of thumbnail.
* @param {number} width Width of thumbnail.
* @param {number} height Height of thumbnail.
* @param {boolean} shouldAnimate Whether the thumbnail is shown with
* animation or not.
* @param {string=} opt_mimeType Optional mime type for the image.
* @private
*/
static setThumbnailImage_(
box, entry, dataUrl, width, height, shouldAnimate, opt_mimeType) {
const oldThumbnails = box.querySelectorAll('.thumbnail');

static setThumbnailImage_(box, entry, dataUrl, width, height, opt_mimeType) {
const thumbnail = box.ownerDocument.createElement('div');
thumbnail.classList.add('thumbnail');

Expand All @@ -782,20 +776,12 @@ class FileGrid extends cr.ui.Grid {
}

thumbnail.style.backgroundImage = 'url(' + dataUrl + ')';
thumbnail.addEventListener('animationend', () => {
// Remove animation css once animation is completed in order not to
// animate again when an item is attached to the dom again.
thumbnail.classList.remove('animate');

for (let i = 0; i < oldThumbnails.length; i++) {
if (box.contains(oldThumbnails[i])) {
box.removeChild(oldThumbnails[i]);
}
}
});
if (shouldAnimate) {
thumbnail.classList.add('animate');

const oldThumbnails = box.querySelectorAll('.thumbnail');
for (let i = 0; i < oldThumbnails.length; i++) {
box.removeChild(oldThumbnails[i]);
}

box.appendChild(thumbnail);
}

Expand Down
30 changes: 8 additions & 22 deletions ui/file_manager/file_manager/foreground/js/ui/file_table.js
Original file line number Diff line number Diff line change
Expand Up @@ -647,8 +647,7 @@ class FileTable extends cr.ui.Table {
if (box) {
if (event.dataUrl) {
this.setThumbnailImage_(
assertInstanceof(box, HTMLDivElement), event.dataUrl,
true /* with animation */);
assertInstanceof(box, HTMLDivElement), event.dataUrl);
} else {
this.clearThumbnailImage_(assertInstanceof(box, HTMLDivElement));
}
Expand Down Expand Up @@ -1129,8 +1128,7 @@ class FileTable extends cr.ui.Table {
this.listThumbnailLoader_.getThumbnailFromCache(entry) :
null;
if (thumbnailData && thumbnailData.dataUrl) {
this.setThumbnailImage_(
box, thumbnailData.dataUrl, false /* without animation */);
this.setThumbnailImage_(box, thumbnailData.dataUrl);
}

return box;
Expand All @@ -1140,30 +1138,18 @@ class FileTable extends cr.ui.Table {
* Sets thumbnail image to the box.
* @param {!HTMLDivElement} box Detail thumbnail div element.
* @param {string} dataUrl Data url of thumbnail.
* @param {boolean} shouldAnimate Whether the thumbnail is shown with
* animation or not.
* @private
*/
setThumbnailImage_(box, dataUrl, shouldAnimate) {
const oldThumbnails = box.querySelectorAll('.thumbnail');

setThumbnailImage_(box, dataUrl) {
const thumbnail = box.ownerDocument.createElement('div');
thumbnail.classList.add('thumbnail');
thumbnail.style.backgroundImage = 'url(' + dataUrl + ')';
thumbnail.addEventListener('animationend', () => {
// Remove animation css once animation is completed in order not to
// animate again when an item is attached to the dom again.
thumbnail.classList.remove('animate');

for (let i = 0; i < oldThumbnails.length; i++) {
if (box.contains(oldThumbnails[i])) {
box.removeChild(oldThumbnails[i]);
}
}
});
const oldThumbnails = box.querySelectorAll('.thumbnail');

if (shouldAnimate) {
thumbnail.classList.add('animate');
for (let i = 0; i < oldThumbnails.length; i++) {
if (box.contains(oldThumbnails[i])) {
box.removeChild(oldThumbnails[i]);
}
}

box.appendChild(thumbnail);
Expand Down

0 comments on commit 0b3cd1d

Please sign in to comment.