From 458c534200cc65aab200ff77e987aec9c1a3e1a8 Mon Sep 17 00:00:00 2001
From: Nicolas Gallagher <nicolasgallagher@gmail.com>
Date: Mon, 20 Mar 2017 14:46:09 -0700
Subject: [PATCH] [change] improve button accessibility and styling

1. If no 'accessibilityRole' is set, fallback to looking for 'button'
role in the equivalent native props. This helps improve accessibility of
button-like components authored without the web platform in mind.

2. Ensure button context is properly inherited.

3. Add 'appearance:none' to DOM button elements to enable better styling
support in Safari

Fix #378
---
 .../renderApplication-test.js.snap            |  1 +
 .../__snapshots__/index-test.js.snap          |  3 +-
 src/components/View/index.js                  | 21 ++++++++----
 .../__snapshots__/index-test.js.snap          | 22 +++++++++++++
 .../createDOMElement/__tests__/index-test.js  | 29 +++++++++++++++++
 src/modules/createDOMElement/index.js         | 32 +++++++++++--------
 .../__tests__/index-test.js                   | 28 ++++++++++++++++
 src/modules/getAccessibilityRole/index.js     | 15 +++++++++
 8 files changed, 131 insertions(+), 20 deletions(-)
 create mode 100644 src/modules/getAccessibilityRole/__tests__/index-test.js
 create mode 100644 src/modules/getAccessibilityRole/index.js

diff --git a/src/apis/AppRegistry/__tests__/__snapshots__/renderApplication-test.js.snap b/src/apis/AppRegistry/__tests__/__snapshots__/renderApplication-test.js.snap
index 5ab83d9e..649ffddf 100644
--- a/src/apis/AppRegistry/__tests__/__snapshots__/renderApplication-test.js.snap
+++ b/src/apis/AppRegistry/__tests__/__snapshots__/renderApplication-test.js.snap
@@ -50,5 +50,6 @@ input::-webkit-inner-spin-button,input::-webkit-outer-spin-button,input::-webkit
 .rn-paddingLeft-gy4na3{padding-left:0px}
 .rn-textAlign-1ttztb7{text-align:inherit}
 .rn-textDecoration-bauka4{text-decoration:none}
