From 2c6fe780123e2cdffdb65961f282c7afb8e06156 Mon Sep 17 00:00:00 2001
From: Michael Telatynski <7t3chguy@gmail.com>
Date: Wed, 22 Jan 2020 10:36:20 +0000
Subject: [PATCH 1/4] Fix roving room list for resizer and ff tabstop a11y

---
 src/accessibility/RovingTabIndex.js           | 70 ++++++++-----------
 src/components/structures/RoomSubList.js      |  4 --
 .../views/groups/GroupInviteTile.js           |  3 +-
 src/components/views/rooms/RoomList.js        | 27 +++----
 src/components/views/rooms/RoomTile.js        |  3 +-
 5 files changed, 48 insertions(+), 59 deletions(-)

diff --git a/src/accessibility/RovingTabIndex.js b/src/accessibility/RovingTabIndex.js
index 8924815f23..38f2594baf 100644
--- a/src/accessibility/RovingTabIndex.js
+++ b/src/accessibility/RovingTabIndex.js
@@ -129,7 +129,7 @@ const reducer = (state, action) => {
     }
 };
 
-export const RovingTabIndexProvider = ({children, handleHomeEnd}) => {
+export const RovingTabIndexProvider = ({children, handleHomeEnd, onKeyDown}) => {
     const [state, dispatch] = useReducer(reducer, {
         activeRef: null,
         refs: [],
@@ -137,53 +137,43 @@ export const RovingTabIndexProvider = ({children, handleHomeEnd}) => {
 
     const context = useMemo(() => ({state, dispatch}), [state]);
 
-    if (handleHomeEnd) {
-        return <RovingTabIndexContext.Provider value={context}>
-            <HomeEndHelper>
-                { children }
-            </HomeEndHelper>
-        </RovingTabIndexContext.Provider>;
-    }
-
-    return <RovingTabIndexContext.Provider value={context}>
-        { children }
-    </RovingTabIndexContext.Provider>;
-};
-RovingTabIndexProvider.propTypes = {
-    handleHomeEnd: PropTypes.bool,
-};
-
-// Helper to handle Home/End to jump to first/last roving-tab-index for widgets such as treeview
-export const HomeEndHelper = ({children}) => {
-    const context = useContext(RovingTabIndexContext);
-
-    const onKeyDown = useCallback((ev) => {
-        // check if we actually have any items
-        if (context.state.refs.length <= 0) return;
-
-        let handled = true;
-        switch (ev.key) {
-            case Key.HOME:
-                // move focus to first item
-                context.state.refs[0].current.focus();
-                break;
-            case Key.END:
-                // move focus to last item
-                context.state.refs[context.state.refs.length - 1].current.focus();
-                break;
-            default:
-                handled = false;
+    const onKeyDownHandler = useCallback((ev) => {
+        let handled = false;
+        if (handleHomeEnd) {
+            // check if we actually have any items
+            switch (ev.key) {
+                case Key.HOME:
+                    handled = true;
+                    // move focus to first item
+                    if (context.state.refs.length > 0) {
+                        context.state.refs[0].current.focus();
+                    }
+                    break;
+                case Key.END:
+                    handled = true;
+                    // move focus to last item
+                    if (context.state.refs.length > 0) {
+                        context.state.refs[context.state.refs.length - 1].current.focus();
+                    }
+                    break;
+            }
         }
 
         if (handled) {
             ev.preventDefault();
             ev.stopPropagation();
+        } else if (onKeyDown) {
+            return onKeyDown(ev);
         }
     }, [context.state]);
 
-    return <div onKeyDown={onKeyDown}>
-        { children }
-    </div>;
+    return <RovingTabIndexContext.Provider value={context}>
+        { children({onKeyDownHandler}) }
+    </RovingTabIndexContext.Provider>;
+};
+RovingTabIndexProvider.propTypes = {
+    handleHomeEnd: PropTypes.bool,
+    onKeyDown: PropTypes.func,
 };
 
 // Hook to register a roving tab index
diff --git a/src/components/structures/RoomSubList.js b/src/components/structures/RoomSubList.js
index 2d41abf902..600b418fe0 100644
--- a/src/components/structures/RoomSubList.js
+++ b/src/components/structures/RoomSubList.js
@@ -142,10 +142,6 @@ export default class RoomSubList extends React.PureComponent {
 
     onHeaderKeyDown = (ev) => {
         switch (ev.key) {
-            case Key.TAB:
-                // Prevent LeftPanel handling Tab if focus is on the sublist header itself
-                ev.stopPropagation();
-                break;
             case Key.ARROW_LEFT:
                 // On ARROW_LEFT collapse the room sublist
                 if (!this.state.hidden && !this.props.forceExpand) {
diff --git a/src/components/views/groups/GroupInviteTile.js b/src/components/views/groups/GroupInviteTile.js
index 3b15c6ff41..91c930525d 100644
--- a/src/components/views/groups/GroupInviteTile.js
+++ b/src/components/views/groups/GroupInviteTile.js
@@ -128,7 +128,8 @@ export default createReactClass({
             'mx_RoomTile_badgeShown': this.state.badgeHover || isMenuDisplayed,
         });
 
-        const label = <div title={this.props.group.groupId} className={nameClasses} dir="auto">
+        // XXX: this is a workaround for Firefox giving this div a tabstop :( [tabIndex]
+        const label = <div title={this.props.group.groupId} className={nameClasses} tabIndex={-1} dir="auto">
             { groupName }
         </div>;
 
diff --git a/src/components/views/rooms/RoomList.js b/src/components/views/rooms/RoomList.js
index bd563b2f28..ee3100b535 100644
--- a/src/components/views/rooms/RoomList.js
+++ b/src/components/views/rooms/RoomList.js
@@ -777,21 +777,22 @@ export default createReactClass({
 
         const subListComponents = this._mapSubListProps(subLists);
 
-        const {resizeNotifier, collapsed, searchFilter, ConferenceHandler, ...props} = this.props; // eslint-disable-line
+        const {resizeNotifier, collapsed, searchFilter, ConferenceHandler, onKeyDown, ...props} = this.props; // eslint-disable-line
         return (
-            <div
-                {...props}
-                ref={this._collectResizeContainer}
-                className="mx_RoomList"
-                role="tree"
-                aria-label={_t("Rooms")}
-                onMouseMove={this.onMouseMove}
-                onMouseLeave={this.onMouseLeave}
-            >
-                <RovingTabIndexProvider handleHomeEnd={true}>
+            <RovingTabIndexProvider handleHomeEnd={true} onKeyDown={onKeyDown}>
+                {({onKeyDownHandler}) => <div
+                    {...props}
+                    onKeyDown={onKeyDownHandler}
+                    ref={this._collectResizeContainer}
+                    className="mx_RoomList"
+                    role="tree"
+                    aria-label={_t("Rooms")}
+                    onMouseMove={this.onMouseMove}
+                    onMouseLeave={this.onMouseLeave}
+                >
                     { subListComponents }
-                </RovingTabIndexProvider>
-            </div>
+                </div> }
+            </RovingTabIndexProvider>
         );
     },
 });
diff --git a/src/components/views/rooms/RoomTile.js b/src/components/views/rooms/RoomTile.js
index 3b13001225..f4f5fa10fc 100644
--- a/src/components/views/rooms/RoomTile.js
+++ b/src/components/views/rooms/RoomTile.js
@@ -353,7 +353,8 @@ export default createReactClass({
             });
 
             subtextLabel = subtext ? <span className="mx_RoomTile_subtext">{ subtext }</span> : null;
-            label = <div title={name} className={nameClasses} dir="auto">{ name }</div>;
+            // XXX: this is a workaround for Firefox giving this div a tabstop :( [tabIndex]
+            label = <div title={name} className={nameClasses} tabIndex={-1} dir="auto">{ name }</div>;
         } else if (this.state.hover) {
             const Tooltip = sdk.getComponent("elements.Tooltip");
             tooltip = <Tooltip className="mx_RoomTile_tooltip" label={this.props.room.name} dir="auto" />;

From 37fb500e22aeb4762cef90ca8ca4f28eead57fa6 Mon Sep 17 00:00:00 2001
From: Michael Telatynski <7t3chguy@gmail.com>
Date: Wed, 22 Jan 2020 10:41:10 +0000
Subject: [PATCH 2/4] fix useCallback dependencies, delint

---
 src/accessibility/RovingTabIndex.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/accessibility/RovingTabIndex.js b/src/accessibility/RovingTabIndex.js
index 38f2594baf..b481f08fe2 100644
--- a/src/accessibility/RovingTabIndex.js
+++ b/src/accessibility/RovingTabIndex.js
@@ -165,7 +165,7 @@ export const RovingTabIndexProvider = ({children, handleHomeEnd, onKeyDown}) =>
         } else if (onKeyDown) {
             return onKeyDown(ev);
         }
-    }, [context.state]);
+    }, [context.state, onKeyDown, handleHomeEnd]);
 
     return <RovingTabIndexContext.Provider value={context}>
         { children({onKeyDownHandler}) }

From 176605c302698d7146380624bd82696ababde64f Mon Sep 17 00:00:00 2001
From: Michael Telatynski <7t3chguy@gmail.com>
Date: Wed, 22 Jan 2020 10:49:58 +0000
Subject: [PATCH 3/4] update tests to match new rendering method

---
 test/accessibility/RovingTabIndex-test.js | 26 +++++++++++++----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/test/accessibility/RovingTabIndex-test.js b/test/accessibility/RovingTabIndex-test.js
index 2b55d1420c..e7d7a0977d 100644
--- a/test/accessibility/RovingTabIndex-test.js
+++ b/test/accessibility/RovingTabIndex-test.js
@@ -47,7 +47,7 @@ const button4 = <Button key={4}>d</Button>;
 describe("RovingTabIndex", () => {
     it("RovingTabIndexProvider renders children as expected", () => {
         const wrapper = mount(<RovingTabIndexProvider>
-            <div><span>Test</span></div>
+            {() => <div><span>Test</span></div> }
         </RovingTabIndexProvider>);
         expect(wrapper.text()).toBe("Test");
         expect(wrapper.html()).toBe('<div><span>Test</span></div>');
@@ -55,9 +55,11 @@ describe("RovingTabIndex", () => {
 
     it("RovingTabIndexProvider works as expected with useRovingTabIndex", () => {
         const wrapper = mount(<RovingTabIndexProvider>
-            { button1 }
-            { button2 }
-            { button3 }
+            {() => <React.Fragment>
+                { button1 }
+                { button2 }
+                { button3 }
+            </React.Fragment>}
         </RovingTabIndexProvider>);
 
         // should begin with 0th being active
@@ -95,13 +97,15 @@ describe("RovingTabIndex", () => {
 
     it("RovingTabIndexProvider works as expected with RovingTabIndexWrapper", () => {
         const wrapper = mount(<RovingTabIndexProvider>
-            { button1 }
-            { button2 }
-            <RovingTabIndexWrapper>
-                {({onFocus, isActive, ref}) =>
-                    <button onFocus={onFocus} tabIndex={isActive ? 0 : -1} ref={ref}>.</button>
-                }
-            </RovingTabIndexWrapper>
+            {() => <React.Fragment>
+                { button1 }
+                { button2 }
+                <RovingTabIndexWrapper>
+                    {({onFocus, isActive, ref}) =>
+                        <button onFocus={onFocus} tabIndex={isActive ? 0 : -1} ref={ref}>.</button>
+                    }
+                </RovingTabIndexWrapper>
+            </React.Fragment>}
         </RovingTabIndexProvider>);
 
         // should begin with 0th being active

From fc724cfe709ec046db13283e2fd8bf91646bb017 Mon Sep 17 00:00:00 2001
From: Michael Telatynski <7t3chguy@gmail.com>
Date: Wed, 22 Jan 2020 11:05:25 +0000
Subject: [PATCH 4/4] fix tests some moar

---
 test/accessibility/RovingTabIndex-test.js | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/test/accessibility/RovingTabIndex-test.js b/test/accessibility/RovingTabIndex-test.js
index e7d7a0977d..8be4a2976c 100644
--- a/test/accessibility/RovingTabIndex-test.js
+++ b/test/accessibility/RovingTabIndex-test.js
@@ -47,7 +47,7 @@ const button4 = <Button key={4}>d</Button>;
 describe("RovingTabIndex", () => {
     it("RovingTabIndexProvider renders children as expected", () => {
         const wrapper = mount(<RovingTabIndexProvider>
-            {() => <div><span>Test</span></div> }
+            {() => <div><span>Test</span></div>}
         </RovingTabIndexProvider>);
         expect(wrapper.text()).toBe("Test");
         expect(wrapper.html()).toBe('<div><span>Test</span></div>');
@@ -82,14 +82,14 @@ describe("RovingTabIndex", () => {
 
         // update the children, it should remain on the same button
         wrapper.setProps({
-            children: [button1, button4, button2, button3],
+            children: () => [button1, button4, button2, button3],
         });
         wrapper.update();
         checkTabIndexes(wrapper.find("button"), [-1, -1, 0, -1]);
 
         // update the children, remove the active button, it should move to the next one
         wrapper.setProps({
-            children: [button1, button4, button3],
+            children: () => [button1, button4, button3],
         });
         wrapper.update();
         checkTabIndexes(wrapper.find("button"), [-1, -1, 0]);