[CEF 2171] Image views don't scale when the height of the view changes

[jasonsanjose]

1. Open the find in files panel
2. Resize it tall enough to cause scrolling when an image is viewed
3. Select an image from the project tree

Actual: Image is clipped on the bottom edge
Expected: Scale to fit or provide scroll bars

[ingorichter]

This might be a dupe of #10175

[JeffryBooher]

@ingorichter the image viewer doesn't have scrollbars -- it scales the image to fit the view
This looks like it may have been just a regression from split view. Checking now

[JeffryBooher]

Looks to be a regression with CEF 2171. It also doesn't scale the image when resizing the app vertically. It scales properly when the width of the window changes, but not when the height changes.

[JeffryBooher]

I don't think this is sever enough to hold up the 1.1 release so marking this for 1.2; if we see other issues related to app/view resizing then we may want to consider upping the priority.

CC:@peterflynn

[peterflynn]

I can confirm this happens in-browser too. I agree this seems lower-priority than #10175.

[peterflynn]

I've been able to boils this down to a simple standalone repro case: https://gist.github.com/peterflynn/6612dca2dcd163648991 (copy this into the Getting Started project so the image file is there). This hardcodes an editor-area size of 1690x594 px using the same CSS/DOM setup as the Brackets UI.

In our old brackets-shell, this renders with the image resized to fit in 594px height:

In the new CEF, the height restriction is ignored and the image resizes to occupy the full 1690px width, even though that makes it taller than 594px:

[peterflynn]

Note: I updated the repro Gist to an even 500px height for simplicity.

Some related info:
http://stackoverflow.com/questions/1948249/how-to-set-maximum-height-for-img-but-keep-proportions
http://stackoverflow.com/questions/17329040/img-max-height-100-causes-img-to-exceed-bounds

[EJanuszewski]

I believe whilst fixing the meta data I may have also fixed this, see 356/357 https://github.com/EJanuszewski/brackets/commit/30cfd3833df69b6df1ad64836eebf9dad3e207b6#diff-d170b001614b4677146b7f46339e2d88R356

[EJanuszewski]

@prksingh See my comment above, from what I can tell it seems to be working fine after those changes.

[EJanuszewski]

Is there something wrong with the fix I did? Just curious why it's not been merged as it's tagged for the 1.2 release :)

[prafulVaishnav]

@EJanuszewski Can you create the pull request for this fix? I have added a note at EJanuszewski@30cfd38#diff-d170b001614b4677146b7f46339e2d88R356.

[EJanuszewski]

@prafulVaishnav Already did just here #10364, let me know if there's something wrong with that pull and i'll make the changes

[prafulVaishnav]

@EJanuszewski This pull contain fixes for different issues. Can you please create different pull request for different files? Brackets.less has fix for issue #5960 and #10180. QuickView.less has fix for issue #10362..

[EJanuszewski]

@prafulVaishnav Sure, I didn't know different files needed different pull requests especially when they are related.

To clarify I need to make another pull request for #10362 and somehow remove https://github.com/EJanuszewski/brackets/commit/45a74c50c1de5300a0a04dfff8010565650ca26d from the current pull request?

[prafulVaishnav]

@EJanuszewski Ideally we make pull requests for each issue. In this case, change in Brackets.less is fixing two issues and change is related to both issues #5960 and #10180. That's why one pull request for two issues is fine. Whereas, issue #10362 and its fix is independent so creating separate pull request will be good. Hope I clarify..

It will be easy if you create two new pull requests. In your case, one PR(Pull Request) containing Brackets.less file change and another PR containing QuickView.less file change.

Please let me know if any doubt.

[EJanuszewski]

#10514 New pull request, thanks for clarifying it all, I must have somehow missed the branch bit in all the excitement of wanting to get involved.

[prafulVaishnav]

Pull request #10514 merged.. Thanks @EJanuszewski for the fix..