terra-image UX Audit

[ryanthemanuel]

terra-image

Known Aliases

Document any known aliases of the component, decide if another name makes more sense.

• [ ] Has known aliases
• [ ] Requires name change

Design Standard Doc

Any documentation needed to be added to terra-ui describing the make up and usage of the component Any doc examples that need to be updated?

• [ ] Missing design standard documentation linkage.

Guides

Any guides needed to be added to terra-ui describing the usage of this component when used with other components.

• [ ] Missing Guides
• [ ] Missing UX recommended Usage.
• [ ] Deprecation guide

Accessibility Guides

• [ ] Missing Accessibility Guides

Behaviours

• [X] Missing behaviours
  • alt tag should be required. if it's a purely decorative image it should be ' '.
  • aria labels should never be applied. Potentially block this from being set as a custom prop.
  • onLoad/onError have potential accessibility implications and should only be used if teams know what they're doing. Possibly privatize them?
• [ ] Contains bad practice behaviours that should be removed

Accessibility

• [ ] Meets wcag 2.0/section 508 standards
• [ ] Meets wcag 2.1/en 301 549 standards

Deprecation

• [ ] Component is a bad pattern and should be deprecated.

Related Issues

Other Considerations

It could be possible in the future to provide some canned placeholders to be used in well defined situations.

[supreethmr]

Tech Design

Summary:

Approach 1

Add imageType prop to terra-image to differentiate between informative and decorative image.

Requirements and Accessibility:

As a consumer of Terra-Image, I would like it have API to support for meaningful images (ability to be read by screen-readers) when they add important context for users and support for decorative images (ignored and hidden to screen-readers), so that Image meets WCAG Success Criterion 1.1.1 (Non-text Content).

New React Props:

Terra-Image

Prop	Type	isRequired	Default	Description
imageType	Enum	optional	'informative'	Specifies Type of image which is either informative OR decorative images

Implementation

    createImage(customProps, imageClasses) {
    const {
      src, alt, height, width, imageType
    } = this.props;

    if (imageType === ImageTypes.DECORATIVE) {
     // Decorative Image
      return (
        <img
-          {...customProps}
          src={src}
+          role="presentation"
+          alt=""
          height={height}
          width={width}
          onLoad={this.handleOnLoad}
          onError={this.handleOnError}
          className={imageClasses}
          ref={this.ImageRef}
        />
      );
    }

// Informative Image (default )
    return (
      <img
        {...customProps}
        src={src}
        alt={alt}
        height={height}
        width={width}
        onLoad={this.handleOnLoad}
        onError={this.handleOnError}
        className={imageClasses}
        ref={this.ImageRef}
      />
    );
  }

Usage Example

import Image from 'terra-image';

export default () => <Image src={placeholder150x150} title="example image" imageType="decorative" />;

import Image from 'terra-image';

export default () => <Image src={placeholder150x150} title="example image" imageType="informative" alt="example image"/>;

With this approach we would require to flex the image tag depending on it's type. For Decorative Image we need to

• clear the value of alt ( if provided by user )
• add a role presentation ( extra prop to ensure the it is non text content )
• remove the customProps

Flexing between image types might add up more functions in a single file and might became difficult to maintain when we provide support for more image types along with informative and decorative.

[supreethmr]

Tech Design

Summary:

Approach 2

Add new subcomponent Decorative Image to render Image API as a non-text content.

Requirements and Accessibility:

As a consumer of Terra-Image, I would like it have API to support for meaningful images (ability to be read by screen-readers) when they add important context for users and support for decorative images (ignored and hidden to screen-readers), so that Image meets WCAG Success Criterion 1.1.1 (Non-text Content).

React Props of New Sub Component:

Decorative-Image

Prop	Type	isRequired	Default	Description
src	string	required	none	The source string for the image which will be loaded and displayed.
variant	Enum	optional	'default'	Sets the style of the image from the following values; default, rounded, circle, thumbnail.
isFluid	bool	optional	false	Sets the fluid behavior of the image, which is non-fluid by default.
placeholder	element	optional	none	A React element which will be displayed during loading and in case of src load failure.
height	string	optional	none	Sets the height of the image
width	string	optional	none	Sets the width of the image
onLoad	function	optional	none	Function to be executed when the image load is successful.
onError	function	optional	none	Function to be executed when the image load errors.
fit	enum	optional	'fill'	Sets the object-fit style of the image from the following values; cover, contain, fill, scale-down, none.

Implementation

import Image from './Image';
import DecorativeImage from './variants/DecorativeImage';

export default Image;
export { DecorativeImage };

createImage(imageClasses) {
    const {
      src, height, width,
    } = this.props;

    return (
      <img
        src={src}
        role="presentation"
        alt=""
        height={height}
        width={width}
        onLoad={this.handleOnLoad}
        onError={this.handleOnError}
        className={imageClasses}
        ref={this.ImageRef}
      />
    );
  }