+.rn-appearance-30o5oe{-moz-appearance:none;-webkit-appearance:none;appearance:none}
 </style>"
 `;
diff --git a/src/components/View/__tests__/__snapshots__/index-test.js.snap b/src/components/View/__tests__/__snapshots__/index-test.js.snap
index 11e5f418..a306af59 100644
--- a/src/components/View/__tests__/__snapshots__/index-test.js.snap
+++ b/src/components/View/__tests__/__snapshots__/index-test.js.snap
@@ -20,8 +20,9 @@ exports[`components/View rendered element is a "div" by default 1`] = `
 
 exports[`components/View rendered element is a "span" when inside <View accessibilityRole="button" /> 1`] = `
 <button
-  className="rn-alignItems-1oszu61 rn-backgroundColor-wib322 rn-borderTopStyle-1efd50x rn-borderRightStyle-14skgim rn-borderBottomStyle-rull8r rn-borderLeftStyle-mm0ijv rn-borderTopWidth-13yce4e rn-borderRightWidth-fnigne rn-borderBottomWidth-ndvcnb rn-borderLeftWidth-gxnn5r rn-boxSizing-deolkf rn-color-homxoj rn-display-6koalj rn-flexShrink-1qe8dj5 rn-flexBasis-1mlwlqe rn-flexDirection-eqz5dr rn-font-1lw9tu2 rn-listStyle-1ebb2ja rn-marginTop-1mnahxq rn-marginRight-61z16t rn-marginBottom-p1pxzi rn-marginLeft-11wrixw rn-minHeight-ifefl9 rn-minWidth-bcqeeo rn-paddingTop-wk8lta rn-paddingRight-9aemit rn-paddingBottom-1mdbw0j rn-paddingLeft-gy4na3 rn-position-bnwqim rn-textAlign-1ttztb7 rn-textDecoration-bauka4"
+  className="rn-alignItems-1oszu61 rn-appearance-30o5oe rn-backgroundColor-wib322 rn-borderTopStyle-1efd50x rn-borderRightStyle-14skgim rn-borderBottomStyle-rull8r rn-borderLeftStyle-mm0ijv rn-borderTopWidth-13yce4e rn-borderRightWidth-fnigne rn-borderBottomWidth-ndvcnb rn-borderLeftWidth-gxnn5r rn-boxSizing-deolkf rn-color-homxoj rn-display-6koalj rn-flexShrink-1qe8dj5 rn-flexBasis-1mlwlqe rn-flexDirection-eqz5dr rn-font-1lw9tu2 rn-listStyle-1ebb2ja rn-marginTop-1mnahxq rn-marginRight-61z16t rn-marginBottom-p1pxzi rn-marginLeft-11wrixw rn-minHeight-ifefl9 rn-minWidth-bcqeeo rn-paddingTop-wk8lta rn-paddingRight-9aemit rn-paddingBottom-1mdbw0j rn-paddingLeft-gy4na3 rn-position-bnwqim rn-textAlign-1ttztb7 rn-textDecoration-bauka4"
   role="button"
+  style={Object {}}
   type="button">
   <span
     className="rn-alignItems-1oszu61 rn-backgroundColor-wib322 rn-borderTopStyle-1efd50x rn-borderRightStyle-14skgim rn-borderBottomStyle-rull8r rn-borderLeftStyle-mm0ijv rn-borderTopWidth-13yce4e rn-borderRightWidth-fnigne rn-borderBottomWidth-ndvcnb rn-borderLeftWidth-gxnn5r rn-boxSizing-deolkf rn-color-homxoj rn-display-6koalj rn-flexShrink-1qe8dj5 rn-flexBasis-1mlwlqe rn-flexDirection-eqz5dr rn-font-1lw9tu2 rn-listStyle-1ebb2ja rn-marginTop-1mnahxq rn-marginRight-61z16t rn-marginBottom-p1pxzi rn-marginLeft-11wrixw rn-minHeight-ifefl9 rn-minWidth-bcqeeo rn-paddingTop-wk8lta rn-paddingRight-9aemit rn-paddingBottom-1mdbw0j rn-paddingLeft-gy4na3 rn-position-bnwqim rn-textAlign-1ttztb7 rn-textDecoration-bauka4" />
diff --git a/src/components/View/index.js b/src/components/View/index.js
index 90983d51..b1e9148a 100644
--- a/src/components/View/index.js
+++ b/src/components/View/index.js
@@ -1,6 +1,7 @@
 import applyLayout from '../../modules/applyLayout';
 import applyNativeMethods from '../../modules/applyNativeMethods';
 import createDOMElement from '../../modules/createDOMElement';
+import getAccessibilityRole from '../../modules/getAccessibilityRole';
 import StyleSheet from '../../apis/StyleSheet';
 import ViewPropTypes from './ViewPropTypes';
 import { Component, PropTypes } from 'react';
@@ -25,7 +26,7 @@ class View extends Component {
   };
 
   getChildContext() {
-    const isInAButtonView = this.props.accessibilityRole === 'button' ||
+    const isInAButtonView = getAccessibilityRole(this.props) === 'button' ||
       this.context.isInAButtonView;
     return isInAButtonView ? { isInAButtonView } : emptyObject;
   }
@@ -35,8 +36,6 @@ class View extends Component {
       pointerEvents,
       style,
       /* eslint-disable */
-      accessibilityComponentType,
-      accessibilityTraits,
       collapsable,
       hitSlop,
       onAccessibilityTap,
@@ -47,10 +46,17 @@ class View extends Component {
       ...otherProps
     } = this.props;
 
-    const component = this.context.isInAButtonView ? 'span' : 'div';
+    const { isInAButtonView } = this.context;
+    const isButton = getAccessibilityRole(this.props) === 'button';
 
-    otherProps.style = [styles.initial, style, pointerEvents && pointerEventStyles[pointerEvents]];
+    otherProps.style = [
+      styles.initial,
+      isButton && styles.buttonOnly,
+      style,
+      pointerEvents && pointerEventStyles[pointerEvents]
+    ];
 
+    const component = isInAButtonView ? 'span' : 'div';
     return createDOMElement(component, otherProps);
   }
 }
@@ -68,7 +74,7 @@ const styles = StyleSheet.create({
     margin: 0,
     padding: 0,
     position: 'relative',
-    // button and anchor reset
+    // button and anchor resets
     backgroundColor: 'transparent',
     color: 'inherit',
     font: 'inherit',
@@ -79,6 +85,9 @@ const styles = StyleSheet.create({
     // fix flexbox bugs
     minHeight: 0,
     minWidth: 0
+  },
+  buttonOnly: {
+    appearance: 'none'
   }
 });
 
diff --git a/src/modules/createDOMElement/__tests__/__snapshots__/index-test.js.snap b/src/modules/createDOMElement/__tests__/__snapshots__/index-test.js.snap
index 2a0a6776..2d995fed 100644
--- a/src/modules/createDOMElement/__tests__/__snapshots__/index-test.js.snap
+++ b/src/modules/createDOMElement/__tests__/__snapshots__/index-test.js.snap
@@ -14,6 +14,28 @@ exports[`modules/createDOMElement prop "accessibilityRole" button 1`] = `
   type="button" />
 `;
 
+exports[`modules/createDOMElement prop "accessibilityRole" compatibility with accessibilityComponentType 1`] = `
+<button
+  role="button"
+  type="button" />
+`;
+
+exports[`modules/createDOMElement prop "accessibilityRole" compatibility with accessibilityComponentType 2`] = `
+<a
+  role="link" />
+`;
+
+exports[`modules/createDOMElement prop "accessibilityRole" compatibility with accessibilityTraits 1`] = `
+<button
+  role="button"
+  type="button" />
+`;
+
+exports[`modules/createDOMElement prop "accessibilityRole" compatibility with accessibilityTraits 2`] = `
+<a
+  role="link" />
+`;
+
 exports[`modules/createDOMElement prop "accessibilityRole" link and target="_blank" 1`] = `
 <a
   rel=" noopener noreferrer"
