Fix element blurring (removing focus) #116
Conversation
Previously, Pjax would blur (remove focus) from the active element regardless of where it was on the page. This restricts that to happen only if the active element is contained by one of the elements represented by options.selectors, because only those are affected by Pjax. Fixes #4
index.js
Outdated
| @@ -129,7 +130,7 @@ Pjax.prototype = { | |||
|
|
|||
| // Clear out any focused controls before inserting new page contents. | |||
| // we clear focus on non form elements | |||
| if (document.activeElement && !document.activeElement.value) { | |||
| if (document.activeElement && !document.activeElement.value && contains(this.options.selectors, document.activeElement)) { | |||
There was a problem hiding this comment.
Is there a reason to not clear the focus on form elements? jquery-pjax doesn't do this: https://github.com/defunkt/jquery-pjax/blob/c9acf5e7e9e16fdd34cb2de882d627f97364a952/jquery.pjax.js#L296
There was a problem hiding this comment.
I guess the question is, what are we trying to accomplish.
From what I understand, jquery-pjax just wanted that the focus shouldn't be on an element that doesn't exist anymore, so they only blurred elements inside the containers that were being replaced. If we're trying to do the same, I think the PR as it is now accomplishes that.
After rereading #4, it sounds like the goal of that issue is to remove focus from the link (or form) that was clicked, in order to get rid of things like residual tooltips, while not removing focus from elements like search boxes etc.
If those links are inside the containers being replaced, I can understand why the user wouldn't want those tooltips, and that would still be covered after this PR.
If those links are elsewhere on the page, why would you want to remove focus from them?
There was a problem hiding this comment.
Yes, I agree that the aim should be to clear the focus on elements that aren't going to exist after the switch, which is why I asked if there was a particular reason not to do this when the active element has a value attribute (is a form element). I think that check could be eliminated, simplifying the test for elements to blur to
if (document.activeElement && contains(this.options.selectors, document.activeElement)) {There was a problem hiding this comment.
I think you're right. I assume that check was only there since the original code was clearing focus no matter where on the page it was.
| @@ -6,6 +6,7 @@ var forEachEls = require("./lib/foreach-els.js") | |||
| var newUid = require("./lib/uniqueid.js") | |||
|
|
|||
| var noop = require("./lib/util/noop") | |||
| var contains = require("./lib/util/contains.js") | |||
There was a problem hiding this comment.
I think these vars were originally alphabetically ordered, but grouping modules by path is also something that makes sense -- another thing to standardise on for #97?
There was a problem hiding this comment.
Good idea. I'll mention that there.
|
LGTM! 🎉 |
Only blur element if it's contained by one of the selectors
Previously, Pjax would blur (remove focus) from the active element regardless of where it was on the page. This commit restricts that to happen only if the active element is contained by one of the elements represented by options.selectors, because only those are affected by Pjax.
Fixes Add blur on active element when page is changed #4
Remove a redundant call to .blur()