[Monaco editor]: Fix accessibility focus trap issue by sebelga · Pull Request #107292 · elastic/kibana

sebelga · 2021-07-30T15:11:08Z

This PR fixes an accessibility issue with the <CodeEditor /> component from the kibana_react plugin where a user would not be able to exit the editor with a keyboard.

Hitting the ESC key will now display an overlay to the user that allows him to navigate to the next element on the page.

How to test

Navigate to the index pattern management app
Click "Add field"
Start navigating with the keyboard using the "TAB" key
Enable "Set value" with the SPACE key
TAB to access the script editor
An overlay should appear to indicate to hit "ENTER" to start writing
Hit ENTER
Write emi ---> you should have the suggestion menu open
Hit the "ESC" key. The suggestion menu should close
Hit the "ESC" again ---> you should now see the overlay back
Hit TAB to go to the next item (the link below the editor)

Note on implementation

I decided to mock the MonacoEditor component in order to be able to not test implementation detail of the <CodeEditor /> and call methods on the component directly. This allows us to have an <textarea /> rendered in our test and interact with it.

elasticmachine · 2021-08-02T10:55:25Z

Pinging @elastic/kibana-accessibility (Project:Accessibility)

elasticmachine · 2021-08-02T11:13:03Z

Pinging @elastic/kibana-stack-management (Team:Stack Management)

src/plugins/kibana_react/public/code_editor/code_editor.tsx

Co-authored-by: Tim Roes <mail@timroes.de>

sebelga · 2021-08-02T14:17:59Z

@elasticmachine merge upstream

src/plugins/kibana_react/public/code_editor/code_editor.tsx

timroes

I've changed the wording for readOnly mode:

There is unfortunately another bug left. When the editor can be scrolled out of view the tooltip is potentially attached out of view too, as you can see (or rather not see) in the following screenshot:

To reproduce that go to Discover when having doc_table:legacy turned off in your advanced setting and open a document and go to the JSON tab.

Since this was one of the initial problems, that we wanted the tooltip to solve, @cchaos do you have any idea how to fix that with the tooltip?

cchaos

I'm not sure we can fully handle the situation where the editor is so long that it's height is greater than it's container. We are currently relying on the browser's innate functionality to ensure that it is visible within the scroll container, which is a win. But neither the overlay nor the tooltip solution seem to handle this scenario well. I'm ok with this behavior as it also is one that's only seen when using keyboard + mouse (a rare-er scenario).

But I do agree with @timroes that we should get rid of the overlay background entirely. There's no need to obscure the text. In fact, it just forces the user to have to interact with the code editor in order to read it which is not ideal.

Question: When I'm in edit mode, and hit tab, it takes me out of the editor. Is this supposed to happen?

Also I'd love for @myasonik to have the final pass at this before he leaves us 😢

src/plugins/kibana_react/public/code_editor/editor.scss

cchaos · 2021-08-10T17:07:19Z

src/plugins/kibana_react/public/code_editor/code_editor.tsx

+          <div>
+            <EuiText>
+              <p>
+                {isReadOnly ? (
+                  <FormattedMessage
+                    id="kibana-react.kibanaCodeEditor.startEditingReadOnly"
+                    defaultMessage="Press Enter to start interacting with the code."
+                  />
+                ) : (
+                  <FormattedMessage
+                    id="kibana-react.kibanaCodeEditor.startEditing"
+                    defaultMessage="Press Enter to start editing."
+                  />
+                )}
+              </p>
+            </EuiText>
+
+            <EuiText>
+              <p>
+                {isReadOnly ? (
+                  <FormattedMessage
+                    id="kibana-react.kibanaCodeEditor.stopEditingReadOnly"
+                    defaultMessage="Press Escape to stop interacting with the code."
+                  />
+                ) : (
+                  <FormattedMessage
+                    id="kibana-react.kibanaCodeEditor.stopEditing"
+                    defaultMessage="Press Escape to stop editing."
+                  />
+                )}
+              </p>
+            </EuiText>
+          </div>


The EuiTooltip already has text styles applied to it so this can be simplified to:

Suggested change

<div>

<EuiText>

<p>

{isReadOnly ? (

<FormattedMessage

id="kibana-react.kibanaCodeEditor.startEditingReadOnly"

defaultMessage="Press Enter to start interacting with the code."

/>

) : (

<FormattedMessage

id="kibana-react.kibanaCodeEditor.startEditing"

defaultMessage="Press Enter to start editing."

/>

)}

</p>

</EuiText>

<EuiText>

<p>

