Skip to content
This repository has been archived by the owner on May 27, 2019. It is now read-only.

WIP: Bug/ff input auto select #158

Merged
merged 5 commits into from
Oct 8, 2017

Conversation

turboMaCk
Copy link
Contributor

@turboMaCk turboMaCk commented Sep 27, 2017

issue: #155

This is an initial spike of a bugfix for FireFox which is not autofocusing search input when the popup is opened.

I've had to upgrade mithril since old version doesn't provide lifecycle hooks we need. It seems there is no change in API that this extension is using.

Secondly, it turned out that FireFox is ignoring focus calls even though the element is already in DOM and can be queried via DOM API. This is the reason for setTimeout.

Finally, it seems I have a different version in my fork than is in upstream (even though I tried to pull) - I'll resolve it.

@turboMaCk turboMaCk force-pushed the bug/ff_input_auto_select branch from 23d0e03 to 5bdd62d Compare September 27, 2017 20:44
? document.querySelector(firstSelector)
: document.activeElement[nextNodeAttr];

if (newActive) {
newActive.focus();
} else {
document.getElementById(inputId).focus();
searchField.focus();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a tiny change I forgot to push to the previous PR it seems. I can remove it from this one if you want.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need, keep it here 👍

if (searchField) {
searchField.focus();
}
}, 100);
Copy link
Contributor Author

@turboMaCk turboMaCk Sep 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is that ugly hack with timeout:(

EDIT: I've tried to set it to 0 just to run it once call stack is empty - it didn't work though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to wrap it in requestAnimationFrame instead of the setTimeout, I'm curious if that's what FF wants.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also move the call to getElementById inside the requestAnimationFrame, just to make sure we aren't searching too early :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just found some time to try it and it doesn't work which makes sense to me. if window.setTimeout(fc, 0) doesn't work I wouldn't expect requestAnimationFrame to work either. With 0 timeout browser evaluate function right after call-stack is empty. With the request, animation frame code is evaluated even before redraw.

I also tried this sort of "recursive" approach:

function oncreate() {
  var searchField = document.getElementById('search-field');
  searchField.focus();

  if (document.activeElement !== searchField) {
    window.requestAnimationFrame(oncreate);
  }
}

which doesn't work either.

One thing I'm still not sure about is what makes timeout 100 works. Is it guaranteed to always work (since there is some timeout before focus() calls are accepted) or is some race in async code? I don't really know and frankly, I don't know how to test it either. So I did it just by trying. and it seems that with 40ms timeout it sometimes works and sometimes doesn't. Which seems to be answer - timeout might be simply buggy.

Next thing I did was to remove whole setTimeout hack together with autofocus: "on" and test that in chrome. And it worked correctly.

To me, it seems that same thing that prevents us to simply use autofocus in FF is the thing that blocks focus() call. Maybe we can try different DOM events but I'm skeptical.

Another solution that came to my mind was to rewrite that function so it will perform check every 10ms and if it's not successful it will set itself to run in next 10ms until it is. Anyway, I did quick and it seems that document.activeElement returns input even though it's not really selected so there is no (easy) way to perform that test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks for doing the experiments! Then let's keep it simple and not over-engineer stuff, it's a bit hacky - yes, but it works and it's quite simple to understand, especially given that you left an explanatory comment. I'll merge now. Thanks 👍

@maximbaz
Copy link
Member

I updated the all the dependencies on master, could you please rebase? 🙂

@maximbaz maximbaz merged commit 83086fd into browserpass:master Oct 8, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants