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 1 commit
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
28 changes: 28 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;
}

ImageBaseViewer.setOriginalImageSize(this.imageEl);
Copy link
Contributor

Choose a reason for hiding this comment

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

this.setOriginalImageSize(this.imageEl);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

setOriginalImageSize is implemented as a static method right now. Is there any preference for avoiding static methods? I know that only one Viewer will actually be instantiated so there will not be much of a difference in our use case but I am advocating for the use of static methods to avoid the creation of monolithic utility files.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have a policy one way or another, but I'd lean towards making this a normal private function in the base viewer until otherwise needed since that's how we've organized other similar helper functions. The moment this is required in a different viewer outside of the ImageBaseViewer inheritance chain, we can easily update this to a static method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to be non-static

this.loadUI();
this.zoom();

Expand Down Expand Up @@ -167,6 +168,33 @@ 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.
*
* @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 {void}
*/
static 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 */


// Do not bother loading a new image when the natural size attributes exist
if (imageEl.naturalWidth > 0 && imageEl.naturalHeight > 0) {
image.originalWidth = imageEl.naturalWidth;
image.originalHeight = imageEl.naturalHeight;
} else {
const originalImg = new Image();
image.originalWidth = 1;
image.originalHeight = 1;

originalImg.onload = () => {
image.originalWidth = originalImg.width || 1;
image.originalHeight = originalImg.height || 1;
};
originalImg.src = imageEl.src;
}
}

//--------------------------------------------------------------------------
// 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
36 changes: 36 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,37 @@ describe('lib/viewers/image/ImageBaseViewer', () => {
});
});

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

ImageBaseViewer.setOriginalImageSize(imageEl);
expect(imageEl.originalWidth).to.equal(imageEl.naturalWidth);
expect(imageEl.originalHeight).to.equal(imageEl.naturalHeight);
});

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

const imageUrlWidth = 12;
const imageUrlHeight = 12;
ImageBaseViewer.setOriginalImageSize(imageEl);
// Caution: event loop dependent
setTimeout(() => {
expect(imageEl.originalWidth).to.equal(imageUrlWidth);
expect(imageEl.originalHeight).to.equal(imageUrlHeight);
done();
}, 0);
});
});

describe('bindControlListeners()', () => {
it('should add the correct controls', () => {
imageBase.controls = {
Expand Down Expand Up @@ -500,6 +533,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(ImageBaseViewer, 'setOriginalImageSize');
});

it('should do nothing if already destroyed', () => {
Expand All @@ -509,6 +543,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 +553,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