{isReadOnly ? (

<FormattedMessage

id="kibana-react.kibanaCodeEditor.stopEditingReadOnly"

defaultMessage="Press Escape to stop interacting with the code."

/>

) : (

<FormattedMessage

id="kibana-react.kibanaCodeEditor.stopEditing"

defaultMessage="Press Escape to stop editing."

/>

)}

</p>

</EuiText>

</div>

<>

<p>

{isReadOnly ? (

<FormattedMessage

id="kibana-react.kibanaCodeEditor.startEditingReadOnly"

defaultMessage="Press Enter to start interacting with the code."

/>

) : (

<FormattedMessage

id="kibana-react.kibanaCodeEditor.startEditing"

defaultMessage="Press Enter to start editing."

/>

)}

</p>

<p>

{isReadOnly ? (

<FormattedMessage

id="kibana-react.kibanaCodeEditor.stopEditingReadOnly"

defaultMessage="Press Escape to stop interacting with the code."

/>

) : (

<FormattedMessage

id="kibana-react.kibanaCodeEditor.stopEditing"

defaultMessage="Press Escape to stop editing."

/>

)}

</p>

</>

And it looks like according to the writing guidelines the key references should be exactly whats on the keyboard, plus capitalization and bold. So it should render as:

Press Esc to stop editing.

@cchaos Could you refer me to the exact part of the writing guidelines? I am not finding it anywhere in https://elastic.github.io/eui/#/guidelines/writing

I am mainly asking because I wonder what that means for "Enter" above? I mean the key does not state "Enter" on most keyboard, but has a symbol on it, are we trying to use that symbol in that case too?

Sorry, I had meant to link to the doc I got them from (had to ask a UX writer): https://wiki.elastic.co/display/DOC/On+text+highlighting

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

timroes · 2021-08-11T08:24:43Z

Question: When I'm in edit mode, and hit tab, it takes me out of the editor. Is this supposed to happen?

This should only happen in readOnly editors, and there it's expected behavior since there is no actual user input, so I'd say that Tab should bring you out of the editor there. I can't reproduce this for editors that are not read-only. If you're experience it there, it's an issue we'd need to look into.

No longer valid

sebelga · 2021-08-11T12:14:15Z

When I'm in edit mode, and hit tab, it takes me out of the editor. Is this supposed to happen?

I just tested and I can't reproduce it either in edit mode.

@cchaos @myasonik I think all the change request have been addressed, can you have another look? Thanks!

cchaos · 2021-08-11T14:42:43Z

This should only happen in readOnly editors

Thanks, yeah! I forgot that the one in Discover is readOnly. 🤦

myasonik

LGTM!

Code review and tested in VoiceOver. A great fix for a broad feature!

cjcenizal · 2021-08-11T20:38:35Z

@cchaos Do you have time to review and update your request for changes? We're trying to get this in today. Thanks!

cchaos · 2021-08-11T21:25:29Z

@cjcenizal I was waiting for the changes to the rendered format of the keys displayed in the tooltip: #107292 (comment)

cjcenizal · 2021-08-11T21:40:49Z

OK thanks, I'll let @timroes handle that -- he is taking this PR up now that Seb is on PTO.

kibanamachine · 2021-08-12T10:55:02Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`kibanaReact`	312.1KB	318.7KB	+6.6KB

History

💚 Build #144357 succeeded adb1869
💔 Build #144336 failed 52c485f
💔 Build #144334 failed 1616931
💔 Build #144329 failed 280c9db
💔 Build #144056 failed 38a42b1

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cchaos

Awesome! LGTM!

# Conflicts: # src/plugins/kibana_react/public/code_editor/__snapshots__/code_editor.test.tsx.snap # src/plugins/kibana_react/public/code_editor/code_editor.tsx # x-pack/plugins/canvas/public/components/expression_input/__stories__/__snapshots__/expression_input.stories.storyshot

