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.

Sign up for GitHub

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

Jump to bottom

Fix: Inconsistent natural image dimensions in IE #324

Merged
merged 5 commits into from
Aug 21, 2017
Merged

Fix: Inconsistent natural image dimensions in IE #324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4094262
Fix: Inconsistent natural image dimensions in IE
Aug 18, 2017
991f919
Update: use promise in ImageBaseViewer test
Aug 19, 2017
d26ab3f
Merge branch 'master' into master
Minh-Ng Aug 21, 2017
3a5e7ea
Merge remote-tracking branch 'upstream/master'
Aug 21, 2017
d9f7261
Update: make setOriginalImageSize non-static
Aug 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 37 additions & 0 deletions src/lib/viewers/image/ImageBaseViewer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ class ImageBaseViewer extends BaseViewer {
return;
}

this.setOriginalImageSize(this.imageEl);
this.loadUI();
this.zoom();

Expand Down Expand Up @@ -167,6 +168,42 @@ class ImageBaseViewer extends BaseViewer {
this.bindControlListeners();
}

/**
* Sets the original image width and height on the img element. Can be removed when
* naturalHeight and naturalWidth attributes work correctly in IE 11.
*
* @private
* @param {Image} imageEl - The image to set the original size attributes on
Copy link
Contributor

Choose a reason for hiding this comment

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

{HTMLElement}

* @return {Promise} A promise that is resolved if the original image dimensions were set.
*/
setOriginalImageSize(imageEl) {
const image = imageEl;
Copy link
Contributor

@tonyjin tonyjin Aug 21, 2017 •
edited

Choose a reason for hiding this comment

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

I think @JustinHoldstock mentioned at some point he prefers just disabling the eslint rule that warns against this when you mean to explicitly modify the passed in param. We also generally tend to keep the 'El' portion of a name to indicate an HTML Element.

Would look like:

/* eslint-disable no-param-reassign */
setOriginalImageSize(imageEl) {
    // implementation
}
/* eslint-enable no-param-reassign */

const promise = new Promise((resolve, reject) => {
// Do not bother loading a new image when the natural size attributes exist
if (imageEl.naturalWidth > 0 && imageEl.naturalHeight > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the preferred way of checking whether natural size attributes are available? What happens for 0x0 or 0 by Y / X by 0 images?

Copy link
Contributor Author

@Minh-Ng Minh-Ng Aug 22, 2017 •
edited

Choose a reason for hiding this comment

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

For 0x0, the original width and height will be set to 1x1. It is not ideal but the original width/height are used to measure scale. Therefore if they are set to 0 we will have division by 0 that results in scale = Infinity.

naturalWidth and naturalHeight are supposed to be supported in IE 9+ but it doesn't appear to hold true. The reason for this PR is due to naturalWidth and naturalHeight being 0x0 on a non-zero image.

image.originalWidth = imageEl.naturalWidth;
image.originalHeight = imageEl.naturalHeight;
resolve();
} else {
const originalImg = new Image();
image.originalWidth = 1;
image.originalHeight = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of adding properties directly to an HTML element, it's better to add a data attribute, see: https://developer.mozilla.org/en-US/docs/Learn/HTML/Howto/Use_data_attributes

// Set
imageEl.dataset.originalWidth = 1;
imageEl.dataset.originalHeight = 1;

// Get
const originalWidth = imageEl.dataset.originalWidth;
const originalHeight = imageEl.dataset.originalHeight;


originalImg.error = () => {
reject();
};
originalImg.onload = () => {
image.originalWidth = originalImg.width || 1;
image.originalHeight = originalImg.height || 1;
resolve();
};
originalImg.src = imageEl.src;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to do this? finishLoading() is triggered after the image element loads (see ImageViewer.js) and this should be before zoom() is called so the image should already be in its natural width/height.

This implementation makes sense for the generic use case, but is probably less efficient for the exact use case you're solving for here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had previously tested out just checking image.width and image.height in the onload function. It appears to have already been styled at that point, hence the need for this. I can go back through and see if there is a point in which the original loaded image can set its data attributes.

}
});

return promise;
}

