Fix input focus on re-ordered elements #164

JRJurman · 2021-07-18T01:21:56Z

Summary

Fixes an issue where if the elements were re-ordered or changed significantly, we wouldn't be able to re-attach focus. Worse so, we never check if the element exists before trying to focus it, so we would get an error!

This PR also fixes an error where the scroll position would be reset when typing in an input.

Screenshots (Before)

When interacting with an input that has <span>-</span> for every letter in the input. This is what happens when typing and then deleting (this is the integration test app, which can be run locally with npm run test:app):

The first set of errors is because we are trying to setSelectionRange on a <span>, the second error is because after deleting no element exists at the index that the <input> was on.

Screenshots (After)

No errors 🎉

Checklist

PR Summary
Tests
Version Bump

JRJurman · 2021-07-18T07:11:57Z

src/observe-tag.js

+const byDistanceFromIndex = targetIndex => (indexA, indexB) => {
+	const diffFromTargetA = Math.abs(indexA - targetIndex)
+	const diffFromTargetB = Math.abs(indexB - targetIndex)
+	return diffFromTargetA < diffFromTargetB ? -1 : 1
+}


this sorting function prioritizes indices that are closest to a target.
E.g. if the target is 5 out of [1, 2, 3, 4, 5, 6, 7, 8, 9],
then the new sorting would be [5, 4, 6, 3, 7, 2, 8, 1, 9].

I don't know if there's a more elegant way to write this, but this works 👍

JRJurman · 2021-07-18T07:13:06Z

src/observe-tag.js