Usage Example

import { DecorativeImage } from 'terra-image';

export default () => <DecorativeImage src={placeholder150x150} title="example image" />;

import Image from 'terra-image'; OR import InformativeImage from 'terra-image'; 

export default () => <Image src={placeholder150x150} title="example image"  alt="example image" />;

With this Approach we can create new sub-components in future when new image types are created. This reduces the load of an existing API by moving the implementation of new image types to different file.

Existing Image would remain as default Export of the package and new code changes are passive to existing Image API's.

[benbcai]

I think in design #2 to create a new subcomponent for this is unnecessary because most of the core features remain the same with the exception of the image type. For this reason, I like design #1 better and it is easier to maintain.

What is the reason to prevent spreading the custom props for a decorative image?

[benbcai]

alt is currently a required prop. For a decorative image it is no longer needed and will be overwritten with an empty string. Do we still have to keep it as required? I think so because it is still needed for the informative image type. Maybe we can just update the prop description to call out that it is not required if the imageType is decorative and will be set to an empty string.

[sdadn]

I agree with @benbcai . While it is good to plan ahead to easily allow updates to the API, Design 2 feels overkill with the scope of this story and information we have right now. I opt for design 1 because it's simpler to consume. A consumer may want to update their UI and update an image from informative or decorative and they can do that by simply updating a prop in Design 1 instead of needing to change the component.

[supreethmr]

alt is currently a required prop. For a decorative image it is no longer needed and will be overwritten with an empty string. Do we still have to keep it as required? I think so because it is still needed for the informative image type. Maybe we can just update the prop description to call out that it is not required if the imageType is decorative and will be set to an empty string.

Keeping alt prop as required would make it inappropriate for decorative image as consumer cannot skip alt prop and passing alt prop with empty string will not make sense for consumer as it should be handled internally.

So either we should make alt prop as optional with doc update to highlight that value for alt prop is required to make image accessible to assistive technologies.

[supreethmr]

I think in design #2 to create a new subcomponent for this is unnecessary because most of the core features remain the same with the exception of the image type. For this reason, I like design #1 better and it is easier to maintain.

we can actually move the code to different file and refer it on both Image.jsx and DecorativeImage.jsx I think this would not require much effort on code review as we are not introducing lot of code changes with new subcomponent. To make this code changes passive the existing Image API will remain unchanged as a default API for informative Images.

What is the reason to prevent spreading the custom props for a decorative image?

Edit: for custom props there are few props that would be inappropriate to use like title, aria-label, aria-labelledby and aria-describedby . these attributes would make decorative image accessible to assistive technologies. So these props should be removed from image when the imageType is decorative

Aria role presentation throws axe error when any of the aria attributes are included in the element.

[benbcai]

for custom props there are few props that would be inappropriate to use like title, aria-label, aria-labelledby and aria-describedby . these attributes would make decorative image accessible to assistive technologies. So these props should be removed from image when the imageType is decorative

Could we selectively delete these unwanted props instead of not spreading all custom props?

For examples:

delete customProps.title;
delete customProps.aria-label;

[neilpfeiffer]

Tech Design Decision

Based on our conversations, the API that has the best the method to educate consumers as to their responsibility for accessibility, Approach 2 will be the most clear and have the most amount of enforceability.

While Approach 1 is more flexible, its puts more onus on the consumer to understand their accessibility responsibilities, and it has more opportunities to create erratic end behaviors if intended to be decorative but title, aria-label are provided, or meant not to be decorative and alt is not able to be strictly enforced.

Approach 2 also has ability to not create necessarily additional code overhead for maintenance as the two exposed components will largely be exposing API-only, and we can move all of the current component logic into a shared single private subcomponent that smartly will add or remove the props and relevant html attributes to create stable markup for both situations. It also allows for additional W3C image concepts types in the future that will have their own unique API demands.

To summarize the changes:

Image.jsx

• props: src Required, alt Required, variant, isFluid, height, width, fit, placeholder, onLoad, onError
• will continue to be the default export. Changes to current will be to remove 'alt' having a default value, making it strictly enforced as required propType (will be a major version bump to revert this change: https://github.com/cerner/terra-core/commit/ae649abd28783bebff35450eaafe0f3804d3924f, since even though alt was marked as required propType, it is not enforced by having a default value)
• will remove role="presentation" or role="none" if added by consumers by spreading custom props

DecorativeImage.jsx

• props: src Required, variant, isFluid, height, width, fit, placeholder, onLoad, onError
• will provide alt="" and role="presentation" by default, still allow spreading custom props, but remove any specific props/html attributes if they are provided by the consumer that will conflict with making the image appear hidden/decorative to the screenreader, such as title, aria-label, aria-labelledby, aria-describedby, etc. as needed.

/private/_image.jsx

• imported and used by both components and applies or removes props and html attributes as needed

This will also resolve https://github.com/cerner/terra-core/issues/3396