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

Zoomed images cause overflow #75

Closed
edwardhorsford opened this issue Sep 22, 2018 · 4 comments
Closed

Zoomed images cause overflow #75

edwardhorsford opened this issue Sep 22, 2018 · 4 comments

Comments

@edwardhorsford
Copy link
Contributor

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:

  • Unnecessary scrollbar
  • Part of the image isn't shown

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
Copy link
Owner

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
Copy link

weranda commented Oct 12, 2018

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 added a commit to edwardhorsford/medium-zoom that referenced this issue Oct 14, 2018
@edwardhorsford
Copy link
Contributor Author

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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants