fix(Accessibility): Color alone is used to convey information #336

mariang-ens · 2024-02-19T11:26:35Z

https://jira.corp.adobe.com/browse/SITES-10934

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.

Pareesh · 2024-07-25T08:48:32Z

LGTM.
Please make sure to test the color input component across porduct to ensure it is not breaking any changes.
cc : @mariang-ens

majornista

So as to not break the component layout. We should use coral-tooltip to show the value. To provide the suggested changes, I implemented this locally. Should I commit those changes to this PR?

majornista · 2024-07-25T18:10:35Z

coral-component-colorinput/src/templates/colorLabel.html

@@ -0,0 +1 @@
+<label handle="colorLabel" type="label"></label>


Simply including the color value as a block element breaks the layouts of both the ColorInput and ColorInputSwatches. It may be better to use a CoralTooltip to display the value on hover or focus.

Suggested change

<label handle="colorLabel" type="label"></label>

<coral-tooltip handle="colorLabel" placement="top" class="_coral-ColorInput-swatch-tooltip"></coral-tooltip>

majornista · 2024-07-25T18:12:02Z

coral-component-colorinput/src/templates/base.html

  <button handle="colorPreview" is="coral-button" variant="_custom" class="_coral-FieldButton _coral-ColorInput-button _coral-ColorInput-preview" type="button" aria-haspopup="dialog" aria-expanded="false" aria-controls="{{uid}}"></button>
+  <label handle="colorPreviewLabel" type="label"></label>


Simply including the color value as a block element breaks the layouts of both the ColorInput and ColorInputSwatches. It may be better to use a CoralTooltip to display the value on hover or focus.

Suggested change

<button handle="colorPreview" is="coral-button" variant="_custom" class="_coral-FieldButton _coral-ColorInput-button _coral-ColorInput-preview" type="button" aria-haspopup="dialog" aria-expanded="false" aria-controls="{{uid}}"></button>

<label handle="colorPreviewLabel" type="label"></label>

<button handle="colorPreview" is="coral-button" variant="_custom" class="_coral-FieldButton _coral-ColorInput-button _coral-ColorInput-preview" type="button" aria-haspopup="dialog" aria-expanded="false" aria-controls="{{uid}}" id="{{uid}}-colorPreview" aria-describedby="{{uid}}-tooltip"></button>

<coral-tooltip handle="colorPreviewLabel" class="_coral-ColorInput-preview-tooltip" id="{{uid}}-tooltip" target="#{{uid}}-colorPreview" placement="top"></coral-tooltip>

majornista · 2024-07-25T18:14:44Z

coral-component-colorinput/src/scripts/ColorInputSwatch.js

    colorButton.call(this._elements);
+    colorLabel.call(this._elements);


Pass commons through to the colorButton template so that we can call generate a uid within the template.

Suggested change

colorButton.call(this._elements);

colorLabel.call(this._elements);

colorButton.call(this._elements, {commons});

colorLabel.call(this._elements);

majornista · 2024-07-25T18:16:06Z

coral-component-colorinput/src/scripts/ColorInputSwatch.js

