another stab at confining images to page width - #8062

ping @jdfreder, @ellisonbg, @erccarls, @rsmith31415 for testing

Images are confined to width by default; double-click restores the 'native' size of the image. The unconfined metadata allows explicit requests for images to ignore confinement.

Are you sure it is working in Firefox? In Chrome, works fine using from IPython.display import Image.

Firefox:

Chrome:

I noticed another issue in Firefox. When I click on the div with class .out_prompt_overlay prompt (the sidebar near the output), I can't click to change the cell again and the image expands quite a bit to cover the .out_prompt_overlay prompt div.

@rsmith31415 thanks for testing, I'm never sure if anything works in Firefox. That looks wrong in all kinds of ways.

Okay, pushed a small additional max-width, and now it seems to work fine on both FF and Chrome (here, at least). I wasn't able to reproduce the weird overlay you saw, though.

Correct. The second issue has been fixed. Unfortunately, the image (1200x1600, by the way) still overflows in Firefox.

Interesting! I wasn't able to reproduce the second issue, but the first issue was fixed by the recent change for me. I'm at a loss if it still doesn't work on other machines.

You mean the opposite, right? You can't reproduce the first issue (the image overflowing the right side of the notebook) but you fixed the second issue (image overlapping div.out_prompt_overlay).

By the way, I'm using Firefox 36.0.1 on Ubuntu 14.10.

@rsmith31415 no. I can reproduce the image overflowing to the right, and the latest commit fixed it. I have not seen the image overlapping the prompt overlay.

I think there was a happy coincidence. After you fixed the image overflowing to the right, the overlapping was fixed. The problem is that the overflowing still remains in Firefox. However, there is no overflowing if I use markdown instead of from IPython.display import Image.

The image appears to be constrained to 100% width in Firefox with the latest commit, but it still overflows to the right, because it's offset by the width of the Out [n]: prompt:

Maybe we can use calc() to get the correct size?

Double click to switch between width constrained and 1:1 seems to work, and images in markdown cells are correctly constrained to the notebook width.

I noticed that there are different HTML elements (with different classes) being injected depending on the source (e.g. from IPython.display import HTML vs from IPython.display import Image). Wouldn't it be possible to use a single markup for all of them? I'm saying that because iframes are still overflowing in Chrome and Firefox:

In that case, you're explicitly setting width/height in a tag in the HTML. I'm not sure there's much we can/should do about that.

I applied the max-width behavior to img, svg in particular, but perhaps it should be applied to the outer element, or all children of output. It is not clear to me whether raw HTML output (e.g. iframes) be restricted, or left alone. Perhaps a combination of this and #7718 should be used.

For the moment, I'm inclined to only confine images. The reason being that we already have the double-click to expand behavior on images (I only changed slightly what expand means in this PR), but not on the output area as a whole. That means that right now, there's an easy way to expand larger images to their native size, but not an easy way to expand other HTML elements. Perhaps we should go that way ultimately, but I like to take these things one step at a time.

@takluyver thanks for pointing that out, yes calc should fix it in the latest commit. I thought it was behaving correctly because of my custom css changes the right margin. I should really remember to test CSS PRs without custom CSS.

In that case, you're explicitly setting width/height in a tag in the HTML

max-width takes effect when width > max-width. I have it working on every output I produce if I use * instead of the img, svg in this PR. So we definitely can do something about it. "Should" is less clear.

@takluyver I don't think it should be different to set widths and heights in an iframe than importing an image of arbitrary size. The container should constraint the output's dimensions under any circumstance. Actually, if you change the iframe to have width:100%; that fixes it. However, I think it is better to clean that part of the markup to make it more consistent and allow for a more definitive solution. For example, even relatively simple HTML will overflow (in the example below, a table):

@minrk I think the double click (and single click) behavior is working correctly in other HTML elements (actually, in the case of a table, the double click works but single click has no effect.) The main problem are not the parent elements. The problem is that the HTML tags that are injected need to have proper styling to fit their parents.

@rsmith31415 I assume you are referring to the scrolling behavior triggered by clicking on the prompt area, because I was referring to the double-click event we only bind to images.

If we combine this with #7718, then all output will be confined to the area. Arbitrary HTML will overflow in with a scrollbar, and images will shrink to fit. It would also mean that expanding images will not expand them onto the entire page, but rather into a scrolling view that still fits within the width of the notebook. It would make it impossible for images to extend beyond the bounds of the notebook area.

@minrk You're right. I was referring to the prompt because in the last commit, I'm seeing again the image overlapping over the prompt, but this time I find it very hard to reproduce consistently.

There is another issue. If you click twice on the prompt (without double clicking), initially the image remains constrained. But if you double click on the image to let it expand and then click twice again on the prompt, the behavior of clicking on the prompt changes between constraining the image and expanding the image.

Last issue: After reducing the browser, all elements are re-positioned accordingly except the image:

Images are now the correct size for me.

@rsmith31415 , I'm not sure what you mean about the behaviour of clicking the prompt area after double clicking an image. In my brief tests, it's working as expected.

@takluyver Yes, sorry. I forgot to describe one crucial step to reproduce this issue. Furthermore, now I see that the issue is not really in the prompt but in resizing the browser. Steps:

1. Start your browser maximized and add a large image.
2. Resize your browser:

3 . Double click on the image to expand it.
4 . Maximize your browser (now the image is overflowing):

5 . Click twice on the prompt. The first click works as expected. The second click returns to the image overflowing.

I seem to be seeing the same things as you, but I'm not sure why you think it's wrong.

There are two separate toggles: image constrained/expanded and output open/scrollboxed. When you switch the output to scrollboxed and back, that doesn't change the image size, so if the image was overflowing before, switching the output area state twice will bring it back to overflowing.

I do see another problem, though. When resizing the window with the image constrained to max width, its size doesn't update as the notebook width updates. I can make it resize in width if I set max-width: 100% on the div containing the img (class="ui-wrapper ui-resizable-autohide"), but then the aspect ratio isn't preserved.

Yes, I see what you mean. I think I find confusing the feature of expanding the image outside the notebook. There is also the case in which the output is scrollboxed and you double-click to expand within the scrollbox. I think that is very reasonable. Although the image doesn't seem to preserve its dimensions, it seems like a middle ground between the expanded image and the scrollboxed image. By the way, what is the purpose of allowing the image to increase to its original size?

The problem you described is the same I was mentioning before, right? The image is not responsive to resizing the window.

I finally understood the thing that is causing problems. It is the .resizable() method from jquery-ui. Particularly, the following lines:

        //Create a wrapper element and set the wrapper to the new current internal element
        this.element.wrap(
            $("<div class='ui-wrapper' style='overflow: hidden;'></div>").css({
                position: this.element.css("position"),
                width: this.element.outerWidth(),
                height: this.element.outerHeight(),
                top: this.element.css("top"),
                left: this.element.css("left")
            })
        );

        //Overwrite the original this.element
        this.element = this.element.parent().data(
            "ui-resizable", this.element.data("ui-resizable")
        );

This is setting a width and height in pixels to the element ui-wrapper:

Therefore, when we resize the window, the old width and height remain. This is also adding a class ui-resizable to the image element with the same width and height than ui-wrapper. By the way, this is the resize handler that @jdfreder was referring to in the first place.

I was wondering whether we really need that feature. I didn't even know it existed.

I am fine without images being resizable in the UI.

On Thu, Mar 19, 2015 at 6:59 PM, rsmith31415 [email protected]
wrote:

I finally understood the thing that is causing problems. It is the
.resizable() method http://jqueryui.com/resizable/ from jquery-ui.
Particularly, the following lines:

    //Create a wrapper element and set the wrapper to the new current internal element
    this.element.wrap(
        $("<div class='ui-wrapper' style='overflow: hidden;'></div>").css({
            position: this.element.css("position"),
            width: this.element.outerWidth(),
            height: this.element.outerHeight(),
            top: this.element.css("top"),
            left: this.element.css("left")
        })
    );

    //Overwrite the original this.element
    this.element = this.element.parent().data(
        "ui-resizable", this.element.data("ui-resizable")
    );

This is setting a width and height in pixels to the element ui-wrapper:

[image: screenshot from 2015-03-19 16 28 38]
https://cloud.githubusercontent.com/assets/599274/6742580/8e487e9a-ce56-11e4-84e9-0cad3c9a9d85.png

Therefore, when we resize the window, the old width and height remain.
This is also adding a class ui-resizable to the image element with the
same width and height than ui-wrapper. By the way, this is the resize
handler that @jdfreder https://github.com/jdfreder was referring to in
the first place.

I was wondering whether we really need that feature. I didn't even know it
existed.

—
Reply to this email directly or view it on GitHub
#8062 (comment).

Brian E. Granger
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
[email protected] and [email protected]

If removing .resizable() is permissible, I think this issue can be solved very easily. I created a branch with the changes. I tested quickly and works okay:

Thanks for investigating this. Let's see what others think and spend time testing it out.

Sent from my iPhone

On Mar 20, 2015, at 3:59 AM, rsmith31415 [email protected] wrote:

If removing .resizable() is permissible, I think this issue can be solved very easily. I created a branch with the changes. I tested quickly and works okay:

—
Reply to this email directly or view it on GitHub.

I'm fine with removing resize

@rsmith31415 do you want to submit a PR to this PR with your proposed changes to get rid of the resizing?

the double-click is not applied to markdown images at all. It could be, though I'm inclined to leave it out for now.

I think you would expect images using markdown and IPython.display.Image to have the same behavior. However, I think that's not good for markdown cells. How could you add the double click feature without interfering with the double clicking to see the markdown code?

They are working, in that they set the width and height attributes of the img tag. If the image is larger than the display area, max-width takes precedence and shrinks the image.

