Merge pull request #16567 from desktop/improve-ghe-login-a11y

Improve GitHub Enterprise login accessibility

app/src/ui/autocompletion/autocompleting-text-input.tsx

@@ -38,7 +38,7 @@ interface IAutocompletingTextInputProps<ElementType, AutocompleteItemType> {
   readonly disabled?: boolean
 
   /** Indicates if input field should be required */
-  readonly isRequired?: boolean
+  readonly required?: boolean
 
   /**
    * Indicates if input field should be considered a combobox by assistive
@@ -390,7 +390,7 @@ export abstract class AutocompletingTextInput<
       onBlur: this.onBlur,
       onContextMenu: this.onContextMenu,
       disabled: this.props.disabled,
-      'aria-required': this.props.isRequired ? true : false,
+      required: this.props.required ? true : false,
       spellCheck: this.props.spellcheck,
       autoComplete: 'off',
       'aria-labelledby': this.props.elementAriaLabelledBy,
@@ -439,6 +439,7 @@ export abstract class AutocompletingTextInput<
     const tagName = this.getElementTagName()
     const className = classNames(
       'autocompletion-container',
+      'no-invalid-state',
       this.props.className,
       {
         'text-box-component': tagName === 'input',

app/src/ui/changes/commit-message.tsx

@@ -874,7 +874,7 @@ export class CommitMessage extends React.Component<
           {this.renderAvatar()}
 
           <AutocompletingInput
-            isRequired={true}
+            required={true}
             className={summaryInputClassName}
             placeholder={this.props.placeholder}
             value={this.state.summary}

app/src/ui/lib/authentication-form.tsx

@@ -8,6 +8,7 @@ import { Button } from './button'
 import { TextBox } from './text-box'
 import { Errors } from './errors'
 import { getDotComAPIEndpoint } from '../../lib/api'
+import { HorizontalRule } from './horizontal-rule'
 
 /** Text to let the user know their browser will send them back to GH Desktop */
 export const BrowserRedirectMessage =
@@ -104,6 +105,8 @@ export class AuthenticationForm extends React.Component<
         <TextBox
           label="Username or email address"
           disabled={disabled}
+          required={true}
+          displayInvalidState={false}
           // eslint-disable-next-line jsx-a11y/no-autofocus
           autoFocus={true}
           onValueChanged={this.onUsernameChange}
@@ -113,6 +116,8 @@ export class AuthenticationForm extends React.Component<
           label="Password"
           type="password"
           disabled={disabled}
+          required={true}
+          displayInvalidState={false}
           onValueChanged={this.onPasswordChange}
         />
 
@@ -151,16 +156,6 @@ export class AuthenticationForm extends React.Component<
     )
   }
 
-  private renderSignInWithBrowser() {
-    return (
-      <>
-        {this.renderSignInWithBrowserButton()}
-
-        {this.props.additionalButtons}
-      </>
-    )
-  }
-
   /**
    * Show the sign in locally form
    *
@@ -174,7 +169,8 @@ export class AuthenticationForm extends React.Component<
       this.renderUsernamePassword()
     ) : (
       <>
-        {this.renderSignInWithBrowser()}
+        {this.renderSignInWithBrowserButton()}
+        <HorizontalRule title="or" />
         {this.renderUsernamePassword()}
       </>
     )

app/src/ui/lib/horizontal-rule.tsx

@@ -0,0 +1,10 @@
+import React from 'react'
+
+/** Horizontal rule/separator with optional title. */
+export const HorizontalRule: React.FunctionComponent<{
+  readonly title?: string
+}> = props => (
+  <div className="horizontal-rule">
+    <span className="horizontal-rule-content">{props.title}</span>
+  </div>
+)

app/src/ui/lib/text-box.tsx

