Comments

trong.nguyen.tcec created an issue. See original summary.

cilefen’s picture

Title: Lage image is overflow » Large images are not responsive, exceed the available space
abbym’s picture

Here is a patch for this for Drupal 8, in which it is also an issue; needs review. Thanks!

abbym’s picture

Version: 7.54 » 8.4.x-dev
Status: Active » Needs review
cilefen’s picture

abbym’s picture

FileSize
1.23 MB
1.34 MB

I've added before and after screenshots (before: large image overflowing the viewport width; after: image scaling to no more than 100% of the width of its container).

abbym’s picture

Issue tags: -Needs screenshots
abbym’s picture

Manuel Garcia’s picture

Status: Needs review » Needs work

Its been sometime since I've worked on CSS, but I think we should be adding height: auto; as well.

Manuel Garcia’s picture

Issue tags: +Novice
pritish.kumar’s picture

Status: Needs work » Needs review
FileSize
366 bytes
244 bytes

The CSS "height = auto;" added.

mahalingam_cs’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
600.24 KB
1.15 MB

Applied patch from #11 and it worked proper. Attached Before and After screenshot. The larger image is displayed properly now.

Manuel Garcia’s picture

+1 to rtbc

Dinesh18’s picture

Reviewed the patch #11, works as expected. +1 to RTBC

Cottser’s picture

This seems sensible and non-disruptive to me, and we have the same CSS in Bartik's elements.css. This CSS is almost boilerplate for responsive CSS.

lauriii’s picture

Status: Reviewed & tested by the community » Fixed

Committed 643e4b8 and pushed to 8.4.x. Thanks!

  • lauriii committed 643e4b8 on 8.4.x
    Issue #2854111 by abbym, pritish.kumar, mahalingam_cs, trong.nguyen.tcec...
Wim Leers’s picture

Status: Fixed » Needs work

This should be reverted, because height: auto overrides <img src="…" height="100">. So instead of the image taking up 100px as specified in the HTML, it takes up as much height as the actual image file contains…

Wim Leers’s picture

Title: Large images are not responsive, exceed the available space » Revert: Large images are not responsive, exceed the available space
Status: Needs work » Reviewed & tested by the community
Wim Leers’s picture

Wim Leers’s picture

Issue summary: View changes
FileSize
19.33 KB

Proof:

(Sorry for the many separate comments — multi-tasking fail.)

abbym’s picture

With regards to comments 18-21 above, reverting `height: auto` undermines the entire purpose of this ticket, which is to make images responsive.

The last submitted patch, 11: drupal-responsive_images_seven-2854111-11.patch, failed testing. View results

lauriii’s picture

Status: Reviewed & tested by the community » Needs review

@Wim Leers: Where is the height attribute being used / what did this break? I tried grepping Drupal Core and I didn't find any usages of the height attribute..

Wim Leers’s picture

FileSize
358.68 KB
291.53 KB

If you install https://github.com/acquia/reservoir, it's at /admin/api/advanced. Code: https://github.com/acquia/reservoir/blob/1.0.0-alpha1/modules/reservoir_....

8.4.x HEAD
See 2854111-committed.png
8.4.x with this patch reverted
See 2854111-reverted.png
Manuel Garcia’s picture

@Wim Leers you could use the style attribute to override this css,
'style' => 'float:right; height: 100px;',

Wim Leers’s picture

Issue tags: +BC break

Ehm… that's totally not cool. Also, that is still a BC break.

We should not be breaking HTML basics.

Manuel Garcia’s picture

I know its not cool, just trying to help.

The reason we need to set height: auto; on the images that have max-width: 100%, is that otherwise when an image has a height attribute set and its wider than its container, then max-width: 100%; will make them skewed because the height will remain to what the attribute says, while its width will shrink to that of its container.

Seven being responsive, it's understandable how when people find that images are not, so they rise a bug report. I think that this is closer to being a bug fix than a BC break.