Skip to content

Commit

Permalink
[fixed] bug where label assertion didn't account for the label being …
Browse files Browse the repository at this point in the history 
…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)
  • Loading branch information
Todd Kloots committed Jun 9, 2015
1 parent e6b3df5 commit 8f2a22a
Show file tree
Hide file tree
Showing 3 changed files with 77 additions and 34 deletions.
60 changes: 44 additions & 16 deletions lib/__tests__/index-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand All @@ -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 (
Expand All @@ -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 (
Expand All @@ -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 (
Expand All @@ -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 (
Expand All @@ -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 (
Expand All @@ -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 (
Expand All @@ -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',

Expand All @@ -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);
});
});

Expand Down
10 changes: 7 additions & 3 deletions lib/assertions.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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)
Expand Down Expand Up @@ -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;
Expand Down
41 changes: 26 additions & 15 deletions lib/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
};

Expand All @@ -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);
}
}
};
Expand All @@ -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);
}
Expand All @@ -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;
};
Expand All @@ -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);
Expand All @@ -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;

Expand All @@ -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 = {
Expand Down

0 comments on commit 8f2a22a

Please sign in to comment.