Fix: Image viewer flickering in IE11 during reset zoom #594
Conversation
|
Verified that @DanDeMicco has signed the CLA. Thanks for the pull request! |
src/lib/viewers/image/ImageViewer.js
Outdated
| @@ -126,12 +126,26 @@ class ImageViewer extends ImageBaseViewer { | |||
| zoom(type) { | |||
| let newWidth; | |||
| let newHeight; | |||
| const imageCurrentDimensions = this.imageEl.getBoundingClientRect(); // Getting bounding rect does not ignore transforms / rotates | |||
| let imageCurrentDimensions; | |||
| if (type === 'reset') { | |||
There was a problem hiding this comment.
Can we keep this in the switch statement? You can set imageCurrentDimensions here and then update it in the switch statement if we are resetting. It feels a little weird to check for the 'reset' case and then switch on type right after.
src/lib/viewers/image/ImageViewer.js
Outdated
| * @param {number} [height] - The height in px | ||
| * @return {boolean} true if the width will be modified instead of height | ||
| */ | ||
| getModifyWidthInsteadOfHeight(width, height) { |
There was a problem hiding this comment.
Why not go with something like isLandscape(), since we're essentially checking whether or not the width is greater than the height? Also, we could reduce this to return width >= height. Were there any other reasons behind using aspect ratio?
Note: We may already have an isLandscape() method hiding somewhere
There was a problem hiding this comment.
Good point, the CR was just moving around code but I don't see the aspect ratio being used anywhere.
As for the isLandscape method hiding somewhere, I was not able to find one, so just incorporated your comments and renamed the method
src/lib/viewers/image/ImageViewer.js
Outdated
| if (modifyWidthInsteadOfHeight) { | ||
| newWidth = type === 'in' ? width * IMAGE_ZOOM_SCALE : width / IMAGE_ZOOM_SCALE; | ||
| } else { | ||
| newHeight = type === 'in' ? height * IMAGE_ZOOM_SCALE : (newHeight = height / IMAGE_ZOOM_SCALE); |
There was a problem hiding this comment.
I think you can remove the assignment as the last component of the ternary.
newHeight = type === 'in' ? height * IMAGE_ZOOM_SCALE : height / IMAGE_ZOOM_SCALE;
There was a problem hiding this comment.
Good catch, that was what I meant to put there to match the newWidth expression
src/lib/viewers/image/ImageViewer.js
Outdated
| @@ -116,6 +116,21 @@ class ImageViewer extends ImageBaseViewer { | |||
| this.setScale(this.imageEl.offsetwidth, this.imageEl.offsetHeight); | |||
| } | |||
|
|
|||
| /** | |||
| * Getter for modifyWidthInsteadOfHeight which will be used in zoom | |||
There was a problem hiding this comment.
Update docs to not refer to modifyWidthInsteadOfHeight
src/lib/viewers/image/ImageViewer.js
Outdated
| * @private | ||
| * @param {number} [width] - The width in px | ||
| * @param {number} [height] - The height in px | ||
| * @return {boolean} true if the width will be modified instead of height |
There was a problem hiding this comment.
true if image is landscape layout, or something like that
src/lib/viewers/image/ImageViewer.js
Outdated
| * @return {boolean} true if the width will be modified instead of height | ||
| */ | ||
| isLandscape(width, height) { | ||
| // For multi page tifs, we always modify the width, since its essentially a DIV and not IMG tag. |
There was a problem hiding this comment.
I'd say we can remove these two comment lines
| @@ -521,4 +543,16 @@ describe('lib/viewers/image/ImageViewer', () => { | |||
| }); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('getModifyWidthInsteadOfHeight()', () => { | |||
There was a problem hiding this comment.
update test name to isLandscape()
DanDeMicco
force-pushed
the
master
branch
2 times, most recently
from
81ac232 to
bed3312
Compare
The old 'reset' case set the image width & height, then recursively called zoom, which caused the image to be resized twice and thus flickered in IE11.
Also, has a minor additional bugfix in which the image was rendered at the incorrect size by default, thus when resetting on a freshly rendered image it would slightly resize. This was due to use of clientHeight instead of offsetHeight which does not include the scrollbar (which may be on initial load)