-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Accessibility improvements #126
Changes from all commits
ae0ba03
d7ba36b
94d1bb8
f44a34b
795f90f
b17c242
2dca081
07a4160
452a62d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,8 @@ on: | |
push: | ||
branches: | ||
- master | ||
# TODO temporary | ||
- a11y-1 | ||
|
||
jobs: | ||
deploy: | ||
|
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,10 +9,12 @@ import { | |
customElement, | ||
css, | ||
property, | ||
query, | ||
internalProperty, | ||
PropertyValues, | ||
html, | ||
} from 'lit-element'; | ||
import {nothing} from 'lit-html'; | ||
import {ifDefined} from 'lit-html/directives/if-defined.js'; | ||
import {CodeMirror} from './lib/codemirror.js'; | ||
import codemirrorStyles from './_codemirror/codemirror-styles.js'; | ||
|
@@ -49,12 +51,40 @@ export class PlaygroundCodeEditor extends LitElement { | |
position: relative; | ||
} | ||
|
||
#focusContainer { | ||
height: 100%; | ||
position: relative; | ||
} | ||
|
||
.CodeMirror { | ||
height: 100% !important; | ||
font-family: inherit !important; | ||
border-radius: inherit; | ||
} | ||
|
||
#keyboardHelpScrim { | ||
position: absolute; | ||
width: 100%; | ||
height: 100%; | ||
left: 0; | ||
top: 0; | ||
display: flex; | ||
align-items: center; | ||
justify-content: center; | ||
background: transparent; | ||
z-index: 999; | ||
pointer-events: none; | ||
} | ||
|
||
#keyboardHelp { | ||
background: #00000099; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we get a little more line spacing and a little less left/right padding? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
padding: 20px 80px; | ||
border-radius: 10px; | ||
color: white; | ||
font-family: sans-serif; | ||
font-size: 18px; | ||
} | ||
|
||
.CodeMirror-foldmarker { | ||
font-family: sans-serif; | ||
} | ||
|
@@ -164,6 +194,15 @@ export class PlaygroundCodeEditor extends LitElement { | |
position: string; | ||
}; | ||
|
||
@internalProperty() | ||
private _showKeyboardHelp = false; | ||
|
||
@query('#focusContainer') | ||
private _focusContainer?: HTMLDivElement; | ||
|
||
@query('.CodeMirror-code') | ||
private _codemirrorEditable?: HTMLDivElement; | ||
|
||
private _resizeObserver?: ResizeObserver; | ||
private _resizing = false; | ||
private _valueChangingFromOutside = false; | ||
|
@@ -212,15 +251,34 @@ export class PlaygroundCodeEditor extends LitElement { | |
} | ||
|
||
render() { | ||
if (this.readonly) { | ||
return this._cmDom; | ||
} | ||
return html` | ||
${this._cmDom} | ||
<div | ||
id="tooltip" | ||
?hidden=${!this._tooltipDiagnostic} | ||
style=${ifDefined(this._tooltipDiagnostic?.position)} | ||
id="focusContainer" | ||
tabindex="0" | ||
@mousedown=${this._onMousedown} | ||
@focus=${this._onFocus} | ||
@blur=${this._onBlur} | ||
@keydown=${this._onKeyDown} | ||
> | ||
<div part="diagnostic-tooltip"> | ||
${this._tooltipDiagnostic?.diagnostic.message} | ||
${this._showKeyboardHelp | ||
? html`<div id="keyboardHelpScrim"> | ||
<p id="keyboardHelp"> | ||
Press Enter to start editing<br />Press Escape to exit editor | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider styling on "Enter" and "Escape" to make them stand out a bit? Maybe bold or monospace? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in #127 |
||
</p> | ||
</div>` | ||
: nothing} | ||
${this._cmDom} | ||
<div | ||
id="tooltip" | ||
?hidden=${!this._tooltipDiagnostic} | ||
style=${ifDefined(this._tooltipDiagnostic?.position)} | ||
> | ||
<div part="diagnostic-tooltip"> | ||
${this._tooltipDiagnostic?.diagnostic.message} | ||
</div> | ||
</div> | ||
</div> | ||
`; | ||
|
@@ -273,6 +331,11 @@ export class PlaygroundCodeEditor extends LitElement { | |
lineNumbers: this.lineNumbers, | ||
mode: this._getLanguageMode(), | ||
readOnly: this.readonly, | ||
inputStyle: 'contenteditable', | ||
// Don't allow naturally tabbing into the editor, because it's a | ||
// tab-trap. Instead, the container is focusable, and Enter/Escape are | ||
// used to explicitly enter the editable area. | ||
tabindex: -1, | ||
} | ||
); | ||
cm.on('change', () => { | ||
|
@@ -295,6 +358,36 @@ export class PlaygroundCodeEditor extends LitElement { | |
this._codemirror = cm; | ||
} | ||
|
||
private _onMousedown() { | ||
// Directly focus editable region. | ||
this._codemirrorEditable?.focus(); | ||
} | ||
|
||
private _onFocus() { | ||
// Outer container was focused, either by tabbing from outside, or by | ||
// pressing Escape. | ||
this._showKeyboardHelp = true; | ||
} | ||
|
||
private _onBlur() { | ||
// Outer container was unfocused, either by tabbing away from it, or by | ||
// pressing Enter. | ||
this._showKeyboardHelp = false; | ||
} | ||
|
||
private _onKeyDown(event: KeyboardEvent) { | ||
if (event.key === 'Enter' && event.target === this._focusContainer) { | ||
this._codemirrorEditable?.focus(); | ||
// Prevent typing a newline from this same event. | ||
event.preventDefault(); | ||
} else if (event.key === 'Escape') { | ||
// Note there is no API for "select the next naturally focusable element", | ||
// so instead we just re-focus the outer container, from which point the | ||
// user can tab to move focus entirely elsewhere. | ||
this._focusContainer?.focus(); | ||
} | ||
} | ||
|
||
/** | ||
* Create hidden and folded regions for playground-hide and playground-fold | ||
* comments. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wdyt about some background shading here so that the help message is more obviously associated with the source editor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in #127