…108386) * [Monaco editor]: Fix accessibility focus trap issue (#107292) # Conflicts: # src/plugins/kibana_react/public/code_editor/__snapshots__/code_editor.test.tsx.snap # src/plugins/kibana_react/public/code_editor/code_editor.tsx # x-pack/plugins/canvas/public/components/expression_input/__stories__/__snapshots__/expression_input.stories.storyshot * update canvas snapshot Co-authored-by: Sébastien Loix <sabee77@gmail.com>

sebelga requested review from a team as code owners July 30, 2021 15:11

sebelga added 4 commits August 2, 2021 09:51

Convert component to function component

a3a6acf

Show keyboard hint when hitting Escape

99c4596

Add tests for showing/hiding keyboard hint on ESC keydown

2b3d576

Update snapshot test and set editor height to 100%

2c845f7

sebelga force-pushed the code-editor-a11y branch from 72f772b to 2c845f7 Compare August 2, 2021 10:11

Remove unnecessary useEffect and use the editor from onDidMount handler

57a85c5

sebelga added bug Fixes for quality problems that affect the customer experience Project:Accessibility release_note:fix v7.14.1 v7.15.0 v8.0.0 labels Aug 2, 2021

sebelga requested review from myasonik and timroes August 2, 2021 10:56

sebelga added the Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// label Aug 2, 2021

timroes reviewed Aug 2, 2021

View reviewed changes

src/plugins/kibana_react/public/code_editor/code_editor.tsx Outdated Show resolved Hide resolved

timroes reviewed Aug 2, 2021

View reviewed changes

src/plugins/kibana_react/public/code_editor/code_editor.tsx Outdated Show resolved Hide resolved

timroes reviewed Aug 2, 2021

View reviewed changes

src/plugins/kibana_react/public/code_editor/code_editor.tsx Outdated Show resolved Hide resolved

timroes reviewed Aug 2, 2021

View reviewed changes

src/plugins/kibana_react/public/code_editor/code_editor.tsx Outdated Show resolved Hide resolved

sebelga and others added 2 commits August 2, 2021 12:58

Apply suggestions from code review

6b3a080

Co-authored-by: Tim Roes <mail@timroes.de>

Use monaco KeyCode map for "Escape"

6756332

timroes mentioned this pull request Aug 2, 2021

Remove EuiCodeEditor from advanced settings #107394

Closed

10 tasks

Fix TS issue

6921170

Merge branch 'master' into code-editor-a11y

6d061fe

timroes reviewed Aug 2, 2021

View reviewed changes

src/plugins/kibana_react/public/code_editor/code_editor.tsx Outdated Show resolved Hide resolved

Change wording for readOnly mode

9ca119a

timroes self-requested a review August 10, 2021 13:50

timroes previously requested changes Aug 10, 2021

View reviewed changes

cchaos suggested changes Aug 10, 2021

View reviewed changes

timroes and others added 5 commits August 11, 2021 09:58

Update src/plugins/kibana_react/public/code_editor/code_editor.tsx

f803178

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

Update src/plugins/kibana_react/public/code_editor/editor.scss

280c9db

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

Update snapshots and formatting

3bcd4ae

Merge branch 'master' into pr/107292

1616931

Fix missing focus border

52c485f

Fix TS issue

adb1869

myasonik approved these changes Aug 11, 2021

View reviewed changes

Change keyboard shortcut formatting

009ae79

timroes requested a review from cchaos August 12, 2021 08:31

cchaos approved these changes Aug 12, 2021

View reviewed changes

alisonelizabeth merged commit 338f7c5 into elastic:master Aug 12, 2021

alisonelizabeth mentioned this pull request Aug 12, 2021

[7.x] [Monaco editor]: Fix accessibility focus trap issue (#107292) #108383

Merged

alisonelizabeth pushed a commit to alisonelizabeth/kibana that referenced this pull request Aug 12, 2021

[Monaco editor]: Fix accessibility focus trap issue (elastic#107292)

3a227f8

alisonelizabeth mentioned this pull request Aug 12, 2021

[7.14] [Monaco editor]: Fix accessibility focus trap issue (#107292) #108386

Merged

alisonelizabeth added a commit that referenced this pull request Aug 12, 2021

[Monaco editor]: Fix accessibility focus trap issue (#107292) (#108383)

50283ac

sebelga deleted the code-editor-a11y branch August 20, 2021 08:29

Conversation

sebelga commented Jul 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to test

Note on implementation

Uh oh!

elasticmachine commented Aug 2, 2021

Uh oh!

elasticmachine commented Aug 2, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sebelga commented Aug 2, 2021

Uh oh!

Uh oh!

timroes left a comment

Choose a reason for hiding this comment

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cchaos Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

cchaos Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

timroes Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

cchaos Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

timroes commented Aug 11, 2021

Uh oh!

sebelga commented Aug 11, 2021

Uh oh!

cchaos commented Aug 11, 2021

Uh oh!

myasonik left a comment

Choose a reason for hiding this comment

Uh oh!

cjcenizal commented Aug 11, 2021

Uh oh!

cchaos commented Aug 11, 2021

Uh oh!

cjcenizal commented Aug 11, 2021

Uh oh!

kibanamachine commented Aug 12, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

sebelga commented Jul 30, 2021 •

edited

Loading