Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Image viewer flickering in IE11 during reset zoom #594

Merged
merged 16 commits into from
Jan 26, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 58 additions & 65 deletions src/lib/viewers/image/ImageViewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update docs to not refer to modifyWidthInsteadOfHeight

* and will determine if the width or the height needs to be modified
*
* @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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true if image is landscape layout, or something like that

*/
isLandscape(width, height) {
// For multi page tifs, we always modify the width, since its essentially a DIV and not IMG tag.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say we can remove these two comment lines

// For images that are wider than taller we use width. For images that are taller than wider, we use height.
return width >= height;
}

/**
* Handles zoom
*
Expand All @@ -126,79 +141,57 @@ class ImageViewer extends ImageBaseViewer {
zoom(type) {
let newWidth;
let newHeight;
const imageCurrentDimensions = this.imageEl.getBoundingClientRect(); // Getting bounding rect does not ignore transforms / rotates
const { height, width } = imageCurrentDimensions;
const aspect = width / height;
const viewport = {
width: this.wrapperEl.clientWidth - IMAGE_PADDING,
height: this.wrapperEl.clientHeight - IMAGE_PADDING
};

// For multi page tifs, we always modify the width, since its essentially a DIV and not IMG tag.
// For images that are wider than taller we use width. For images that are taller than wider, we use height.
const modifyWidthInsteadOfHeight = aspect >= 1;
let height;
let width;

// From this point on, only 1 dimension will be modified. Either it will be width or it will be height.
// The other one will remain null and eventually get cleared out. The image should automatically use the proper value
// for the dimension that was cleared out.
switch (type) {
case 'in':
if (type === 'in' || type === 'out') {
const imageCurrentDimensions = this.imageEl.getBoundingClientRect(); // Getting bounding rect does not ignore transforms / rotates
({ height, width } = imageCurrentDimensions);

const modifyWidthInsteadOfHeight = this.isLandscape(width, height);
if (modifyWidthInsteadOfHeight) {
newWidth = type === 'in' ? width * IMAGE_ZOOM_SCALE : width / IMAGE_ZOOM_SCALE;
} else {
newHeight = type === 'in' ? height * IMAGE_ZOOM_SCALE : height / IMAGE_ZOOM_SCALE;
}
} else {
// This can be triggered by initial render as well as reset
// When it is an initial render or reset, take the original dimensions of the image
// dont use getBoundingClientRect because it may be a reset, and it causes a reflow
height = parseInt(this.imageEl.getAttribute('originalHeight'), 10);
width = parseInt(this.imageEl.getAttribute('originalWidth'), 10);
const modifyWidthInsteadOfHeight = this.isLandscape(width, height);

const viewport = {
width: this.wrapperEl.offsetWidth - 2 * IMAGE_PADDING,
height: this.wrapperEl.offsetHeight - 2 * IMAGE_PADDING
};
// If the image is overflowing the viewport, figure out by how much
// Then take that aspect that reduces the image the maximum (hence min ratio) to fit both width and height
if (width > viewport.width || height > viewport.height) {
const ratio = Math.min(viewport.width / width, viewport.height / height);
if (modifyWidthInsteadOfHeight) {
newWidth = width * IMAGE_ZOOM_SCALE;
newWidth = width * ratio;
} else {
newHeight = height * IMAGE_ZOOM_SCALE;
newHeight = height * ratio;
}
break;

case 'out':
if (modifyWidthInsteadOfHeight) {
newWidth = width / IMAGE_ZOOM_SCALE;
} else {
newHeight = height / IMAGE_ZOOM_SCALE;
}
break;

case 'reset':
// Reset the dimensions to their original values by removing overrides
// Doing so will make the browser render the image in its natural size
// Then we can proceed by recalculating stuff from that natural size.
this.imageEl.style.width = '';
this.imageEl.style.height = '';

this.adjustImageZoomPadding();

// Image may still overflow the page, so do the default zoom by calling zoom again
// This will go through the same workflow but end up in another case block.
this.zoom();

// Kill further execution
return;
/* istanbul ignore next */
default:
// If the image is overflowing the viewport, figure out by how much
// Then take that aspect that reduces the image the maximum (hence min ratio) to fit both width and height
if (width > viewport.width || height > viewport.height) {
const ratio = Math.min(viewport.width / width, viewport.height / height);

if (modifyWidthInsteadOfHeight) {
newWidth = width * ratio;
} else {
newHeight = height * ratio;
}

// If the image is smaller than the new viewport, zoom up to a
// max of the original file size
} else if (modifyWidthInsteadOfHeight) {
const originalWidth = this.isRotated()
? this.imageEl.getAttribute('originalHeight')
: this.imageEl.getAttribute('originalWidth');
newWidth = Math.min(viewport.width, originalWidth);
} else {
const originalHeight = this.isRotated()
? this.imageEl.getAttribute('originalWidth')
: this.imageEl.getAttribute('originalHeight');
newHeight = Math.min(viewport.height, originalHeight);
}
// If the image is smaller than the new viewport, zoom up to a
// max of the original file size
} else if (modifyWidthInsteadOfHeight) {
const originalWidth = this.isRotated()
? this.imageEl.getAttribute('originalHeight')
: this.imageEl.getAttribute('originalWidth');
newWidth = Math.min(viewport.width, originalWidth);
} else {
const originalHeight = this.isRotated()
? this.imageEl.getAttribute('originalWidth')
: this.imageEl.getAttribute('originalHeight');
newHeight = Math.min(viewport.height, originalHeight);
}
}