Well, it is working under the hood but not working from the user's perspective. I'm not sure what to do about it.

One reason to not add iframes is the max-width: 100%; height: auto doesn't preserve the aspect ratio correctly the same way it does on images so extra fixes would be needed to make iframes work. For that reason alone, I'm inclined to leave them out, at least initially.

Actually, you describe there another aspect. Right now, height is set to auto. That has the unpleasant effect of extending the cell vertically without limits (well, the limit is the image's height).

I think I found something very strange. After double clicking to expand the image, the menu to copy the image URL is disabled. It is also disabled when the image is constrained to 100%. This doesn't happen in a markdown cell.

By the way, I think I asked this before but now that I'm testing this feature, I'm wondering again what is the purpose of allowing to scroll the raw image? I like the part when the image is constrained with max-width: 100%; but once I double click, I'm not entirely sure that view is useful.

How could you add the double click feature without interfering with the double clicking to see the markdown code?

I can prevent the event from bubbling up. I just need to move the functionality to a common place shared by text cell rendering and output areas.

Well, it is working under the hood but not working from the user's perspective.

If the user's perspective is that their width, height should always be respected, regardless of whether it fits in the output area, that's exactly the problematic behavior of 3.0 that we are trying to fix here. Note that the vast majority of explicit width and height are not set explicitly by the user, but under the hood by a library.

That has the unpleasant effect of extending the cell vertically without limits

I don't understand what you are describing here. It sets the height of the image according to max-width in order to preserve the aspect ratio of the original width, height. Is there any other behavior that's sensible?

Okay, I see only one decision left to make here, and it's what to do with the double-click behavior:

1. (current) affects output images, not markdown
2. apply to both markdown, output
3. remove entirely

I don't have a strong preference, but it makes sense that markdown and output should behave the same.

Output also has the scroll option. I think it's OK to have more features for controlling how output is viewed than for markdown images.

If the user's perspective is that their width, height should always be respected, regardless of whether it fits in the output area, that's exactly the problematic behavior of 3.0 that we are trying to fix here. Note that the vast majority of explicit width and height are not set explicitly by the user, but under the hood by a library.

Yes, you're right. I guess that's not a big problem. But it would be better if we could come up with a set of features that are consistent. If it is going to create scrollbars, it should always create scrollbars. If it is going to be constrained to max-width, then that should always be the case. Actually, that's not possible because, as you said, some elements (such as dataframes) shouldn't be subject to max-width.

I don't understand what you are describing here. It sets the height of the image according to max-width in order to preserve the aspect ratio of the original width, height. Is there any other behavior that's sensible?

Let me show you:

Notice that the image's height is large enough to require an extremely large cell in the vertical direction. In Chrome, it creates a useless vertical scrollbar:

In Firefox, there is not vertical scrollbar.

If the double click feature is useful, obviously I would like to keep it. However, I can't think of a reason to maintain it right now. We would be sacrificing consistency and simpler code, so in my mind, it needs to be really useful to justify its inclusion.

Since the double-click behavior is now reduced to a trivial CSS class toggle (.unconfined), I'm currently somewhat inclined to remove it from this PR, and save it for a discrete proposal, once someone decides it is useful.

Ahh, got it! It is working now...ok back to this PR...

• I really like all of the behaviors in the example notebook.
• I am fine waiting until someone asks for the unconfined dblclick behavior.

One question though: what is the default setting of images returned by the matplotlib inline backend? My initial thought is that they should have a max-width so they don't escape.

I am also fine leaving the unconfined class there and dblclick there. If we don't include the dblclick now, would you keep the unconfined class and kwarg?

what is the default setting of images returned by the matplotlib inline backend?

Like all images, the default behavior is to be confined by max-width.

If we don't include the dblclick now, would you keep the unconfined class and kwarg?

Yes, I think being able to request the behavior from the display-side is actually more important than the double-click behavior.

OK, I am fine with merging this with or without the dblclick event - everything else looks fine.

Sorry, I should have mentioned somewhere that I diagnosed the missing schema issue in some tests, and it came down to doing git clean -fxd to remove old files.

Yep, that did it for me...thanks!

On Sun, Mar 29, 2015 at 3:54 PM, Thomas Kluyver [email protected]
wrote:

Sorry, I should have mentioned somewhere that I diagnosed the missing
schema issue in some tests, and it came down to doing git clean -fxd to
remove old files.

—
Reply to this email directly or view it on GitHub
#8062 (comment).

Brian E. Granger
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
[email protected] and [email protected]

Ok, great let's merge, try and complain :-)

Yeah, great work everyone - that one was surprisingly painful...

On Sun, Mar 29, 2015 at 4:33 PM, Matthias Bussonnier <
[email protected]> wrote:

Merged #8062 #8062.

—
Reply to this email directly or view it on GitHub
#8062 (comment).

Brian E. Granger
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
[email protected] and [email protected]