//--------------------------------------------------------------------------
// Event Listeners
//--------------------------------------------------------------------------
Expand Down
8 changes: 4 additions & 4 deletions src/lib/viewers/image/ImageViewer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,10 +183,10 @@ class ImageViewer extends ImageBaseViewer {
// 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.naturalHeight : this.imageEl.naturalWidth;
const originalWidth = this.isRotated() ? this.imageEl.originalHeight : this.imageEl.originalWidth;
newWidth = Math.min(viewport.width, originalWidth);
} else {
const originalHeight = this.isRotated() ? this.imageEl.naturalWidth : this.imageEl.naturalHeight;
const originalHeight = this.isRotated() ? this.imageEl.originalWidth : this.imageEl.originalHeight;
newHeight = Math.min(viewport.height, originalHeight);
}
}
Expand Down Expand Up @@ -231,7 +231,7 @@ class ImageViewer extends ImageBaseViewer {
* @return {void}
*/
setScale(width, height) {
this.scale = width ? width / this.imageEl.naturalWidth : height / this.imageEl.naturalHeight;
this.scale = width ? width / this.imageEl.originalWidth : height / this.imageEl.originalHeight;
this.rotationAngle = this.currentRotationAngle % 3600 % 360;
this.emit('scale', {
scale: this.scale,
Expand Down Expand Up @@ -390,7 +390,7 @@ class ImageViewer extends ImageBaseViewer {
handleOrientationChange() {
this.adjustImageZoomPadding();

this.scale = this.imageEl.clientWidth / this.imageEl.naturalWidth;
this.scale = this.imageEl.clientWidth / this.imageEl.originalWidth;
this.rotationAngle = this.currentRotationAngle % 3600 % 360;
this.emit('scale', {
scale: this.scale,
Expand Down
38 changes: 38 additions & 0 deletions src/lib/viewers/image/__tests__/ImageBaseViewer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ const CSS_CLASS_PANNABLE = 'pannable';

let stubs = {};

const imageUrl = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAwAAAAMCAYAAABWdVznAAAKQWlDQ1BJQ0MgUHJvZmlsZQAASA2dlndUU9kWh8+9N73QEiIgJfQaegkg0jtIFQRRiUmAUAKGhCZ2RAVGFBEpVmRUwAFHhyJjRRQLg4Ji1wnyEFDGwVFEReXdjGsJ7601896a/cdZ39nnt9fZZ+9917oAUPyCBMJ0WAGANKFYFO7rwVwSE8vE9wIYEAEOWAHA4WZmBEf4RALU/L09mZmoSMaz9u4ugGS72yy/UCZz1v9/kSI3QyQGAApF1TY8fiYX5QKUU7PFGTL/BMr0lSkyhjEyFqEJoqwi48SvbPan5iu7yZiXJuShGlnOGbw0noy7UN6aJeGjjAShXJgl4GejfAdlvVRJmgDl9yjT0/icTAAwFJlfzOcmoWyJMkUUGe6J8gIACJTEObxyDov5OWieAHimZ+SKBIlJYqYR15hp5ejIZvrxs1P5YjErlMNN4Yh4TM/0tAyOMBeAr2+WRQElWW2ZaJHtrRzt7VnW5mj5v9nfHn5T/T3IevtV8Sbsz55BjJ5Z32zsrC+9FgD2JFqbHbO+lVUAtG0GQOXhrE/vIADyBQC03pzzHoZsXpLE4gwnC4vs7GxzAZ9rLivoN/ufgm/Kv4Y595nL7vtWO6YXP4EjSRUzZUXlpqemS0TMzAwOl89k/fcQ/+PAOWnNycMsnJ/AF/GF6FVR6JQJhIlou4U8gViQLmQKhH/V4X8YNicHGX6daxRodV8AfYU5ULhJB8hvPQBDIwMkbj96An3rWxAxCsi+vGitka9zjzJ6/uf6Hwtcim7hTEEiU+b2DI9kciWiLBmj34RswQISkAd0oAo0gS4wAixgDRyAM3AD3iAAhIBIEAOWAy5IAmlABLJBPtgACkEx2AF2g2pwANSBetAEToI2cAZcBFfADXALDIBHQAqGwUswAd6BaQiC8BAVokGqkBakD5lC1hAbWgh5Q0FQOBQDxUOJkBCSQPnQJqgYKoOqoUNQPfQjdBq6CF2D+qAH0CA0Bv0BfYQRmALTYQ3YALaA2bA7HAhHwsvgRHgVnAcXwNvhSrgWPg63whfhG/AALIVfwpMIQMgIA9FGWAgb8URCkFgkAREha5EipAKpRZqQDqQbuY1IkXHkAwaHoWGYGBbGGeOHWYzhYlZh1mJKMNWYY5hWTBfmNmYQM4H5gqVi1bGmWCesP3YJNhGbjS3EVmCPYFuwl7ED2GHsOxwOx8AZ4hxwfrgYXDJuNa4Etw/XjLuA68MN4SbxeLwq3hTvgg/Bc/BifCG+Cn8cfx7fjx/GvyeQCVoEa4IPIZYgJGwkVBAaCOcI/YQRwjRRgahPdCKGEHnEXGIpsY7YQbxJHCZOkxRJhiQXUiQpmbSBVElqIl0mPSa9IZPJOmRHchhZQF5PriSfIF8lD5I/UJQoJhRPShxFQtlOOUq5QHlAeUOlUg2obtRYqpi6nVpPvUR9Sn0vR5Mzl/OX48mtk6uRa5Xrl3slT5TXl3eXXy6fJ18hf0r+pvy4AlHBQMFTgaOwVqFG4bTCPYVJRZqilWKIYppiiWKD4jXFUSW8koGStxJPqUDpsNIlpSEaQtOledK4tE20Otpl2jAdRzek+9OT6cX0H+i99AllJWVb5SjlHOUa5bPKUgbCMGD4M1IZpYyTjLuMj/M05rnP48/bNq9pXv+8KZX5Km4qfJUilWaVAZWPqkxVb9UU1Z2qbapP1DBqJmphatlq+9Uuq43Pp893ns+dXzT/5PyH6rC6iXq4+mr1w+o96pMamhq+GhkaVRqXNMY1GZpumsma5ZrnNMe0aFoLtQRa5VrntV4wlZnuzFRmJbOLOaGtru2nLdE+pN2rPa1jqLNYZ6NOs84TXZIuWzdBt1y3U3dCT0svWC9fr1HvoT5Rn62fpL9Hv1t/ysDQINpgi0GbwaihiqG/YZ5ho+FjI6qRq9Eqo1qjO8Y4Y7ZxivE+41smsImdSZJJjclNU9jU3lRgus+0zwxr5mgmNKs1u8eisNxZWaxG1qA5wzzIfKN5m/krCz2LWIudFt0WXyztLFMt6ywfWSlZBVhttOqw+sPaxJprXWN9x4Zq42Ozzqbd5rWtqS3fdr/tfTuaXbDdFrtOu8/2DvYi+yb7MQc9h3iHvQ732HR2KLuEfdUR6+jhuM7xjOMHJ3snsdNJp9+dWc4pzg3OowsMF/AX1C0YctFx4bgccpEuZC6MX3hwodRV25XjWuv6zE3Xjed2xG3E3dg92f24+ysPSw+RR4vHlKeT5xrPC16Il69XkVevt5L3Yu9q76c+Oj6JPo0+E752vqt9L/hh/QL9dvrd89fw5/rX+08EOASsCegKpARGBFYHPgsyCRIFdQTDwQHBu4IfL9JfJFzUFgJC/EN2hTwJNQxdFfpzGC4sNKwm7Hm4VXh+eHcELWJFREPEu0iPyNLIR4uNFksWd0bJR8VF1UdNRXtFl0VLl1gsWbPkRoxajCCmPRYfGxV7JHZyqffS3UuH4+ziCuPuLjNclrPs2nK15anLz66QX8FZcSoeGx8d3xD/iRPCqeVMrvRfuXflBNeTu4f7kufGK+eN8V34ZfyRBJeEsoTRRJfEXYljSa5JFUnjAk9BteB1sl/ygeSplJCUoykzqdGpzWmEtPi000IlYYqwK10zPSe9L8M0ozBDuspp1e5VE6JA0ZFMKHNZZruYjv5M9UiMJJslg1kLs2qy3mdHZZ/KUcwR5vTkmuRuyx3J88n7fjVmNXd1Z752/ob8wTXuaw6thdauXNu5Tnddwbrh9b7rj20gbUjZ8MtGy41lG99uit7UUaBRsL5gaLPv5sZCuUJR4b0tzlsObMVsFWzt3WazrWrblyJe0fViy+KK4k8l3JLr31l9V/ndzPaE7b2l9qX7d+B2CHfc3em681iZYlle2dCu4F2t5czyovK3u1fsvlZhW3FgD2mPZI+0MqiyvUqvakfVp+qk6oEaj5rmvep7t+2d2sfb17/fbX/TAY0DxQc+HhQcvH/I91BrrUFtxWHc4azDz+ui6rq/Z39ff0TtSPGRz0eFR6XHwo911TvU1zeoN5Q2wo2SxrHjccdv/eD1Q3sTq+lQM6O5+AQ4ITnx4sf4H++eDDzZeYp9qukn/Z/2ttBailqh1tzWibakNml7THvf6YDTnR3OHS0/m/989Iz2mZqzymdLz5HOFZybOZ93fvJCxoXxi4kXhzpXdD66tOTSna6wrt7LgZevXvG5cqnbvfv8VZerZ645XTt9nX297Yb9jdYeu56WX+x+aem172296XCz/ZbjrY6+BX3n+l37L972un3ljv+dGwOLBvruLr57/17cPel93v3RB6kPXj/Mejj9aP1j7OOiJwpPKp6qP6391fjXZqm99Oyg12DPs4hnj4a4Qy//lfmvT8MFz6nPK0a0RupHrUfPjPmM3Xqx9MXwy4yX0+OFvyn+tveV0auffnf7vWdiycTwa9HrmT9K3qi+OfrW9m3nZOjk03dp76anit6rvj/2gf2h+2P0x5Hp7E/4T5WfjT93fAn88ngmbWbm3/eE8/syOll+AAAA4klEQVQoFWNkQABG7QlvU/7/Z0xmYGTQBgv/Z7jKyPh/7tUC4TlA/n+QGCOI0Ox/LcnIwLKEkZHBCcRHB///M+z7z/An5nqh6HMmoCRQHapiN1VWhlZXLrg+kEEgNSC1LCBnABkoJvOyMTKIcoMtR9EEUsuo1f/uBCMjozlcBg/j////J5ngHkRS6KrCylDvxIkkAmUCAwPkBwygIszE4KDEiiEOEmACBtZVrDLYBIFqmUDhjE0OmxhILSgogB5/vwdXHMA0guLiWqGgC8gPQPafGJAATBKdhkUcSC1yYBOVNAAVx0qxuz8xqgAAAABJRU5ErkJggg==';

const sandbox = sinon.sandbox.create();
let imageBase;
let containerEl;
Expand Down Expand Up @@ -218,6 +220,39 @@ describe('lib/viewers/image/ImageBaseViewer', () => {
});
});

describe('setOriginalImageSize()', () => {
it('should use the naturalHeight and naturalWidth when available', (done) => {
const imageEl = {
naturalWidth: 100,
naturalHeight: 100
};

const promise = imageBase.setOriginalImageSize(imageEl);
promise.should.be.fulfilled.then(() => {
expect(imageEl.originalWidth).to.equal(imageEl.naturalWidth);
expect(imageEl.originalHeight).to.equal(imageEl.naturalHeight);
done();
});
});

it('should work when naturalHeight and naturalWidth are undefined', (done) => {
const imageEl = {
naturalWidth: undefined,
naturalHeight: undefined,
src: imageUrl
};

const imageUrlWidth = 12;
const imageUrlHeight = 12;
const promise = imageBase.setOriginalImageSize(imageEl);
promise.should.be.fulfilled.then(() => {
expect(imageEl.originalWidth).to.equal(imageUrlWidth);
expect(imageEl.originalHeight).to.equal(imageUrlHeight);
done();
});
});
});

describe('bindControlListeners()', () => {
it('should add the correct controls', () => {
imageBase.controls = {
Expand Down Expand Up @@ -500,6 +535,7 @@ describe('lib/viewers/image/ImageBaseViewer', () => {
stubs.emit = sandbox.stub(imageBase, 'emit');
stubs.zoom = sandbox.stub(imageBase, 'zoom');
stubs.loadUI = sandbox.stub(imageBase, 'loadUI');
stubs.setOriginalImageSize = sandbox.stub(imageBase, 'setOriginalImageSize');
});

it('should do nothing if already destroyed', () => {
Expand All @@ -509,6 +545,7 @@ describe('lib/viewers/image/ImageBaseViewer', () => {
expect(imageBase.loaded).to.be.false;
expect(stubs.emit).to.not.have.been.called;
expect(stubs.zoom).to.not.have.been.called;
expect(stubs.setOriginalImageSize).to.not.have.been.called;
expect(stubs.loadUI).to.not.have.been.called;
});

Expand All @@ -518,6 +555,7 @@ describe('lib/viewers/image/ImageBaseViewer', () => {
expect(imageBase.loaded).to.be.true;
expect(stubs.emit).to.have.been.calledWith('load');
expect(stubs.emit).to.have.been.calledWith('load');
expect(stubs.setOriginalImageSize).to.have.been.called;
expect(stubs.zoom).to.have.been.called;
expect(stubs.loadUI).to.have.been.called;
});
Expand Down