// If the image has been rotated, we need to swap the width and height
Expand Down
42 changes: 38 additions & 4 deletions src/lib/viewers/image/__tests__/ImageViewer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -287,16 +287,38 @@ describe('lib/viewers/image/ImageViewer', () => {
});

it('should reset dimensions and adjust padding when called with reset', () => {
image.imageEl.style.width = '10px';
image.imageEl.style.height = '20px';
image.imageEl.style.width = '1000px';
image.imageEl.style.height = '2000px';
const naturalHeight = 10;
const naturalWidth = 5;
image.imageEl.setAttribute('originalHeight', naturalHeight);
image.imageEl.setAttribute('originalWidth', naturalWidth);

sandbox.spy(image, 'zoom');

image.zoom('reset');

expect(image.imageEl.style.width).to.equal('');
expect(image.imageEl.style.height).to.equal(`${naturalHeight}px`);
expect(stubs.adjustZoom).to.be.called;
});

it('when rotated should reset dimensions and adjust padding when called with reset', () => {
image.currentRotationAngle = -90;
image.imageEl.style.width = '1000px';
image.imageEl.style.height = '2000px';
const naturalHeight = 5;
const naturalWidth = 10;
image.imageEl.setAttribute('originalHeight', naturalHeight);
image.imageEl.setAttribute('originalWidth', naturalWidth);

sandbox.spy(image, 'zoom');

image.zoom('reset');

expect(image.imageEl.style.width).to.equal('');
expect(image.imageEl.style.height).to.equal('');
expect(image.imageEl.style.height).to.equal(`${naturalHeight}px`);
expect(stubs.adjustZoom).to.be.called;
expect(image.zoom).to.be.calledWith();
});
});

Expand Down Expand Up @@ -521,4 +543,16 @@ describe('lib/viewers/image/ImageViewer', () => {
});
});
});

describe('getModifyWidthInsteadOfHeight()', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update test name to isLandscape()

it('should modify the width if the aspect ratio is >= 1', () => {
const getModifyWidthInsteadOfHeight = image.isLandscape(100, 50);
expect(getModifyWidthInsteadOfHeight).to.be.true;
});

it('should modify the height if the aspect ratio is < 1', () => {
const getModifyWidthInsteadOfHeight = image.isLandscape(100, 500);
expect(getModifyWidthInsteadOfHeight).to.be.false;
});
})
});