feat(invisibleMessage): introduce invisibleMessage util #3192

ivoplashkov · 2021-04-28T07:28:29Z

Introduce InvisibleMessage util to allow custom announcements by screen readers.

The aria-live region provide a way to programmatically expose dynamic content changes in a way that can be announced by assistive technologies. Thanks to that we are able to inform the users of assistive technologies for a change that has happened to the UI.

How to use:

Import the util with => import announce from "@ui5/webcomponents-base/dist/util/InvisibleMessage.js";
The "announce" method expects two arguments, first one is the string to be announced and the second one is the InvisibleMessageMode enum, which is either "Polite" or "Assertive", depending on the priority we wish to apply.

packages/main/test/specs/base/InvisibleMessage.spec.js

packages/base/src/util/InvisibleMessage.js

d3xter666 · 2021-05-17T09:24:25Z

packages/base/src/util/InvisibleMessage.js

+politeSpan.style.cssText = styles;
+assertiveSpan.style.cssText = styles;
+
+if (!politeSpan.parentElement) {


Wouldn't this be always true?

Please check if you have pushed the changes.

d3xter666 · 2021-05-17T09:26:03Z

packages/base/src/util/InvisibleMessage.js

+	const span = mode === InvisibleMessageMode.Assertive ? assertiveSpan : politeSpan;
+
+	// Set textContent to empty string in order to trigger screen reader's announcement.
+	span.textContent = "";


Why you'd need to make it empty and then assign the message?

If someone wants to announce the same message over and over again on some interaction, the screen reader won't be triggered if we don't empty the value.

Are we sure that way, it'd be read out?

d3xter666 · 2021-05-17T09:30:30Z

packages/main/test/specs/base/InvisibleMessage.spec.js

+        checkBox.click();
+        button.click();
+
+        assert.ok(politeSpan.getHTML().indexOf("announcement") > -1, "value has been announced");


This is not true "value has been announced"
It is true that it has been rendered

Please check if you have pushed the changes.

d3xter666 · 2021-05-18T06:50:15Z

packages/base/src/util/InvisibleMessage.js

+politeSpan.style.cssText = styles;
+assertiveSpan.style.cssText = styles;
+
+if (!politeSpan.parentElement) {


Please check if you have pushed the changes.

d3xter666 · 2021-05-18T06:50:33Z

packages/main/test/specs/base/InvisibleMessage.spec.js

+        checkBox.click();
+        button.click();
+
+        assert.ok(politeSpan.getHTML().indexOf("announcement") > -1, "value has been announced");


Please check if you have pushed the changes.

MapTo0

lgtm

d3xter666

🚢 🇮🇹

vladitasev · 2021-08-02T07:38:33Z

Please add me as a reviewer for all significant changes to the base package in the future.

vladitasev · 2021-08-02T07:29:26Z

packages/base/src/types/InvisibleMessageMode.js

@@ -0,0 +1,30 @@
+import DataType from "./DataType.js";


This file should be called AnnouncementMode IMO

vladitasev · 2021-08-02T07:30:29Z

packages/base/src/util/InvisibleMessage.js

+import InvisibleMessageMode from "../types/InvisibleMessageMode.js";
+import getSingletonElementInstance from "./getSingletonElementInstance.js";
+
+const styles = `position: absolute;


This whole code is not CSP compliant. We cannot set CSS like this (innerText, cssText, etc...). Use the imperative APIs instead.

vladitasev · 2021-08-02T07:33:17Z

packages/base/src/util/InvisibleMessage.js

+	top: -1000px;
+	pointer-events: none;`;
+
+const politeSpan = document.createElement("span");


This code must be synchronized with the lifecycle of the framework. You can achieve this in one of two ways:

Make the creation of these spans lazy (during the call of the announce function - an only create the span that is announced - polite or assertive)

Export another function from this file, and call it in boot.js to create the spans safely after the HTML is loaded..

I prefer the first.

vladitasev · 2021-08-02T07:34:47Z

packages/base/src/util/InvisibleMessage.js

+	const span = mode === InvisibleMessageMode.Assertive ? assertiveSpan : politeSpan;
+
+	// Set textContent to empty string in order to trigger screen reader's announcement.
+	span.textContent = "";


This could would then become:
getSpan(mode).textContent = message;
where getSpan is a local function for this module that returns the span, if created, or creates and then returns it.

vladitasev · 2021-08-02T07:35:30Z

packages/main/bundle.common.js

@@ -103,6 +103,7 @@ import applyDirection from "@ui5/webcomponents-base/dist/locale/applyDirection.j
 import { attachDirectionChange } from "@ui5/webcomponents-base/dist/locale/directionChange.js";
 import ResizeHandler from "@ui5/webcomponents-base/dist/delegate/ResizeHandler.js";
 import * as defaultTexts from "./dist/generated/i18n/i18n-defaults.js";
+import announce from "@ui5/webcomponents-base/dist/util/InvisibleMessage.js";


This is an antipattern - if a module has an export default, it should coincide with the name of the module.

vladitasev · 2021-08-02T07:37:20Z

packages/base/src/util/InvisibleMessage.js

@@ -0,0 +1,48 @@
+import InvisibleMessageMode from "../types/InvisibleMessageMode.js";


This file does not belong to util in its current form. This is a module with side effects (creates DOM) and not a utility such as camelToKebabCase.

* feat(invisibleMessage): introduce invisibleMessage util * apply comments * fix typo * apply comments

ivoplashkov added 2 commits April 28, 2021 10:08

feat(invisibleMessage): introduce invisibleMessage util

2e890eb

Merge remote-tracking branch 'origin/master' into invisibleMessage

6a490fd

elenastoyanovaa requested changes Apr 29, 2021

View reviewed changes

packages/main/test/specs/base/InvisibleMessage.spec.js Outdated Show resolved Hide resolved

packages/base/src/util/InvisibleMessage.js Show resolved Hide resolved

ivoplashkov requested a review from d3xter666 May 10, 2021 07:28

ivoplashkov added 2 commits May 17, 2021 11:03

Merge remote-tracking branch 'origin/master' into invisibleMessage

4589c6f

apply comments

8e4b8a0

d3xter666 requested changes May 17, 2021

View reviewed changes

fix typo

f5bd3db

d3xter666 requested changes May 18, 2021

View reviewed changes

apply comments

307974f

ivoplashkov requested a review from MapTo0 May 18, 2021 07:33

MapTo0 approved these changes May 18, 2021

View reviewed changes

d3xter666 approved these changes May 18, 2021

View reviewed changes

ivoplashkov requested a review from elenastoyanovaa May 18, 2021 10:02

elenastoyanovaa approved these changes May 18, 2021

View reviewed changes

ivoplashkov merged commit 0a33c40 into SAP:master May 18, 2021

vladitasev reviewed Aug 2, 2021

View reviewed changes

vladitasev mentioned this pull request Aug 2, 2021

Fix InvisibleMessage.js #3582

Closed

ilhan007 pushed a commit that referenced this pull request Sep 20, 2021

feat(invisibleMessage): introduce invisibleMessage util (#3192)

021619b

* feat(invisibleMessage): introduce invisibleMessage util * apply comments * fix typo * apply comments

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(invisibleMessage): introduce invisibleMessage util #3192

feat(invisibleMessage): introduce invisibleMessage util #3192

ivoplashkov commented Apr 28, 2021

d3xter666 May 17, 2021

ivoplashkov May 17, 2021

d3xter666 May 18, 2021

d3xter666 May 17, 2021

ivoplashkov May 17, 2021

d3xter666 May 18, 2021

d3xter666 May 17, 2021

ivoplashkov May 17, 2021

d3xter666 May 18, 2021

d3xter666 May 18, 2021

d3xter666 May 18, 2021

MapTo0 left a comment

d3xter666 left a comment

vladitasev commented Aug 2, 2021

vladitasev Aug 2, 2021

vladitasev Aug 2, 2021

vladitasev Aug 2, 2021

vladitasev Aug 2, 2021

vladitasev Aug 2, 2021

vladitasev Aug 2, 2021

		@@ -0,0 +1,48 @@
		import InvisibleMessageMode from "../types/InvisibleMessageMode.js";

feat(invisibleMessage): introduce invisibleMessage util #3192

feat(invisibleMessage): introduce invisibleMessage util #3192

Conversation

ivoplashkov commented Apr 28, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MapTo0 left a comment

Choose a reason for hiding this comment

d3xter666 left a comment

Choose a reason for hiding this comment

vladitasev commented Aug 2, 2021

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