diff --git a/src/modules/createDOMElement/__tests__/index-test.js b/src/modules/createDOMElement/__tests__/index-test.js
index 594298d3..ba5140ac 100644
--- a/src/modules/createDOMElement/__tests__/index-test.js
+++ b/src/modules/createDOMElement/__tests__/index-test.js
@@ -43,6 +43,35 @@ describe('modules/createDOMElement', () => {
       );
       expect(component.toJSON()).toMatchSnapshot();
     });
+
+    describe('compatibility with', () => {
+      test('accessibilityComponentType', () => {
+        let component = renderer.create(
+          createDOMElement('span', { accessibilityComponentType: 'button' })
+        );
+        expect(component.toJSON()).toMatchSnapshot();
+
+        component = renderer.create(
+          createDOMElement('span', {
+            accessibilityComponentType: 'button',
+            accessibilityRole: 'link'
+          })
+        );
+        expect(component.toJSON()).toMatchSnapshot();
+      });
+
+      test('accessibilityTraits', () => {
+        let component = renderer.create(
+          createDOMElement('span', { accessibilityTraits: 'button' })
+        );
+        expect(component.toJSON()).toMatchSnapshot();
+
+        component = renderer.create(
+          createDOMElement('span', { accessibilityTraits: 'button', accessibilityRole: 'link' })
+        );
+        expect(component.toJSON()).toMatchSnapshot();
+      });
+    });
   });
 
   test('prop "accessible"', () => {
diff --git a/src/modules/createDOMElement/index.js b/src/modules/createDOMElement/index.js
index 049bba94..94639992 100644
--- a/src/modules/createDOMElement/index.js
+++ b/src/modules/createDOMElement/index.js
@@ -1,5 +1,6 @@
 import '../injectResponderEventPlugin';
 
+import getAccessibilityRole from '../getAccessibilityRole';
 import normalizeNativeEvent from '../normalizeNativeEvent';
 import React from 'react';
 import StyleRegistry from '../../apis/StyleSheet/registry';
@@ -54,16 +55,21 @@ const createDOMElement = (component, rnProps) => {
   const {
     accessibilityLabel,
     accessibilityLiveRegion,
-    accessibilityRole,
     accessible = true,
     style: rnStyle,
     testID,
     type,
+    /* eslint-disable */
+    accessibilityComponentType,
+    accessibilityRole,
+    accessibilityTraits,
+    /* eslint-enable */
     ...domProps
   } = rnProps || emptyObject;
 
   // use equivalent platform elements where possible
-  const accessibilityComponent = accessibilityRole && roleComponents[accessibilityRole];
+  const role = getAccessibilityRole(rnProps || emptyObject);
+  const accessibilityComponent = role && roleComponents[role];
   const Component = accessibilityComponent || component;
 
   // convert React Native styles to DOM styles
@@ -89,23 +95,23 @@ const createDOMElement = (component, rnProps) => {
   if (accessibilityLiveRegion) {
     domProps['aria-live'] = accessibilityLiveRegion;
   }
-  if (testID) {
-    domProps['data-testid'] = testID;
-  }
-  if (accessibilityRole) {
-    domProps.role = accessibilityRole;
-    if (accessibilityRole === 'button') {
-      domProps.type = 'button';
-    } else if (accessibilityRole === 'link' && domProps.target === '_blank') {
-      domProps.rel = `${domProps.rel || ''} noopener noreferrer`;
-    }
-  }
   if (className && className !== '') {
     domProps.className = domProps.className ? `${domProps.className} ${className}` : className;
   }
+  if (role) {
+    domProps.role = role;
+    if (role === 'button') {
+      domProps.type = 'button';
+    } else if (role === 'link' && domProps.target === '_blank') {
+      domProps.rel = `${domProps.rel || ''} noopener noreferrer`;
+    }
+  }
   if (style) {
     domProps.style = style;
   }
+  if (testID) {
+    domProps['data-testid'] = testID;
+  }
   if (type) {
     domProps.type = type;
   }
diff --git a/src/modules/getAccessibilityRole/__tests__/index-test.js b/src/modules/getAccessibilityRole/__tests__/index-test.js
new file mode 100644
index 00000000..13360b5c
--- /dev/null
+++ b/src/modules/getAccessibilityRole/__tests__/index-test.js
@@ -0,0 +1,28 @@
+/* eslint-env jasmine, jest */
+
+import getAccessibilityRole from '..';
+
+describe('modules/getAccessibilityRole', () => {
+  test('returns undefined when missing accessibility props', () => {
+    expect(getAccessibilityRole({})).toBeUndefined();
+  });
+
+  test('returns value of "accessibilityRole" when defined', () => {
+    expect(getAccessibilityRole({ accessibilityRole: 'banner' })).toEqual('banner');
+  });
+
+  test('returns "button" when iOS/Android accessibility prop equals "button"', () => {
+    expect(getAccessibilityRole({ accessibilityComponentType: 'button' })).toEqual('button');
+    expect(getAccessibilityRole({ accessibilityTraits: 'button' })).toEqual('button');
+  });
+
+  test('prioritizes "accessibilityRole" when defined', () => {
+    expect(
+      getAccessibilityRole({
+        accessibilityComponentType: 'button',
+        accessibilityRole: 'link',
+        accessibilityTraits: 'button'
+      })
+    ).toEqual('link');
+  });
+});
diff --git a/src/modules/getAccessibilityRole/index.js b/src/modules/getAccessibilityRole/index.js
new file mode 100644
index 00000000..490e31f9
--- /dev/null
+++ b/src/modules/getAccessibilityRole/index.js
@@ -0,0 +1,15 @@
+const getAccessibilityRole = (
+  {
+    accessibilityComponentType,
+    accessibilityRole,
+    accessibilityTraits
+  }
+) => {
+  if (accessibilityRole) {
+    return accessibilityRole;
+  } else if (accessibilityComponentType === 'button' || accessibilityTraits === 'button') {
+    return 'button';
+  }
+};
+
+module.exports = getAccessibilityRole;