[terra-embedded-content-consumer] - Show focus indicator on iframe of the embedded content when it has focus #1827

kolkheang · 2023-10-05T00:12:48Z

Summary

Show visual focus indicator on the frame when the embedded content has focus. This allows keyboard only users to see where focus is and use keyboard to scroll the embedded content when content is not fully visible within the viewport.

What was changed:
Showing outline style on the frame when it has focus and remove the outline style when it's not in focus (blur)

Why it was changed:
It's necessary for keyboard only users to see where the focus is and give users indication that while focus is on the frame, they can use arrow keys to scroll the content inside the frame when needed (if content isn't fully visible and scrolling is enabled)

Testing

This change was tested using:

Reviews

In addition to engineering reviews, this PR needs:

UX review
Accessibility review
Functional review

Additional Details

This library depends on the changes in xfc library which is currently under review with oracle-samples/xfc#95

Doc Site: https://engineering.cerner.com/terra-framework/pull/1827/

Screen Recording using keyboard navigation with tab key:
https://github.com/cerner/terra-framework/assets/3170314/71e86961-94a2-4ea9-bf53-a55e25c6bed6

This PR resolves:

UXPLATFORM-9564

Thank you for contributing to Terra.
@cerner/terra

…different URLs

…frame's contentWindow

…updated examples, add style scss modules

…rame when in focus

packages/terra-embedded-content-consumer/package.json

packages/terra-embedded-content-consumer/CHANGELOG.md

package.json

Co-authored-by: Saad Adnan <[email protected]>

vinaybhargavar · 2023-10-12T17:41:42Z

packages/terra-framework-docs/CHANGELOG.md

@@ -5,6 +5,9 @@
 * Added
  * Added documentation for FlowsheetDataGrid in `terra-data-grid`.

+* Updated
+  * Updated examples, and tests to show the visual focus indicator on the iframe of the embedded content


nit: We can add a . at the EOL.

aalexanderj · 2023-10-13T13:23:58Z

...ra-framework-docs/src/terra-dev-site/doc/embedded-content-consumer/example/BasicConsumer.jsx

@@ -4,14 +4,10 @@ import { Consumer } from 'xfc';
 import '../providers/EmbeddedContentConsumerCommon.module.scss';

 Consumer.init();
-


nit: can we keep the newline here or remove it from the other doc files as well?

aalexanderj · 2023-10-13T13:27:30Z

...k-docs/src/terra-dev-site/doc/embedded-content-consumer/providers/BasicProvider.provider.jsx

