Add setTimeout to AutoFocusMixin.

[kramerc]

This ensures the element gets focused after the component is mounted. Without it, the element may not be focused as the browser hasn't finished rendering it, an issue which I encountered.

[syranide]

I'm unable to repro this myself: http://jsfiddle.net/fCF45/1/

Are you sure you're not delaying attaching to the document yourself?

[syranide]

The following code provides a fallback for when the DOM cannot be focused (presumably because it's not yet attached to the document). It would be possible to avoid trial-and-error but it would put older browsers (including IE8/9) at a disadvantage as setTimeout(f, 0) is not instant. It can also become indeterministic if two inputs are fighting for autoFocus.

But personally I'm not feeling good about it without a real use-case.

var requestAnimationFrame = require('requestAnimationFrame');
var focusNode = require('focusNode');

var AutoFocusMixin = {
  componentDidMount: function() {
    if (this.props.autoFocus) {
      node = this.getDOMNode();
      focusNode(node);

      if (node.ownerDocument.activeElement !== node) {
        requestAnimationFrame(function() {
          focusNode(node);
        });
      }
    }
  }
};

[kramerc]

That's interesting. I am adding components when I receive data regarding a screen that adds field elements and switches over to that in terms of visibility. I can't reproduce it in JSFiddle either but I can reproduce it in my app.

As for your code, it does also solve the problem, but it appears IE8/IE9 don't support it. http://caniuse.com/#search=requestAnimationFrame

[syranide]

Hmm, that's weird, I couldn't find anything immediately off about your setup (also, awesome GIF!). You're running React 0.10 I assume?

As for requestAnimationFrame you would be right :), but it's var requestAnimationFrame = require('requestAnimationFrame') not window.requestAnimationFrame, it's an internal shim with fallbacks.

[kramerc]

Ah right, I didn't check that there was a polyfill for requestAnimationFrame and I overlooked that require you put there. I am using React 0.10 but when making this pull request I was using React 0.11.0-alpha to test this change with my app. That GIF is demonstrating the problem with React 0.10 though without any of my changes.

[syranide]

Could you add the following to componentDidMount in MessageForm and try it:

var node = this.getDOMNode();
while (node.parentNode) {
    node = node.parentNode;
}
console.log(node.outerHTML);

[kramerc]

node.outerHTML comes out as undefined. I added console.log(node) and node ends up being the #document element.

[syranide]

Right, looking at your GIF again (again, awesome!), I think I know what the problem is. Your window is not focused when the DOM is mounted and thus the autofocus fails. Try manually loading your page (or just refreshing) and see if it helps (which is supported by your last comment).

[kramerc]

Hmm, doing with what I can in Atom Shell, it doesn't seem to have any effect. This problem also occurs when new buffers are created and switched over at the same time.

For instance, the message field should have been focused right after I performed /connect. You'll see I clicked on the field shortly after.

[syranide]

Hmm, intuitively it seems like a defect in Atom Shell. If it's possible for you to try it in the browser, then that would provide a definitive answer. However, seeing as it never manages to autofocus at all, and it's running in what looks like Chrome, it seems to me that Atom Shell is at fault here.

It doesn't make sense that input.focus() doesn't actually focus the field, as we know it's attached to the document.

[sophiebits]

I've also encountered this in the past and added a setTimeout to fix:

https://github.com/Khan/khan-exercises/blob/f73bb42c01d105937b149073893bedfdb4674c91/khan-exercise.js#L1206-L1213

I don't have repro steps handy though.

[syranide]

@kramerc One final test on my behalf for today if you please, in componentDidMount:

var node = this.refs.[inputRefName].getDOMNode();
node.focus();
console.log(node.ownerDocument.activeElement !== node);

[kramerc]

@syranide Browserifying my code (with small modifications as I needed to stub the required ipc module), the input field doesn't focus on Chrome 34. Your code logs true on both Atom Shell and Chrome though.

Edit: Turns out I had a setTimeout in my code under componentDidMount before Browserifying it, so your code didn't cause it to focus in Chrome.

[syranide]

@kramerc Hmm, this is really weird, since it logs true it would mean that it does focus the input (and it is attached to the document as we already know), but then blurs it for whatever reason before it's presented to the user. Wups, !==, so it doesn't focus it for some reason, that's "good" in that we can at least use it as a cheap test for whether we should use the fallback or not, if we decide to go that route.

It's really interesting though that it doesn't even work in Chrome for you, as I mentioned, your code is simple enough and I couldn't find anything suspicious in it, but it works in the fiddle... this is weird.

Huge thanks helping debug this.

[kramerc]

@syranide Thanks for helping, this problem is weird indeed. Should I update this pull request to use requestAnimationFrame, leave it as is, or close it?

[syranide]

@kramerc I defer that decision to some official dev (but please keep it open). Personally I feel like it would be good to know what's actually wrong so that we could fix it at the source. However, from a practical standpoint, this is going to be an ongoing problem and I'm fairly certain that even if we conclusively find the source of the problem, that it wouldn't lead to an actionable fix (telling people they're doing it wrong without an actionable fix will just leave autoFocus unused).

Given that we can use node.ownerDocument.activeElement !== node as a very cheap fallback (at no detriment to unaffected users), it seems like the best of both worlds to me. I don't think we use the requestAnimationFrame-shim elsewhere in React core, so I'm unsure of if it's worth the increase in size over a plain setTimeout(f, 0).

I think there is a theoretical downside to having the fallback in-terms of inputs fighting for focus, realistically though it's a "bad situation" and there's no right answer (a deterministic result would be preferable, but hardly of great importance).

Again, I defer to some official dev, but that would be my take on the situation.

[kramerc]

@syranide Alright, I'll keep this open. I added a log statement to see what node.ownerDocument.activeElement comes up as and it ends up being the body element of the document. Perhaps that might give some clues as to what the underlying problem is.

[zpao]

cc @salier & @joshduck who might have some more insight into focus.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[bmeck]

example test case : http://jsfiddle.net/fCF45/3/

[syranide]

@bmeck If I'm reading the code correctly, that is a different issue and one that we can't solve as there is no DOM event to signal when an element has become visible.

[bmeck]

@syranide I am unsure, don't see any simple test cases if you have a better one. If the element is on the DOM and is visible it should focus() properly. I've a lot of experience in the area.

[sebmarkbage]

The problem remains but I'm adding the needs-revision label because this can't go in as-is.

[syranide]

Not exactly sure what @sebmarkbage is referring to, but I imagine we would want to use requestAnimationFrame (or vendor prefixed) when available to make it instantaneous.

[syranide]

I'll go ahead and 👍 this, it is has started occurring consistently across our entire app in Chrome (I'm confident I've made no related change). I use this PR as a workaround for now and it seems to work well.

[kramerc]

@sebmarkbage What needs to be changed?

[syranide]

@kramerc Thinking about it briefly, I imagine we want to guarantee that only one setTimeout is active at any one time. I haven't actually battle tested it, but my idea of using:

node.focus(); if (node.ownerDocument.activeElement !== node) { ... defer ... }

...is.also a possibility where we might get the best of both worlds (although consistent behavior/timing across browsers might be preferable).

[jimfb]

I don't think this plugin even exists anymore. Regardless, merge conflicts and no comments in six months, closing.