Fix element blurring (removing focus) #116

2018-01-24T20:20:50-05:00

BehindTheMath commented

2018-01-24 20:20:50 -05:00

(Migrated from github.com)

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 #4
Remove a redundant call to .blur()

- 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 #4 - Remove a redundant call to .blur()

robinnorth (Migrated from github.com) reviewed 2018-01-25 04:59:30 -05:00

index.js

						
				@@ -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")

robinnorth (Migrated from github.com) commented

2018-01-25 04:58:32 -05:00

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?

index.js

robinnorth (Migrated from github.com) commented

2018-01-25 04:54:39 -05:00

Is there a reason to not clear the focus on form elements? jquery-pjax doesn't do this: c9acf5e7e9/jquery.pjax.js (L296)

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

BehindTheMath (Migrated from github.com) reviewed 2018-01-25 19:14:45 -05:00

index.js

						
				@@ -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")

BehindTheMath (Migrated from github.com) commented

2018-01-25 19:14:45 -05:00

Good idea. I'll mention that there.

BehindTheMath (Migrated from github.com) reviewed 2018-01-29 09:49:10 -05:00

index.js

BehindTheMath (Migrated from github.com) commented

2018-01-29 09:49:10 -05:00

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?

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?

robinnorth (Migrated from github.com) reviewed 2018-01-29 10:19:07 -05:00

index.js

robinnorth (Migrated from github.com) commented

2018-01-29 10:19:07 -05:00

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)) {

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 ```js if (document.activeElement && contains(this.options.selectors, document.activeElement)) { ```

BehindTheMath (Migrated from github.com) reviewed 2018-01-29 13:50:20 -05:00

index.js

BehindTheMath (Migrated from github.com) commented

2018-01-29 13:50:20 -05:00

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.

robinnorth commented

2018-01-29 15:02:43 -05:00

(Migrated from github.com)

LGTM! 🎉

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: iLoveElysia/pjax#116