@@ -7,7 +7,8 @@ const cx = classNames.bind(styles);

 const Provider = () => (
  <ProviderTestTemplate>
-    <div className={cx('content-wrapper')}>
+    {/* eslint-disable-next-line */}
+    <div className={cx('content-wrapper')} tabIndex="0" style={{ outline: 'none' }}>


nit: can we add a new class to use here that includes the outline style?

scottwilmarth · 2023-10-13T16:05:28Z

Allowing the consumers to set a "hasActionableContent" prop (that is named for intent rather than for result) would be good to allow consumers to disable the focus when they KNOW there is actionable content within the frame. BUT, I would not want the default to be the frame doesn't receive focus - would have to default to FALSE. That would provide a more accessible experience out of the box, even though it may not always be perfect.

scottwilmarth · 2023-10-13T16:21:08Z

@kolkheang if we add the "hasActionableContent" prop, in addition to removing the visible focus indicator, can we also remove the tabindex="0" to ensure keyboard focus is not set on the container?

scottwilmarth · 2023-10-13T17:02:35Z

Please disregard my comments about adding a "hasActionableContent" prop. @kolkheang and I discussed it more and determined this would be moot. From an accessibility perspective, we must show a visible focus indicator if we have something with a tabindex="0" Adding the actionable content prop would have potentially allowed for failures which we do not want to do. As is, the current behavior that Kol provides ensures (as best as possible) that the container can receive keyboard focus when there is scrolling AND that the container has the required visible focus indicator to meet WCAG 2.4.7 Focus Visible criterion.

cm9361 · 2023-10-13T17:48:32Z

packages/terra-embedded-content-consumer/package.json

  },
  "dependencies": {
    "classnames": "^2.2.5",
-    "prop-types": "^15.5.8"
+    "prop-types": "^15.5.8",
+    "style-to-object": "^0.4.2"


We should not need to add this third-party dependency.

Good call. With the current way, there is just 1 style for focus outline, and one style for blur outline and we have two variables for these. We can set the value for each into the frame's style directly. Previously, I was thinking that we could set multiple style attributes for focus or blur.

cm9361 · 2023-10-13T17:56:50Z

packages/terra-embedded-content-consumer/src/EmbeddedContentConsumer.module.scss

+  $blur-outline: var(--terra-embedded-content-consumer-blur-outline, none);
+  $focus-outline: var(--terra-embedded-content-consumer-focus-outline, 2px dashed #000);
+
+  :export {
+    /* stylelint-disable property-no-unknown */
+    blurOutline: $blur-outline;
+    focusOutline: $focus-outline;
+  }


You should be able to set the styles using classes. I don't think that this is the correct implementation.

These are not for the classes to be applied to an html element like we normally do.

You would add the class to the iFrame element.

cm9361 · 2023-10-13T17:58:31Z

packages/terra-embedded-content-consumer/src/EmbeddedContentConsumer.jsx

+    this.xfcFrame?.iframe?.contentWindow?.addEventListener('focus', () => {
+      const styleObj = parse(focusStyleStr);
+      Object.entries(styleObj).forEach(([key, value]) => {
+        this.xfcFrame.iframe.style[key] = value;
+      });
+    }, true);
+
+    // Listen for blur event and callback function to apply the style
+    this.xfcFrame?.iframe?.contentWindow?.addEventListener('blur', () => {
+      const styleObj = parse(blurStyleStr);
+      Object.entries(styleObj).forEach(([key, value]) => {
+        this.xfcFrame.iframe.style[key] = value;
+      });
+    }, true);


Instead of this logic, you should be able to use the ":focus" psuedo class.

Can you give an example on how to setup the style and apply to the iframe's style?

Like this?

iframe:focus { outline: 2px dashed #000; }

I would think that your iFrame would have an associated class name or id for styling. However, it would be a similar concept to what you have described here, yes.

The :focus pseudo-class isn't working for iframe. I'll check to see if we can update the classname on the fly for switching the style.

cm9361 · 2023-10-13T18:03:09Z

packages/terra-embedded-content-consumer/src/EmbeddedContentConsumer.jsx

+    // Cover other scenario where xfc frame style doesn't apply
+    // such as when `srcdoc` attribute is used which doesn't work
+    // within xfc's JSONRPC communication.
+    this.xfcFrame?.iframe?.contentWindow?.addEventListener('focus', () => {


The contentWindow gives you a reference to the window for the embedded content. You should be able to use document.documentElement (HTML tag) or document.body to handle the logic as necessary.

Yeah, I'd like to handle the focus within the iframe element though, not everywhere else in the document body.

I think you can handle by styling the iFrame element. How are you giving focus to your iFrame? Did you set a tabIndex?

cm9361 · 2023-10-13T18:04:01Z

...ra-embedded-content-consumer/src/clinical-lowlight-theme/EmbeddedContentConsumer.module.scss

+:local {
+  .clinical-lowlight-theme {
+    --terra-embedded-content-consumer-focus-outline: 2px dashed #b2b5b6;
+    --terra-embedded-content-consumer-blur-outline: none;


We shouldn't need a blur outline style. That should just be the normal state of the element.

Are you saying we can hard-code outline: none to remove the style from the frame when there is no focus anymore? Because we set the outline style when there is focus. And if we don't remove that style, it stays.

No, I am saying that you would have CSS for the normal state. When the element is not focused, that CSS will no longer be applied.

kolkheang · 2023-10-20T16:10:42Z

Based on the meeting several of us had on Monday, there is additional changes that are needed. The changes are specifically related to checking whether the embedded document (in the iframe) has any focusable/interactable element or not and if not (using querySelectorAll), there should be logic to set the tabIndex="0" for the embedded document so that focus can be set on the embedded document when scrolling is enabled, and content is scrollable.

I'll be issuing a new PR with these additional changes and include the existing change on this PR. I'll close this PR out for now.

kolkheang added 8 commits October 4, 2023 10:36

Set visual focus indicator on the frame when the frame has focus

368c3fc

WIP - trying out different scenario

47a1105

trying different scenario. Allow all hosts in webpack config to test …

2685b29

…different URLs

WIP - trying out another approach to set style on the frame

836bd5c

WIP - setting the frame's style when focus/blur event happens on the …

5e8e4c8

…frame's contentWindow

WIP - rework the logic a to pull in the css parser string to object, …

4bedf98

…updated examples, add style scss modules

Update xfc paths and remove some files no longer needed

93d9aa3

Update wdio tests

2e9e450

kolkheang added the 📦 terra-embedded-content-consumer label Oct 5, 2023

kolkheang self-assigned this Oct 5, 2023

github-actions bot temporarily deployed to preview-pr-1827 October 5, 2023 00:13 Destroyed

Remove config to allowed host for dev server

3bf6808

github-actions bot temporarily deployed to preview-pr-1827 October 5, 2023 14:04 Destroyed

Remove package-lock.json

0727c53

github-actions bot temporarily deployed to preview-pr-1827 October 5, 2023 16:04 Destroyed

github-actions bot temporarily deployed to preview-pr-1827 October 5, 2023 16:26 Destroyed

github-actions bot temporarily deployed to preview-pr-1827 October 5, 2023 16:44 Destroyed

github-actions bot temporarily deployed to preview-pr-1827 October 5, 2023 17:07 Destroyed

Update wdio tests, add a few tests to show the outline style on the f…

e04850d

…rame when in focus

github-actions bot temporarily deployed to preview-pr-1827 October 10, 2023 04:14 Destroyed

Fix lint issues

4b57dc3

github-actions bot temporarily deployed to preview-pr-1827 October 10, 2023 14:18 Destroyed

Read styles from scss file and update wdio tests

6f6d1d9

github-actions bot temporarily deployed to preview-pr-1827 October 10, 2023 17:41 Destroyed

Fix lint issue

49aa11d

github-actions bot temporarily deployed to preview-pr-1827 October 10, 2023 18:17 Destroyed

kolkheang requested review from aalexanderj, vinaybhargavar, sdadn and sycombs October 10, 2023 18:32

github-actions bot temporarily deployed to preview-pr-1827 October 11, 2023 00:20 Destroyed

Update CHANGELOG

f417dbd

kolkheang marked this pull request as ready for review October 11, 2023 01:34

kolkheang requested a review from scottwilmarth October 11, 2023 01:34

Merge branch 'main' into show-focus-indicator-on-iframe

ec511e2

kolkheang changed the title ~~[terra-embedded-content-consumer] WIP - Show focus indicator on iframe of the embedded content when it has focus~~ [terra-embedded-content-consumer] - Show focus indicator on iframe of the embedded content when it has focus Oct 11, 2023

github-actions bot temporarily deployed to preview-pr-1827 October 11, 2023 01:53 Destroyed

kolkheang requested a review from a team October 11, 2023 15:38

sdadn reviewed Oct 11, 2023

View reviewed changes

packages/terra-embedded-content-consumer/package.json Show resolved Hide resolved

sdadn reviewed Oct 11, 2023

View reviewed changes

packages/terra-embedded-content-consumer/CHANGELOG.md Outdated Show resolved Hide resolved

sdadn reviewed Oct 11, 2023

View reviewed changes

package.json Show resolved Hide resolved

sdadn added do not merge Accessibility Review Required labels Oct 11, 2023

Update packages/terra-embedded-content-consumer/CHANGELOG.md

b1d1148

Co-authored-by: Saad Adnan <[email protected]>

github-actions bot temporarily deployed to preview-pr-1827 October 12, 2023 15:24 Destroyed

kolkheang mentioned this pull request Oct 12, 2023

Set visual focus indicator on the iframe when focus/blur style is given by consumers oracle-samples/xfc#95

Closed

vinaybhargavar reviewed Oct 12, 2023

View reviewed changes

aalexanderj reviewed Oct 13, 2023

View reviewed changes

scottwilmarth approved these changes Oct 13, 2023

View reviewed changes

scottwilmarth added the ⭐ Accessibility Reviewed label Oct 13, 2023

cm9361 reviewed Oct 13, 2023

View reviewed changes

kolkheang closed this Oct 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[terra-embedded-content-consumer] - Show focus indicator on iframe of the embedded content when it has focus #1827

[terra-embedded-content-consumer] - Show focus indicator on iframe of the embedded content when it has focus #1827

kolkheang commented Oct 5, 2023 •

edited

Loading

vinaybhargavar Oct 12, 2023

aalexanderj Oct 13, 2023

aalexanderj Oct 13, 2023

scottwilmarth commented Oct 13, 2023 •

edited

Loading

scottwilmarth commented Oct 13, 2023

scottwilmarth commented Oct 13, 2023

cm9361 Oct 13, 2023

kolkheang Oct 13, 2023

cm9361 Oct 13, 2023

kolkheang Oct 13, 2023

cm9361 Oct 13, 2023

cm9361 Oct 13, 2023

kolkheang Oct 13, 2023

kolkheang Oct 13, 2023

cm9361 Oct 13, 2023 •

edited

Loading

kolkheang Oct 20, 2023

cm9361 Oct 13, 2023

kolkheang Oct 13, 2023

cm9361 Oct 13, 2023

cm9361 Oct 13, 2023

kolkheang Oct 13, 2023

cm9361 Oct 13, 2023

kolkheang commented Oct 20, 2023

		@@ -4,14 +4,10 @@ import { Consumer } from 'xfc';
		import '../providers/EmbeddedContentConsumerCommon.module.scss';

		Consumer.init();

[terra-embedded-content-consumer] - Show focus indicator on iframe of the embedded content when it has focus #1827

[terra-embedded-content-consumer] - Show focus indicator on iframe of the embedded content when it has focus #1827

Conversation

kolkheang commented Oct 5, 2023 • edited Loading

Summary

Testing

Reviews

Additional Details

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottwilmarth commented Oct 13, 2023 • edited Loading

scottwilmarth commented Oct 13, 2023

scottwilmarth commented Oct 13, 2023

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

cm9361 Oct 13, 2023 • 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

kolkheang commented Oct 20, 2023

kolkheang commented Oct 5, 2023 •

edited

Loading

scottwilmarth commented Oct 13, 2023 •

edited

Loading

cm9361 Oct 13, 2023 •

edited

Loading