@@ -25,6 +25,15 @@ export interface ITextBoxProps {
   /** Whether the input field is disabled. */
   readonly disabled?: boolean
 
+  /** Indicates if input field should be required */
+  readonly required?: boolean
+
+  /**
+   * Indicates whether or not the control displays an invalid state.
+   * Default: true
+   */
+  readonly displayInvalidState?: boolean
+
   /**
    * Called when the user changes the value in the input field.
    *
@@ -241,7 +250,11 @@ export class TextBox extends React.Component<ITextBoxProps, ITextBoxState> {
     const inputId = label ? this.state.inputId : undefined
 
     return (
-      <div className={classNames('text-box-component', className)}>
+      <div
+        className={classNames('text-box-component', className, {
+          'no-invalid-state': this.props.displayInvalidState === false,
+        })}
+      >
         {label && <label htmlFor={inputId}>{label}</label>}
 
         <input
@@ -262,6 +275,7 @@ export class TextBox extends React.Component<ITextBoxProps, ITextBoxState> {
           spellCheck={this.props.spellcheck === true}
           aria-label={this.props.ariaLabel}
           aria-controls={this.props.ariaControls}
+          required={this.props.required}
         />
       </div>
     )

app/src/ui/sign-in/sign-in.tsx

@@ -20,6 +20,7 @@ import { getWelcomeMessage } from '../../lib/2fa'
 import { getDotComAPIEndpoint } from '../../lib/api'
 import { OkCancelButtonGroup } from '../dialog/ok-cancel-button-group'
 import { Button } from '../lib/button'
+import { HorizontalRule } from '../lib/horizontal-rule'
 
 interface ISignInProps {
   readonly dispatcher: Dispatcher
@@ -239,9 +240,7 @@ export class SignIn extends React.Component<ISignInProps, ISignInState> {
           </Button>
         </Row>
 
-        <div className="horizontal-rule">
-          <span className="horizontal-rule-content">or</span>
-        </div>
+        <HorizontalRule title="or" />
 
         <Row>
           <TextBox

app/styles/ui/_autocompletion.scss

@@ -6,40 +6,11 @@
   flex-direction: column;
 }
 
-.CodeMirror-hints {
-  list-style: none;
-  flex-direction: column;
-  padding: 0;
-  max-height: 100px;
-
-  // hack to position the dialog a little better than the
-  // default codemirror position.
-  transform: translate(0, -15px);
-  overflow-y: auto;
-
-  li {
-    flex-shrink: 0;
-    height: 29px;
-    padding: 0 var(--spacing);
-  }
-
-  // We can't replicate the nice scrollbars that we have in
-  // the regular autocomplete popup since that's based on List
-  // and this CodeMirror stuff is just an unordered list.
-  // The autocomplete popup is not intended to be used with
-  // a pointer-device anyway so not being able to mouse-wheel
-  // through it shouldn't be a big deal
-  @include win32 {
-    overflow-y: hidden;
-  }
-}
-
 .autocompletion-popup {
   overflow: hidden; // To get those sweet rounded corners
 }
 
-.autocompletion-popup,
-.CodeMirror-hints {
+.autocompletion-popup {
   display: flex;
   position: absolute;
   z-index: var(--popup-z-index);
@@ -69,8 +40,7 @@
       border-top: var(--base-border);
     }
 
-    &.selected,
-    &.CodeMirror-hint-active {
+    &.selected {
       --text-color: var(--box-selected-active-text-color);
       --text-secondary-color: var(--box-selected-active-text-color);
 
@@ -78,8 +48,7 @@
       background-color: var(--box-selected-active-background-color);
       border-top-color: var(--box-selected-active-background-color);
 
-      & + .list-item,
-      & + .CodeMirror-hint-active {
+      & + .list-item {
         border-top-color: var(--box-selected-active-background-color);
       }
     }
@@ -172,14 +141,3 @@
     }
   }
 }
-
-.author-input-component {
-  ul.CodeMirror-hints {
-    margin-top: var(--spacing-double);
-    padding: 0;
-    li {
-      margin-bottom: auto;
-      padding-left: var(--spacing);
-    }
-  }
-}

app/styles/ui/_errors.scss

@@ -2,7 +2,7 @@
   background: var(--form-error-background);
   border: 1px solid var(--form-error-border-color);
   border-radius: var(--border-radius);
-  color: var(--error-color);
+  color: var(--text-color);
   font-size: var(--font-size-sm);
   padding: var(--spacing-half);
 }

app/styles/ui/_text-box.scss

@@ -10,10 +10,6 @@
     margin-bottom: var(--spacing-third);
   }
 
-  :invalid {
-    border-color: var(--error-color);
-  }
-
   input {
     @include textboxish;
     @include textboxish-disabled;
@@ -37,4 +33,8 @@
       -webkit-mask-repeat: no-repeat;
     }
   }
+
+  &:not(.no-invalid-state) :not(:focus):invalid {
+    border-color: var(--error-color);
+  }
 }

app/styles/ui/_welcome.scss

@@ -38,6 +38,10 @@
   #sign-in-enterprise {
     .sign-in-form {
       margin-top: 0;
+
+      > button {
+        margin-right: 0;
+      }
     }
 
     .sign-in-footer {