instructure / canvas-lms

The open LMS by Instructure, Inc.
https://github.com/instructure/canvas-lms/wiki
GNU Affero General Public License v3.0
5.45k stars 2.44k forks source link

LTI Content Item deep link image is squashed on thin screens #2121

Open karendolan opened 1 year ago

karendolan commented 1 year ago

Summary:

Canvas distorts Content Item images on a thin screen when the width of the content item's image hits the max-width CSS Canvas constraint. This is because the LTI content item is required to pass an image height & width in px. The width is overridden with max-width but the height remains fixed/

This shows the current problem, and how height:auto affects the image. The proposed solution is to add .lti-thumbnail-launch img style to force height to always be auto and width to be defined up until max-width:100% kicks in.

DistortedLTIContentItemImage
  1. Existing Canvas default IMG CSS that causes max-width to kick in... https://github.com/instructure/canvas-lms/blob/master/app/stylesheets/components/_ic-reset.scss#L21-L28
// Responsive images
img {
  max-width: 100%;
  height: auto;
 ...
}
  1. Existing Canvas default height and width of LTI embed that forces at least a default if height px & width px not passed... Follows the LTI spec requiring height and width to be in pixels LTI spec: https://www.imsglobal.org/lti/model/mediatype/application/vnd/ims/lti/v1/contentitems+json/index.html#ContentItemPlacement Canvas: https://github.com/instructure/canvas-lms/blob/master/app/controllers/external_content_controller.rb#L185-L191

    def default_placement_advice
    IMS::LTI::Models::ContentItemPlacement.new(
      presentation_document_target: "default",
      display_height: 600,
      display_width: 800
    )
    end
  2. A Possible solution... with a potential problem but in keeping with Canvas image treatment... Use CSS selector to override height .lti-thumbnail-launch img

    // Responsive LTI content item deep link images
    .lti-thumbnail-launch img {
    height: auto !important;
    ...
    }

    Possible Problem: This causes height auto to always be honored and the height from the LTI content item to always be ignored. The image will never be distorted because the width will be honored and the ratio of the height will match the width until max-width kicks in to scrunch the image. Currently, it's possible to intentionally distort the image, until the max-width kicks in. However, the strategy of preventing the image from ever being distorted fits in with the current Canvas CSS styling approach, so an undistorted image fits the Canvas way of doing things.

Steps to reproduce:

  1. Do an LTI content item deep link embed on a Canvas page and save the page.
  2. Squash the browser window as thin as you can so that the image is constrained by the max-width: 100%; and the image starts distorting.
  3. Notice that the image distorts as the width is squashed. The max-width: 100% overrides the fixed width of the embedded image, but the height: auto does not override the fixed height of the embedded image.

Expected behavior:

The height:auto protects the LTI embed image from being squashed

Actual behavior:

The height:auto does not protect the LTI embed image from being squashed

karendolan commented 1 year ago

I think this is a better patch, I'll verify and make a pull...

.lti-thumbnail-launch > img { object-fit: contain; }

Found a better idea: Permit the LTI content item thumbnail image to have the style that every other Canvas image element is allowed to have "auto"!!!

On: https://github.com/instructure/canvas-lms/blob/master/ui/shared/deep-linking/models/helpers.ts#L104-L113

Change to

  if (iframe.width) {
    iframeTag.style.width = iframe.width === 'auto' ? 'auto' : `${iframe.width}px`;
  }

  if (iframe.height) {
    iframeTag.style.height = iframe.height === 'auto' ? 'auto' : `${iframe.height}px`;
  }

and add unit test passing

                  "thumbnail":{
                    "height":"auto",
                    "width":128,
                    "@id":"http://www.runeaudio.com/assets/img/banner-archlinux.png"
                  },