@@ -30,6 +40,7 @@ module.exports = tagFunction => {
 			// if an element had focus, copy over all the selection data (so we can copy it back later)
 			if (removedElementWithFocusData.index >= 0) {
 				const removedElementWithFocus = parentAndChildrenNodes[removedElementWithFocusData.index]
+				removedElementWithFocusData.tagName = removedElementWithFocus.tagName


since we now search for the element we previously had focus on, we want to store the tagName (this way, if an input is surrounded by other different elements, we can still find it again and reapply focus)

JRJurman · 2021-07-18T07:40:08Z

integration-tests/regression.test.js

@@ -221,4 +221,44 @@ describe('Tram-One - regressions', () => {
 		// cleanup - remove app
 		appContainer.remove()
 	})
+
+	it('should not error when reseting focus if the number of elements changed', async () => {


the test app needed to be updated, and actually caused a previous test to fail without the fix, but it still feels worthwhile to give this it's own test, so that if they ever get refactored, we don't forget about it.

JRJurman · 2021-07-18T07:41:43Z

integration-tests/test-app/sub-mirror-input.js

@@ -8,14 +8,17 @@ const html = registerHtml({
 * component to test url parameters
 */
 module.exports = () => {
-	const mirrorable = useGlobalStore('mirrorable-input', { value: '' })


I don't think we actually needed this , { value: '' }, it was documented as an error #156 , but this works without it

In reality, I just need to create a minimal example of this...

JRJurman · 2021-07-18T07:43:15Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "tram-one",
-  "version": "10.1.5",
+  "version": "10.1.6",


no API change, fixes an issue with a feature implementation, so patch is good 👍

JRJurman · 2021-07-18T07:55:33Z

src/observe-tag.js

+				removedElementWithFocusData.scrollLeft = removedElementWithFocus.scrollLeft
+				removedElementWithFocusData.scrollTop = removedElementWithFocus.scrollTop


this preserves the scroll position in an input or textarea that you are typing in. Previously you this would get reset on re-render, so this experience is more in-line with what people would expect.

I don't know that there's a great way to test this, so I'm fine not writing an integration test - this is one thing where I think it really requires manual verification... anything else would be just checking properties, which seems disingenuous since all elements support scrollLeft and scrollTop, even if they don't do anything.

JRJurman · 2021-07-18T07:58:25Z

src/observe-tag.js

+				const elementToGiveFocus = allElements
+					.map(toIndicies)
+					.sort(byDistanceFromIndex(removedElementWithFocusData.index))
+					.map(toNodes(allElements))
+					.find(nodeMatchesTagName)
+
+				// if the element / child exists, focus it
+				if (elementToGiveFocus !== undefined) {


This is the actual fix. We look through all elements now, and verify that the element even exists. Previously, even if the element was removed, we'd still try to focus on something, which caused errors to be thrown.

JRJurman · 2021-07-18T08:00:31Z

docs/badges/cjs.svg

-    <text x="59" y="15" fill="#010101" fill-opacity=".3">27.59 kB</text>
-    <text x="59" y="14">27.59 kB</text>
+    <text x="59" y="15" fill="#010101" fill-opacity=".3">29.33 kB</text>
+    <text x="59" y="14">29.33 kB</text>


Most of this is comments, which is fine 👍

src/observe-tag.js

ethanjurman · 2021-07-18T15:09:43Z

src/observe-tag.js

+const parentAndChildrenElements = node => {
+	const children = node.querySelectorAll('*')
+	return [node, ...children]
+}


Is this recursive/ should it be?

querySelectorAll (which we use in other places as well) will actually get all the children under an element.

ethanjurman · 2021-07-18T15:11:32Z

src/observe-tag.js

-			const parentAndChildrenNodes = [oldTag, ...children]
-			removedElementWithFocusData.index = parentAndChildrenNodes.findIndex(element => element === document.activeElement)
+			const allElements = parentAndChildrenElements(oldTag)
+			removedElementWithFocusData.index = allElements.findIndex(element => element === document.activeElement)


Could we do a query selector here for active element, instead of checking against a large list created by a wildcard selector?

The goal here is to find the index of the element, not the element itself (if that's all we needed, we could just use document.activeElement). I'm not sure that a more specific query selector would help... 🤔 thinking about it now, we could query selector on only things that have a positive tabIndex?

This might be just another one of those things where I want to do the performance test, and then fix as necessary.

I do agree that if there are performance issues, it's that we have to query and iterate over all the children of an element before and after. I'm not a fan either...

ethanjurman

Looks good, a few questions, and can you provide any examples of it failing before/ success after?

chtinahow · 2021-07-18T15:17:04Z

src/observe-tag.js

+					elementToGiveFocus.focus()
+
+					// also try to set the selection, if there is a selection for this element
+					const hasSelectionStart = removedElementWithFocusData.selectionStart !== null && removedElementWithFocusData.selectionStart !== undefined


Is there a reason why you can check to see if removedElementWithFocusData.selectionStart is truthy? Do null and undefined come back as truthy in this case, or is there a falsy value that you want to return as true?

selectionStart can be 0 which is a falsy value, so yeah, we have to explicitly check if it's null or undefined.

JRJurman · 2021-07-18T15:35:08Z

Looks good, a few questions, and can you provide any examples of it failing before/ success after?

Summary has been updated 👍

chtinahow

LGTM

JRJurman added 4 commits July 17, 2021 21:20

Fix input focus on reordered elements

6d5544e

lint fixes

3718f63

10.1.6

b0fc88a

update build badges

0729401

JRJurman commented Jul 18, 2021

View reviewed changes

add comments, simplify logic, set scrollLeft

074339c

JRJurman commented Jul 18, 2021

View reviewed changes

JRJurman added 2 commits July 18, 2021 03:46

better comments

b6d9479

fix scrollTop while we are here

0275c31

JRJurman commented Jul 18, 2021

View reviewed changes

ethanjurman reviewed Jul 18, 2021

View reviewed changes

src/observe-tag.js Outdated Show resolved Hide resolved

ethanjurman reviewed Jul 18, 2021

View reviewed changes

ethanjurman approved these changes Jul 18, 2021

View reviewed changes

chtinahow reviewed Jul 18, 2021

View reviewed changes

slight change to sort

6d19044

chtinahow approved these changes Jul 18, 2021

View reviewed changes

JRJurman merged commit d29344e into master Jul 18, 2021

JRJurman deleted the input-order-fix branch July 18, 2021 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix input focus on re-ordered elements #164

Fix input focus on re-ordered elements #164

JRJurman commented Jul 18, 2021 •

edited

Loading

JRJurman Jul 18, 2021

JRJurman Jul 18, 2021

JRJurman Jul 18, 2021

JRJurman Jul 18, 2021

JRJurman Jul 18, 2021

JRJurman Jul 18, 2021

JRJurman Jul 18, 2021

JRJurman Jul 18, 2021

JRJurman Jul 18, 2021

ethanjurman Jul 18, 2021

JRJurman Jul 18, 2021

ethanjurman Jul 18, 2021

JRJurman Jul 18, 2021 •

edited

Loading

JRJurman Jul 18, 2021 •

edited

Loading

ethanjurman left a comment

chtinahow Jul 18, 2021

JRJurman Jul 18, 2021

JRJurman commented Jul 18, 2021

chtinahow left a comment

		removedElementWithFocusData.scrollLeft = removedElementWithFocus.scrollLeft
		removedElementWithFocusData.scrollTop = removedElementWithFocus.scrollTop

Fix input focus on re-ordered elements #164

Fix input focus on re-ordered elements #164

Conversation

JRJurman commented Jul 18, 2021 • edited Loading

Summary

Screenshots (Before)

Screenshots (After)

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JRJurman Jul 18, 2021 • edited Loading

Choose a reason for hiding this comment

JRJurman Jul 18, 2021 • edited Loading

Choose a reason for hiding this comment

ethanjurman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JRJurman commented Jul 18, 2021

chtinahow left a comment

Choose a reason for hiding this comment

JRJurman commented Jul 18, 2021 •

edited

Loading

JRJurman Jul 18, 2021 •

edited

Loading

JRJurman Jul 18, 2021 •

edited

Loading