@@ -107,10 +109,12 @@ const ColorInputSwatch = Decorator(class extends BaseColorInputAbstractSubview(B
    if (cssColorValue) {
      this._elements.colorButton.style.backgroundColor = cssColorValue;
      this._elements.colorButton.label.textContent = hexColorValue;
+      this._elements.colorLabel.textContent = hexColorValue;


Using coral-tooltip we set the textContent of the content element.

Suggested change

this._elements.colorLabel.textContent = hexColorValue;

this._elements.colorLabel.content.textContent = hexColorValue;

majornista · 2024-07-25T18:16:41Z

coral-component-colorinput/src/scripts/ColorInputSwatch.js

      this.setAttribute('data-value', hexColorValue);
    } else {
      this._elements.colorButton.classList.add('_coral-ColorInput-swatch-novalue');
      this._elements.colorButton.label.textContent = i18n.get('unset');
+      this._elements.colorLabel.textContent = i18n.get('unset');


Using coral-tooltip we set the textContent of the content element.

Suggested change

this._elements.colorLabel.textContent = i18n.get('unset');

this._elements.colorLabel.content.textContent = i18n.get('unset');

majornista · 2024-07-25T18:18:08Z

coral-component-colorinput/src/scripts/ColorInputSwatch.js

    this.appendChild(this._elements.colorButton);
+    this.appendChild(this._elements.colorLabel);


The coral-tooltip should target the colorButton element.

Suggested change

this.appendChild(this._elements.colorLabel);

this.appendChild(this._elements.colorLabel);

this._elements.colorLabel.target = '#' + this._elements.colorButton.id;

majornista · 2024-07-25T18:19:36Z

coral-component-colorinput/src/styles/index.styl

+
+  label {
+    display block;
+  }


Fix text wrapping:

Suggested change

label {

display block;

}

._coral-ColorInput-swatch-tooltip {

min-width: max-content;

}

majornista · 2024-07-25T18:21:27Z

coral-component-colorinput/src/styles/index.styl

 }

 ._coral-ColorInput-buttonWrapper {
  position: absolute;
  right: $colorinput-button-wrapper-offset;
  top: $colorinput-button-wrapper-offset;
+  text-align: center;

  // set opacity < 1 to create new stacking context (for checker image to work)
  opacity: .999;


Fix text wrapping within tooltip:

Suggested change

opacity: .999;

opacity: .999;

._coral-ColorInput-preview-tooltip {

min-width: max-content;

}

…label

…es tests)

…oral-spectrum into issue/SITES-10934

mariang-ens · 2024-08-01T06:15:25Z

Hello @majornista @Pareesh . I see that all the comments have been resolved and I updated the branch with the latest master branch. Can you please approve & merge this PR when you have some time?

ens13533 added 4 commits February 19, 2024 13:24

add labels to color picker swatches

a5525bb

fixed curly brace warnings

547c197

fixed curly brace warnings

b3324f1

fixed curly brace warnings

b2073bc

mariang-ens mentioned this pull request Feb 20, 2024

SITES-10934: Color alone is used to convey information. #319

Closed

StoianLucian requested a review from Pareesh February 22, 2024 09:58

StoianLucian changed the title ~~SITES-10934: Color alone is used to convey information~~ fix(SITES-10934) Color alone is used to convey information Mar 18, 2024

mariang-ens changed the title ~~fix(SITES-10934) Color alone is used to convey information~~ fix(Accessibility): Color alone is used to convey information Apr 23, 2024

StoianLucian requested a review from majornista July 25, 2024 09:19

majornista requested changes Jul 25, 2024

View reviewed changes

majornista and others added 4 commits July 25, 2024 15:28

fix(Accessibility): use tooltip to fix broken layouts caused by text …

66ddfa2

…label

fix(Accessibility): ColorInput popup button doesn't need tooltip (fix…

c2e1ca6

…es tests)

Merge branch 'master' into issue/SITES-10934

f4abd38

Merge branch 'issue/SITES-10934' of https://github.com/MarianGiurge/c…

a0d0093

…oral-spectrum into issue/SITES-10934

Merge branch 'master' into issue/SITES-10934

525225b

StoianLucian requested a review from majornista August 9, 2024 07:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(Accessibility): Color alone is used to convey information #336

fix(Accessibility): Color alone is used to convey information #336

mariang-ens commented Feb 19, 2024

Pareesh commented Jul 25, 2024 •

edited

Loading

majornista left a comment •

edited

Loading

majornista Jul 25, 2024

majornista Jul 25, 2024

majornista Jul 25, 2024

majornista Jul 25, 2024

majornista Jul 25, 2024

majornista Jul 25, 2024

majornista Jul 25, 2024

majornista Jul 25, 2024

mariang-ens commented Aug 1, 2024

		@@ -0,0 +1 @@
		<label handle="colorLabel" type="label"></label>

	<label handle="colorLabel" type="label"></label>
	<coral-tooltip handle="colorLabel" placement="top" class="_coral-ColorInput-swatch-tooltip"></coral-tooltip>

		<button handle="colorPreview" is="coral-button" variant="_custom" class="_coral-FieldButton _coral-ColorInput-button _coral-ColorInput-preview" type="button" aria-haspopup="dialog" aria-expanded="false" aria-controls="{{uid}}"></button>
		<label handle="colorPreviewLabel" type="label"></label>

		colorButton.call(this._elements);
		colorLabel.call(this._elements);

	this._elements.colorLabel.textContent = hexColorValue;
	this._elements.colorLabel.content.textContent = hexColorValue;

		this.appendChild(this._elements.colorButton);
		this.appendChild(this._elements.colorLabel);

-  label {
-    display block;
-  }
+  ._coral-ColorInput-swatch-tooltip {
+    min-width: max-content;
+  }

fix(Accessibility): Color alone is used to convey information #336

Are you sure you want to change the base?

fix(Accessibility): Color alone is used to convey information #336

Conversation

mariang-ens commented Feb 19, 2024

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Pareesh commented Jul 25, 2024 • edited Loading

majornista left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariang-ens commented Aug 1, 2024

Pareesh commented Jul 25, 2024 •

edited

Loading

majornista left a comment •

edited

Loading