francoischalifour / medium-zoom

🔎🖼 A JavaScript library for zooming images like Medium
https://medium-zoom.francoischalifour.com
MIT License
3.58k stars 162 forks source link

Zoomed images cause overflow #75

Closed edwardhorsford closed 5 years ago

edwardhorsford commented 5 years ago

Bug description

If you zoom an image with 0 margin and scrollbars enabled, the resulting image is too wide by the width of a scrollbar and causes a horizontal scrollbar to appear.

This causes two issues:

Here's a gif from the demo site: medium-zoom-overflow-bug

How to reproduce

Visit demo site and zoom first image with scroll bars (vertical) already visible. When zoomed, a horizontal scrollbar will appear.

Expected behavior

The image should fill the visible area.

Environment

Observed in Chrome and Safari on Osx 10.13.4.

Other.

Possibly related, when a margin is set the position of the image doesn't take account of the scrollbar (if it's visible). For small margins, this causes the image to appear off-centre.

Example: screen shot 2018-09-22 at 22 24 02 This image has a 20px margin. Because of the scrollbar, it appears off-centre.

francoischalifour commented 5 years ago

Thank you for reporting this bug!

I think this issue appears because we rely on window.innerWidth and window.innerHeight when retrieving the viewport size. We should rather rely on document.documentElement.clientWidth and document.documentElement.clientHeight, which ignore the scroll bar (see corresponding lines).

Do you want to contribute and fix this issue?

Let me know if you need help with anything (you can start by reading the contributing guide).

weranda commented 5 years ago

I fixed it like this (with jQuery):

// 1.
var getScrollbarWidth = function() {

    var container = $('<div style="width:50px;height:50px;overflow:auto"><div></div></div>').appendTo('body');
    var child = container.children();

    // Check for Zepto
    if (typeof child.innerWidth != 'function') {
        return 0;
    }

    var width = child.innerWidth() - child.height(90).innerWidth();
    container.remove();
    return width;
};

// 2. 
function zoomImage(barSize){
    var zoom = mediumZoom('[data-action="zoom"]', {
        container: {
            right: barSize
        }
    });
}

// 3.
$(window).load(function() {
    var scrollbarSize = getScrollbarWidth();
    zoomImage(scrollbarSize);
});

I don't know, correct it is or not, but it works.

edwardhorsford commented 5 years ago

Thank you for reporting this bug!

I think this issue appears because we rely on window.innerWidth and window.innerHeight when retrieving the viewport size. We should rather rely on document.documentElement.clientWidth and document.documentElement.clientHeight, which ignore the scroll bar (see corresponding lines).

Do you want to contribute and fix this issue?

Let me know if you need help with anything (you can start by reading the contributing guide).

@francoischalifour Thanks for the suggested fix. I've made on a fork and it seems to work. However I think the requirements for the contribution guidelines are beyond my abilities. Happy to PR for the two lines of changes, but won't be able to add tests.

francoischalifour commented 5 years ago

Feel free to send the PR, I'll take care of the rest!