Label assertion fails when child is a Component

[kloots]

We have tests covering the case where an element's label is a deeply nested text node. For example:

it('does not warn if there are deeply nested text node children', () => {
  doNotExpectWarning(assertions.props.onClick.NO_LABEL.msg, () => {
    <div onClick={k}><span><span>foo</span></span></div>;
  });
});

However, if an element's label is a text node nested inside a Component, the label test fails. For example:

it('does not warn if there are deeply nested text node children inside of a component', () => {
  var Foo = React.createClass({
    render: function() {
      return (
        <div className="foo">
          <span><span>foo</span></span>
        </div>
      );
    }
  });

  doNotExpectWarning(assertions.props.onClick.NO_LABEL.msg, () => {
    <div role="gridcell" onClick={k}>
      <Foo/>
    </div>;
  });
});

Another, more simplified example that also fails:

it('warns if a Component has no label of any sort', () => {
  var Foo = React.createClass({
    render: function() {
      return (
        <div className="foo"></div>
      );
    }
  });

  expectWarning(assertions.props.onClick.NO_LABEL.msg, () => {
    <Foo onClick={k}/>;
  });
});

[kloots]

Looked into this problem yesterday. The short of it is I don't think we can rely on virtual DOM when evaluating labels as it's very likely the text of an element is nested inside a Component whose subtree isn't accessible until render.

[ryanflorence]

can just skip composite components? we'll capture all the createElements of the normal components and might still get away with it? I'm not sure, I'm in a training right now and can't think it all the way through.

[AsaAyers]

I'm fairly sure you're right, but I think it might help to try to explain what's going on to a Rubber Duck.

<div onClick={k}>
  <span>
    <span>foo</span>
  </span>
</div>;
// Becomes:
React.createElement("div", {onClick: k}, 
  React.createElement("span", null, 
    React.createElement("span", null, "foo")
  )
);

react-a11y hooks into React.createElement(element, props, children). From this perspective the div call can walk the children down until it finds a child that is a string. This works great.

<div role="gridcell" onClick={k}>
  <Foo/>
</div>
// Becomes:
React.createElement("div", {role: "gridcell", onClick: k}, 
  React.createElement(Foo, null)
)  

Now we find an opaque component. At the time this is called Foo hasn't called its render so there's no way to know what might be inside of it. If I understand correctly Foo's render() isn't going to get called until this div is about to be mounted into the DOM.

We can confirm this from the console:

Foo = React.createClass({ render: function() { console.log('RENDER'); return false} })
React.createElement("div", {role: "gridcell"}, 
  React.createElement(Foo, null)
);

If Foo had rendered you would see RENDER in the console. So it's children definitely don't exist at this time.

[kloots]

@AsaAyers yes, you've got it. Those were my findings yesterday as well. Glad we're on the same page.

One idea I had to address this problem was to detect if an element's child is a Component and, if so, defer the evaluation of the label until it's been mounted. My initial thought was this would be easily achievable (famous last words) using the ref prop. However, I wasn't sure if that was a viable solution considering the element or Component might already have a ref defined.

[kloots]

@ryanflorence not sure how we can skip evaluation. Consider the following simple case (this is from our codebase at Twitter):

<div role="gridcell" onClick={k}>
  <Tweet/>
</div>;

When react-a11y evaluates the label for <div role="gridcell" onClick={k}> it'll throw an error because it cannot traverse into until Tweet has been rendered.

[AsaAyers]

Also React 0.13 introduced using callbacks for refs. It's going to get really ugly trying to hook into that.

<Foo ref={ (foo) => this.foo = foo} />

Everything is evaluated deepest call first, so I can see where you might use a ref if Foo had one and it's a string, but it doesn't seem like you can add it very easily. You'd have to send down a new set of props to add the ref correctly.

[AsaAyers]

Just like linters can't catch 100% of things, I don't think react-a11y needs to catch 100% of the problems. This might just be an edge case that isn't worth adding the needed complexity to solve.

If you know <Tweet /> ignores its children and you're sure it will have text you can make it pass:

<div role="gridcell" onClick={k}>
  <Tweet>Dummy Text</Tweet>
</div>;

[kloots]

@AsaAyers I agree with you; we don't need to catch 100% of the problems. However, when I incorporate react-a11y into our react-based Twitter client I end up getting a lot of label errors for the previously mentioned reason. I think this pattern of a component as a first-level descendant of an element could be a very common use case, so I'd be interested in attempting to solve this.

While I can make the change you suggested to make a test pass, it won't solve the problem for us outside the scope of tests.

[AsaAyers]

Why is the onClick handler on the <div> and not part of the component? You could push that responsibility down and have <Tweet /> generate the DOM node with the click handler.

<div role="gridcell" >
  <Tweet onClick={k} />
</div>

[kloots]

Good question. The timeline is it's own interactive component, with responsibilities and default behaviors independent of its content (Tweets). As a composite control, each of the items in the timeline is labeled by it's content (Tweets).

Visually impaired users need to be able to perceive the timeline as an interactive control; for that reason we use the ARIA grid roles to describe it; and make it interactive by applying a tabindex and onClick listener to each cell.

I hope that clears things up.