From 8f2a22af9e8f3abf756a36caf7441541e383f3a2 Mon Sep 17 00:00:00 2001
From: Todd Kloots <kloots@twitter.com>
Date: Tue, 9 Jun 2015 10:25:24 -0700
Subject: [PATCH] [fixed] bug where label assertion didn't account for the
 label being an image created by a custom component (resolves #52) [fixed] bug
 where label assertion wouldn't fail when none of the child Components have
 label text (resolves #53)

---
 lib/__tests__/index-test.js | 60 +++++++++++++++++++++++++++----------
 lib/assertions.js           | 10 +++++--
 lib/index.js                | 41 +++++++++++++++----------
 3 files changed, 77 insertions(+), 34 deletions(-)

diff --git a/lib/__tests__/index-test.js b/lib/__tests__/index-test.js
index ba4ce86..000f44e 100644
--- a/lib/__tests__/index-test.js
+++ b/lib/__tests__/index-test.js
@@ -291,7 +291,7 @@ describe('labels', () => {
     });
   });
 
-  it('does not warn when the label text is inside a child component', (done) => {
+  it('does not warn when the label text is inside a child component', () => {
     var Foo = React.createClass({
       render: function() {
         return (
@@ -303,11 +303,39 @@ describe('labels', () => {
     });
 
     doNotExpectWarning(assertions.render.NO_LABEL.msg, () => {
-      React.render(<div role="button"><span><Foo/></span></div>, fixture, done);
+      React.render(<div role="button"><span><Foo/></span></div>, fixture);
     });
   });
 
-  it('does not warn when the label is an image with alt text inside a child component', (done) => {
+  it('does not warn when the label is an image with alt text', () => {
+    var Foo = React.createClass({
+      render: function() {
+        return (
+          <img alt="foo"/>
+        );
+      }
+    });
+
+    doNotExpectWarning(assertions.render.NO_LABEL.msg, () => {
+      React.render(<div role="button"><Foo/></div>, fixture);
+    });
+  });
+
+  it('warns when the label is an image without alt text', () => {
+    var Foo = React.createClass({
+      render: function() {
+        return (
+          <img alt=""/>
+        );
+      }
+    });
+
+    expectWarning(assertions.render.NO_LABEL.msg, () => {
+      React.render(<div role="button"><Foo/></div>, fixture);
+    });
+  });
+
+  it('does not warn when the label is an image with alt text nested inside a child component', () => {
     var Foo = React.createClass({
       render: function() {
         return (
@@ -319,11 +347,11 @@ describe('labels', () => {
     });
 
     doNotExpectWarning(assertions.render.NO_LABEL.msg, () => {
-      React.render(<div role="button"><span><Foo/></span></div>, fixture, done);
+      React.render(<div role="button"><span><Foo/></span></div>, fixture);
     });
   });
 
-  it('warns when a child is a component with image content without alt', (done) => {
+  it('warns when an image without alt text is nested inside a child component', () => {
     var Foo = React.createClass({
       render: function() {
         return (
@@ -335,11 +363,11 @@ describe('labels', () => {
     });
 
     expectWarning(assertions.render.NO_LABEL.msg, () => {
-      React.render(<div role="button"><span><Foo/></span></div>, fixture, done);
+      React.render(<div role="button"><span><Foo/></span></div>, fixture);
     });
   });
 
-  it('does not warn when there is an image without alt text with a sibling text node', (done) => {
+  it('does not warn when there is an image without alt text with a sibling text node', () => {
     var Foo = React.createClass({
       render: function() {
         return (
@@ -351,11 +379,11 @@ describe('labels', () => {
     });
 
     doNotExpectWarning(assertions.render.NO_LABEL.msg, () => {
-      React.render(<div role="button"><span><Foo/></span></div>, fixture, done);
+      React.render(<div role="button"><span><Foo/></span></div>, fixture);
     });
   });
 
-  it('warns when a child is a component without text content', (done) => {
+  it('warns when a child is a component without text content', () => {
     var Bar = React.createClass({
       render: () => {
         return (
@@ -365,11 +393,11 @@ describe('labels', () => {
     });
 
     expectWarning(assertions.render.NO_LABEL.msg, () => {
-      React.render(<div role="button"><Bar/></div>, fixture, done);
+      React.render(<div role="button"><Bar/></div>, fixture);
     });
   });
 
-  it('does not warn as long as one child component has label text', (done) => {
+  it('does not warn as long as one child component has label text', () => {
     var Bar = React.createClass({
       render: () => {
         return (
@@ -389,11 +417,11 @@ describe('labels', () => {
     });
 
     doNotExpectWarning(assertions.render.NO_LABEL.msg, () => {
-      React.render(<div role="button"><Bar/><Foo/></div>, fixture, done);
+      React.render(<div role="button"><Bar/><Foo/></div>, fixture);
     });
   });
 
-  it('warns if no child components have label text', (done) => {
+  it('warns if no child components have label text', () => {
     var Bar = React.createClass({
       render: () => {
         return (
@@ -411,12 +439,12 @@ describe('labels', () => {
     });
 
     expectWarning(assertions.render.NO_LABEL.msg, () => {
-      React.render(<div role="button"><Bar/><Foo/></div>, fixture, done);
+      React.render(<div role="button"><Bar/><div /><Foo/></div>, fixture);
     });
   });
 
 
-  it('does not error when the component has a componentDidMount callback', (done) => {
+  it('does not error when the component has a componentDidMount callback', () => {
     var Bar = React.createClass({
       _privateProp: 'bar',
 
@@ -431,7 +459,7 @@ describe('labels', () => {
     });
 
     expectWarning(assertions.render.NO_LABEL.msg, () => {
-      React.render(<div role="button"><Bar/></div>, fixture, done);
+      React.render(<div role="button"><Bar/></div>, fixture);
     });
   });
 
diff --git a/lib/assertions.js b/lib/assertions.js
index c0dbe1f..764c45d 100644
--- a/lib/assertions.js
+++ b/lib/assertions.js
@@ -42,7 +42,9 @@ var getComponents = (children) => {
 };
 
 var hasLabel = (node) => {
-  var hasTextContent = node.textContent.trim().length > 0;
+  var text = node.tagName.toLowerCase() == 'img' ? node.alt : node.textContent;
+  var hasTextContent = text.trim().length > 0;
+
   var images = node.querySelectorAll('img[alt]');
   images = Array.prototype.slice.call(images);
 
@@ -66,7 +68,9 @@ var assertLabel = function(node, context, failureCB) {
 var hasChildTextNode = (props, children, failureCB) => {
   var hasText = false;
   var childComponents = getComponents(children);
-  var hasChildComponents = childComponents.length > 0;
+  var nChildComponents = childComponents.length;
+  var hasChildComponents = nChildComponents > 0;
+  var nCurrentComponent = 0;
   var context;
 
   if (hasChildComponents)
@@ -97,7 +101,7 @@ var hasChildTextNode = (props, children, failureCB) => {
       // Return true because the label check is now going to be async
       // (due to the componentDidMount listener) and we want to avoid
       // pre-maturely calling the failure callback.
-      hasText = true;
+      hasText = (nChildComponents == ++nCurrentComponent);
     }
   });
   return hasText;
diff --git a/lib/index.js b/lib/index.js
index afbd19e..de5df9f 100644
--- a/lib/index.js
+++ b/lib/index.js
@@ -18,7 +18,7 @@ var runTagTests = (tagName, props, children, deviceFilter, onFailure) => {
       !tagTests[key].test(tagName, props, children);
 
     if (tagTests[key] && testFailed)
-      onFailure(tagName, props, children, tagTests[key].msg);
+      onFailure(tagName, props, tagTests[key].msg);
    }
 };
 
@@ -37,7 +37,7 @@ var runPropTests = (tagName, props, children, deviceFilter, onFailure) => {
         !propTests[key].test(tagName, props, children);
 
       if (propTests[key] && testTailed)
-        onFailure(tagName, props, children, propTests[key].msg);
+        onFailure(tagName, props, propTests[key].msg);
     }
   }
 };
@@ -49,7 +49,7 @@ var runLabelTests = (tagName, props, children, deviceFilter, onFailure) => {
   for (key in renderTests) {
     if (renderTests[key]) {
       let failureCB = onFailure.bind(
-        undefined, tagName, props, children, renderTests[key].msg);
+        undefined, tagName, props, renderTests[key].msg);
 
       renderTests[key].test(tagName, props, children, failureCB);
     }
@@ -66,7 +66,7 @@ var runTests = (tagName, props, children, deviceFilter, onFailure) => {
 var shouldShowError = (failureInfo, options) => {
   var filterFn = options.filterFn;
   if (filterFn)
-    return filterFn(failureInfo.name, failureInfo.id);
+    return filterFn(failureInfo.tagName, failureInfo.id);
 
   return true;
 };
@@ -75,7 +75,7 @@ var throwError = (failureInfo, options) => {
   if (!shouldShowError(failureInfo, options))
     return;
 
-  var error = [failureInfo.name, failureInfo.failure];
+  var error = [failureInfo.tagName, failureInfo.msg];
 
   if (options.includeSrcNode)
     error.push(failureInfo.id);
@@ -95,25 +95,36 @@ var logWarning = (component, failureInfo, options) => {
     if (!shouldShowError(failureInfo, options))
       return;
 
-    var warning = [failureInfo.name, failureInfo.failure];
-
-    if (includeSrcNode)
-      warning.push(document.getElementById(failureInfo.id));
+    var warning = [failureInfo.tagName, failureInfo.msg];
+
+    if (includeSrcNode && component) {
+      // TODO:
+      // 1) Consider using React.findDOMNode() over document.getElementById
+      //    https://github.com/rackt/react-a11y/issues/54
+      // 2) Consider using ref to expand element element reference logging
+      //    to all element (https://github.com/rackt/react-a11y/issues/55)
+      let srcNode = document.getElementById(failureInfo.id);
+
+      // Guard against logging null element references should render()
+      // return null or false.
+      // https://facebook.github.io/react/docs/component-api.html#getdomnode
+      if (srcNode)
+        warning.push(srcNode);
+    }
 
     console.warn.apply(console, warning);
   };
 
-  if (component && includeSrcNode) {
+  if (includeSrcNode && component)
     // Cannot log a node reference until the component is in the DOM,
     // so defer the document.getElementById call until componentDidMount
     // or componentDidUpdate.
     logAfterRender(component._instance, warn);
-  } else {
+  else
     warn();
-  }
 };
 
-var handleFailure = (options, reactEl, type, props, children, failure) => {
+var handleFailure = (options, reactEl, type, props, failureMsg) => {
   var includeSrcNode = options && !!options.includeSrcNode;
   var reactComponent = reactEl._owner;
 
@@ -123,9 +134,9 @@ var handleFailure = (options, reactEl, type, props, children, failure) => {
     type + '#' + props.id;
 
   var failureInfo = {
-    'name': name ,
+    'tagName': name ,
     'id': props.id,
-    'failure': failure
+    'msg': failureMsg
   };
 
   var notifyOpts = {