feat(FEC-7551): handling keyboard a11y #409

RoyBregman · 2019-09-19T09:20:20Z

User should be able to navigate menus using the TAB and the UP/DOWN arrows

This reverts commit d10fda5.

RoyBregman · 2019-09-22T13:08:02Z

src/components/language/language.js

+          }
+        }}
+        className={style.smartContainerItem}
+        onSelect={props.onSelect}>


check if needed (onSelect)

RoyBregman · 2019-09-22T13:11:41Z

src/components/menu/menu.js

+            setFirstFocusedElement={props.setFirstFocusedElement}
+            addAccessibleChild={props.addAccessibleChild}
+            isSelected={this.isSelected}
+            onSelect={this.onSelect.bind(this)}


change to arrow function

RoyBregman · 2019-09-22T13:14:30Z

src/components/menu/menu.js

+        ref={se => {
+          this._selectedElement = props.isSelected(props.data) ? se : this._selectedElement;
+          props.setFirstFocusedElement(this._selectedElement);
+          if (!this._mounted) {


check for better pattern to add it only onMount

RoyBregman · 2019-09-22T13:20:26Z

src/ui-presets/playback.js

@@ -41,6 +41,7 @@ export function playbackUI(props: any): React$Element<any> {
    <div className={style.playbackGuiWWrapper}>
      <KeyboardControl player={props.player} config={props.config} />
      <Loading player={props.player} />
+      <PictureInPictureOverlay player={props.player} />


check if it better inside player gui

RoyBregman · 2019-09-22T13:27:00Z

src/utils/popup-item-keyboard-accessibility.js

+     */
+    render(props: any): React$Element<any> | void {
+      return h(
+        'div',


see example here #410

yairans · 2019-09-23T07:31:29Z

src/components/cvaa-overlay/cvaa-overlay.js

+      <Overlay open onClose={() => props.onClose()} type="cvaa">
+        {this.state.activeWindow === cvaaOverlayState.Main ? (
+          <KeyboardAccessibleMainWindow
+            tabbable="true"


should not be true? or just tabbable

yairans · 2019-09-23T07:35:17Z

src/components/dropdown/dropdown.js

@@ -138,10 +143,15 @@ class DropDown extends Component {
        className={this.state.dropMenuActive ? [style.dropdown, style.active].join(' ') : style.dropdown}
        ref={el => (this._el = el)}>
        <div
-          tabIndex="0"
-          ref={el => (this._dropdownButton = el)}
+          tabIndex={props.tabbable ? '0' : -1}


tabIndex is string or number?

yairans · 2019-09-23T07:40:34Z

src/components/menu/menu.js

+        className={props.isSelected(props.data) ? [style.dropdownMenuItem, style.active].join(' ') : style.dropdownMenuItem}
+        onClick={() => this.onSelect(props)}>
+        <span>{props.data.label}</span>
+        <span className={style.menuIconContainer} style={`opacity: ${props.isSelected(props.data) ? 1 : 0}`}>


handle the opacity by class

yairans · 2019-09-23T07:43:30Z

src/components/smart-container/smart-container.js

 }

-export {SmartContainer};
+const keyboardAccessibleSmartContainer = popupWithKeyboardA11y(SmartContainer);


can't use @with... pattern here?

I am not familiar with @with...

AMAZING!!!! done

yairans · 2019-09-23T07:45:18Z

src/utils/popup-item-keyboard-accessibility.js

+
+/**
+ * @param {BaseComponent} WrappedComponent - The popup item component to implement keyboard accessibility
+ * @returns {BaseComponent} - HOC that handles animation


fix the desc

yairans · 2019-09-23T07:45:55Z

src/utils/popup-keyboard-accessibility.js

+
+/**
+ * @param {BaseComponent} WrappedComponent - The popup component to implement keyboard accessibility
+ * @returns {BaseComponent} - HOC that handles animation


fix the desc

HOC that handles animation ?

In addition in the Oren's change (feat(FEC-9341): refactor base component to HoC #410) there is no BaseComponent any more. change it to Component (and remove/add the proper import)
this is for the whole PR (I see 18 appearances of BaseComponent here)

yairans · 2019-09-23T07:46:09Z

src/utils/popup-keyboard-accessibility.js

+    _element: ?HTMLElement;
+
+    /**
+     * after component mounted, listen to events


fix what? :)

the jsdoc. you don't listen to events here

yairans · 2019-09-24T14:07:06Z

src/components/cast-on-tv/cast-before-play.js

-            <a className={[style.btn, style.btnDarkTransparent, style.castOnTvButton].join(' ')}>
+          <div className={rootStyle.join(' ')}>
+            <a
+              tabIndex="0"


tabIndex is string or number?

html attributes are strings

yairans · 2019-09-24T14:07:22Z

src/components/cast/cast.js

        },
        h('google-cast-launcher', {
-          class: style.castButton
+          class: [style.castButton].join(' '),
+          tabIndex: 0


see my question above

refactored entire code to jsx

yairans · 2019-09-24T14:16:40Z

src/components/smart-container/smart-container.js

 }

-export {SmartContainer};
+const keyboardAccessibleSmartContainer = popupWithKeyboardA11y(SmartContainer);


yairans · 2019-09-24T14:18:14Z

src/utils/popup-item-keyboard-accessibility.js

+     */
+    render(props: any): React$Element<any> | void {
+      return h(
+        'div',


see example here #410

OrenMe · 2019-09-24T13:44:41Z

src/components/cast-on-tv/cast-before-play.js

-          <div className={rootStyle.join(' ')} onClick={() => this.onClick()}>
-            <a className={[style.btn, style.btnDarkTransparent, style.castOnTvButton].join(' ')}>
+          <div className={rootStyle.join(' ')}>
+            <a


if this is a button why not use a button?
Also add aria label

OrenMe · 2019-09-24T15:29:03Z

src/components/cvaa-overlay/cvaa-overlay.js

-        {this.renderMainState()}
-        {this.renderCustomCaptionsState(props)}
-      </Overlay>
+      <div className={props.state.activeWindow === cvaaOverlayState.Main ? [style.overlayScreen, style.active].join(' ') : style.overlayScreen}>


no need for this as you handle it in render already

OrenMe · 2019-09-24T15:30:01Z

src/components/cvaa-overlay/cvaa-overlay.js

+ * @class MainWindow
+ * @extends {Component}
+ */
+class MainWindow extends Component {


Can be a Function component

OrenMe · 2019-09-24T15:30:22Z

src/components/cvaa-overlay/cvaa-overlay.js

+ * @class CustomCaptionsWindow
+ * @extends {Component}
+ */
+class CustomCaptionsWindow extends Component {


can be a Function component

OrenMe · 2019-09-24T15:30:51Z

src/components/cvaa-overlay/cvaa-overlay.js

-      <div className={this.state.state === cvaaOverlayState.CustomCaptions ? [style.overlayScreen, style.active].join(' ') : style.overlayScreen}>
+      <div
+        className={
+          props.state.activeWindow === cvaaOverlayState.CustomCaptions ? [style.overlayScreen, style.active].join(' ') : style.overlayScreen


no need for this, you handle it in render already
Also check if there are non used CSS rules/classes now

yairans · 2019-09-25T12:56:00Z

src/utils/popup-keyboard-accessibility.js

+
+/**
+ * @param {BaseComponent} WrappedComponent - The popup component to implement keyboard accessibility
+ * @returns {BaseComponent} - HOC that handles animation


HOC that handles animation ?

In addition in the Oren's change (feat(FEC-9341): refactor base component to HoC #410) there is no BaseComponent any more. change it to Component (and remove/add the proper import)
this is for the whole PR (I see 18 appearances of BaseComponent here)

yairans · 2019-09-25T12:56:31Z

src/utils/popup-keyboard-accessibility.js

+    _element: ?HTMLElement;
+
+    /**
+     * after component mounted, listen to events


the jsdoc. you don't listen to events here

# Conflicts: # src/components/cast-on-tv/cast-before-play.js # src/components/cast/cast.js # src/components/cvaa-overlay/cvaa-overlay.js # src/components/language/language.js # src/components/playlist-next-screen/playlist-next-screen.js # src/components/settings/settings.js # src/ui-presets/playback.js

RoyBregman · 2019-09-26T09:06:21Z

src/components/smart-container/smart-container.js

-      <div tabIndex="-1" className={[style.smartContainer, style.top, style.left].join(' ')}>
+      <div
+        onKeyDown={e => {
+          props.handleKeyDown(e);


check to remove

RoyBregman added 7 commits September 11, 2019 19:59

feat(FEC-7551): adding missing keyboard accessability

a0b6f31

smartcontainer and menu use the new hocs

828f8cc

adding onClose to keyboard popup item

8930826

tab closes popup and focuses on the next tabindex: 0

a4eea91

test

d10fda5

Revert "test"

d678c6c

This reverts commit d10fda5.

Working on cvaa overlay accessability

d0f6381

RoyBregman requested a review from OrenMe September 19, 2019 09:20

RoyBregman added 2 commits September 19, 2019 17:11

cvaaoverlay handling

cad515b

making pip tab accessible

66d92a3

RoyBregman commented Sep 22, 2019

View reviewed changes

unneeded code

1f81f28

yairans suggested changes Sep 23, 2019

View reviewed changes

RoyBregman added 8 commits September 23, 2019 11:15

Fixing cast buttons and pip accessability

99183f5

Watermark accessability

5b6e249

reverting component reordering

24b606d

playlist countdown and next screen

943ae14

adding jsdocs and refactoring code a bit

7d5364a

fixing bug, removing unneeded code

d71a3da

small fix

7c2572b

refactored menuitem code

f18bce5

OrenMe mentioned this pull request Sep 24, 2019

feat(FEC-7551): adding up down support #401

Closed

yairans suggested changes Sep 24, 2019

View reviewed changes

refactoring popupKeyboard a11y hoc

c4d6062

OrenMe reviewed Sep 25, 2019

View reviewed changes

refactoring

4f4955a

RoyBregman added 3 commits September 25, 2019 14:21

cast buttons refactor

cca9411

changing opacity handling from style to class

dc5a9ef

refactoring to @with decorator

50acd3a

yairans suggested changes Sep 25, 2019

View reviewed changes

RoyBregman commented Sep 26, 2019

View reviewed changes

RoyBregman added 4 commits September 26, 2019 13:18

remove keydown handling in a11y hoc

33ac7e2

remove tabbable from a11y hoc

f16d0f7

using component instead of basecomponent

18258ca

removing _portal ref

e4a47c5

RoyBregman marked this pull request as ready for review September 26, 2019 11:20

RoyBregman force-pushed the FEC-7551-a11y-hoc branch from 0317984 to e4a47c5 Compare September 26, 2019 12:20

RoyBregman added 2 commits September 26, 2019 15:20

fixed comment

04735f1

Merge branch 'master' into FEC-7551-a11y-hoc

94e9c56

yairans merged commit 53698a5 into master Sep 26, 2019

yairans deleted the FEC-7551-a11y-hoc branch September 26, 2019 13:03

feat(FEC-7551): handling keyboard a11y #409

feat(FEC-7551): handling keyboard a11y #409

Conversation

RoyBregman commented Sep 19, 2019

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment