From 6d5544e329ec08b63eb38013f7d689f35da88d1a Mon Sep 17 00:00:00 2001 From: JRJurman Date: Sat, 17 Jul 2021 21:20:17 -0400 Subject: [PATCH 1/8] Fix input focus on reordered elements --- integration-tests/regression.test.js | 42 +++++++++++++- .../test-app/sub-mirror-input.js | 5 +- src/observe-tag.js | 57 +++++++++++++++---- 3 files changed, 91 insertions(+), 13 deletions(-) diff --git a/integration-tests/regression.test.js b/integration-tests/regression.test.js index 50e74bf..59f1f97 100644 --- a/integration-tests/regression.test.js +++ b/integration-tests/regression.test.js @@ -1,4 +1,4 @@ -const { getByText, fireEvent, waitFor, getByLabelText } = require('@testing-library/dom') +const { getByText, queryAllByText, fireEvent, waitFor, getByLabelText } = require('@testing-library/dom') const { default: userEvent } = require('@testing-library/user-event') const { startApp } = require('./test-app') @@ -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 () => { + // for focus to work correctly, the element needs to be attached to the document + const appContainer = document.createElement('div') + appContainer.id = 'app' + window.document.body.appendChild(appContainer) + + // start the app using a css selector + startApp('#app') + + // previously when interacting with an input, if the number of elements decreased + // an error was thrown because the element to focus on no longer existed + + // focus on the child input + userEvent.click(getByLabelText(appContainer, 'Sub Mirror Input')) + + // verify that the element has focus (before we start changing text) + await waitFor(() => { + expect(getByLabelText(appContainer, 'Sub Mirror Input')).toHaveFocus() + }) + + // update the state by typing + userEvent.type(getByLabelText(appContainer, 'Sub Mirror Input'), 'Test') + + // verify the element has the new value + expect(getByLabelText(appContainer, 'Sub Mirror Input')).toHaveValue('Test') + + // verify the element and it's parent have the new value + // also verify that the elements were added above it too (previously this would have failed) + // the element should still have focus + await waitFor(() => { + expect(getByLabelText(appContainer, 'Sub Mirror Input')).toHaveFocus() + expect(queryAllByText(appContainer, '-')).toHaveLength(4) + expect(getByLabelText(appContainer, 'Mirror Input')).toHaveValue('Test') + expect(getByLabelText(appContainer, 'Sub Mirror Input')).toHaveValue('Test') + }) + + // cleanup - remove app + appContainer.remove() + }) }) diff --git a/integration-tests/test-app/sub-mirror-input.js b/integration-tests/test-app/sub-mirror-input.js index b8d5877..6c4cc7b 100644 --- a/integration-tests/test-app/sub-mirror-input.js +++ b/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: '' }) + const mirrorable = useGlobalStore('mirrorable-input') const onEvent = event => { mirrorable.value = event.target.value } + const letterSpans = [...Array(mirrorable.value.length)].map(() => html`-`) + return html`
+ ${letterSpans}
diff --git a/src/observe-tag.js b/src/observe-tag.js index 6038cdc..a6fd180 100644 --- a/src/observe-tag.js +++ b/src/observe-tag.js @@ -1,6 +1,15 @@ const { observe } = require('@nx-js/observer-util') const { TRAM_TAG_REACTION, TRAM_TAG_NEW_EFFECTS, TRAM_TAG_CLEANUP_EFFECTS } = require('./node-names') +// helper functions for sorting +const toIndicies = (node, index) => index +const toNodes = (elementAndChildren) => (index) => elementAndChildren[index] +const byDistanceFromIndex = (targetIndex) => (indexA, indexB) => { + const diffFromTargetA = Math.abs(indexA - targetIndex) + const diffFromTargetB = Math.abs(indexB - targetIndex) + return diffFromTargetA < diffFromTargetB ? -1 : 1 +} + /** * This is a helper function for the dom creation. * This function observes any state values used when making the tag, and allow it to update @@ -15,6 +24,7 @@ module.exports = tagFunction => { let oldTag = tagResult const removedElementWithFocusData = { index: null, + tagName: null, selectionStart: null, selectionEnd: null, selectionDirection: null @@ -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 removedElementWithFocusData.selectionStart = removedElementWithFocus.selectionStart removedElementWithFocusData.selectionEnd = removedElementWithFocus.selectionEnd removedElementWithFocusData.selectionDirection = removedElementWithFocus.selectionDirection @@ -57,18 +68,42 @@ module.exports = tagFunction => { // if an element had focus, reapply it if (removedElementWithFocusData.index >= 0) { const children = tagResult.querySelectorAll('*') + const elementAndChildren = [tagResult, ...children] + + const newElementAtIndex = elementAndChildren[removedElementWithFocusData.index] + const newElementAtIndexMatches = newElementAtIndex && (elementAndChildren[removedElementWithFocusData.index].tagName === removedElementWithFocusData.tagName) + + let elementToGiveFocus + + // if the elementAtIndex matches, set that to be the element to give focus + if (newElementAtIndexMatches) { + elementToGiveFocus = newElementAtIndex + } + + // if the tagName doesn't match (or exist), it means the number of children probably changed... + // we can still try to find it though, we'll look through the children (in order of nodes closest to original index) and find a tag that matches + const nodeMatchesTagName = node => node.tagName === removedElementWithFocusData.tagName + if (!newElementAtIndexMatches) { + elementToGiveFocus = elementAndChildren + .map(toIndicies) + .sort(byDistanceFromIndex(removedElementWithFocusData.index)) + .map(toNodes(elementAndChildren)) + .find(nodeMatchesTagName) + } + + // if the element / child exists, focus it + if (elementToGiveFocus !== undefined) { + elementToGiveFocus.focus() - const elementToGiveFocus = [tagResult, ...children][removedElementWithFocusData.index] - elementToGiveFocus.focus() - - // also try to set the selection, if there is a selection for this element - const hasSelectionStart = removedElementWithFocusData.selectionStart !== null && removedElementWithFocusData.selectionStart !== undefined - if (hasSelectionStart) { - elementToGiveFocus.setSelectionRange( - removedElementWithFocusData.selectionStart, - removedElementWithFocusData.selectionEnd, - removedElementWithFocusData.selectionDirection - ) + // also try to set the selection, if there is a selection for this element + const hasSelectionStart = removedElementWithFocusData.selectionStart !== null && removedElementWithFocusData.selectionStart !== undefined + if (hasSelectionStart) { + elementToGiveFocus.setSelectionRange( + removedElementWithFocusData.selectionStart, + removedElementWithFocusData.selectionEnd, + removedElementWithFocusData.selectionDirection + ) + } } } From 3718f6390b3ff9a48b6bbea402ff73eb832c4d18 Mon Sep 17 00:00:00 2001 From: JRJurman Date: Sun, 18 Jul 2021 03:06:59 -0400 Subject: [PATCH 2/8] lint fixes --- integration-tests/test-app/sub-mirror-input.js | 2 +- src/observe-tag.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/integration-tests/test-app/sub-mirror-input.js b/integration-tests/test-app/sub-mirror-input.js index 6c4cc7b..9434a31 100644 --- a/integration-tests/test-app/sub-mirror-input.js +++ b/integration-tests/test-app/sub-mirror-input.js @@ -14,7 +14,7 @@ module.exports = () => { mirrorable.value = event.target.value } - const letterSpans = [...Array(mirrorable.value.length)].map(() => html`-`) + const letterSpans = [...new Array(mirrorable.value.length)].map(() => html`-`) return html`
diff --git a/src/observe-tag.js b/src/observe-tag.js index a6fd180..392555c 100644 --- a/src/observe-tag.js +++ b/src/observe-tag.js @@ -3,8 +3,8 @@ const { TRAM_TAG_REACTION, TRAM_TAG_NEW_EFFECTS, TRAM_TAG_CLEANUP_EFFECTS } = re // helper functions for sorting const toIndicies = (node, index) => index -const toNodes = (elementAndChildren) => (index) => elementAndChildren[index] -const byDistanceFromIndex = (targetIndex) => (indexA, indexB) => { +const toNodes = elementAndChildren => index => elementAndChildren[index] +const byDistanceFromIndex = targetIndex => (indexA, indexB) => { const diffFromTargetA = Math.abs(indexA - targetIndex) const diffFromTargetB = Math.abs(indexB - targetIndex) return diffFromTargetA < diffFromTargetB ? -1 : 1 From b0fc88a2ee7e319b37dfce3beb6703e673024ff6 Mon Sep 17 00:00:00 2001 From: JRJurman Date: Sun, 18 Jul 2021 03:07:19 -0400 Subject: [PATCH 3/8] 10.1.6 --- package-lock.json | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index c72d790..02a4f89 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,11 +1,11 @@ { "name": "tram-one", - "version": "10.1.5", + "version": "10.1.6", "lockfileVersion": 2, "requires": true, "packages": { "": { - "version": "10.1.5", + "version": "10.1.6", "license": "MIT", "dependencies": { "@nx-js/observer-util": "^4.2.2", diff --git a/package.json b/package.json index 77b0bd3..78ebc58 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "tram-one", - "version": "10.1.5", + "version": "10.1.6", "description": "🚋 Modern View Framework for Vanilla Javascript", "main": "dist/tram-one.cjs.js", "commonjs": "dist/tram-one.cjs.js", From 07294017ef898691855f0f4c697a745ef60106f6 Mon Sep 17 00:00:00 2001 From: JRJurman Date: Sun, 18 Jul 2021 03:07:53 -0400 Subject: [PATCH 4/8] update build badges --- docs/badges/cjs.svg | 4 ++-- docs/badges/umd.svg | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/badges/cjs.svg b/docs/badges/cjs.svg index dcf4a33..0246803 100644 --- a/docs/badges/cjs.svg +++ b/docs/badges/cjs.svg @@ -14,7 +14,7 @@ cjs cjs - 27.59 kB - 27.59 kB + 29.14 kB + 29.14 kB diff --git a/docs/badges/umd.svg b/docs/badges/umd.svg index e0daa35..318e78b 100644 --- a/docs/badges/umd.svg +++ b/docs/badges/umd.svg @@ -14,7 +14,7 @@ umd umd - 21.02 kB - 21.02 kB + 21.28 kB + 21.28 kB From 074339c1e87cee277819da3d374b8f552659b0dd Mon Sep 17 00:00:00 2001 From: JRJurman Date: Sun, 18 Jul 2021 03:37:51 -0400 Subject: [PATCH 5/8] add comments, simplify logic, set scrollLeft --- docs/badges/cjs.svg | 4 ++-- docs/badges/umd.svg | 4 ++-- src/observe-tag.js | 52 ++++++++++++++++++++++----------------------- 3 files changed, 30 insertions(+), 30 deletions(-) diff --git a/docs/badges/cjs.svg b/docs/badges/cjs.svg index 0246803..d812f8f 100644 --- a/docs/badges/cjs.svg +++ b/docs/badges/cjs.svg @@ -14,7 +14,7 @@ cjs cjs - 29.14 kB - 29.14 kB + 28.96 kB + 28.96 kB diff --git a/docs/badges/umd.svg b/docs/badges/umd.svg index 318e78b..c2d7de2 100644 --- a/docs/badges/umd.svg +++ b/docs/badges/umd.svg @@ -14,7 +14,7 @@ umd umd - 21.28 kB - 21.28 kB + 21.27 kB + 21.27 kB diff --git a/src/observe-tag.js b/src/observe-tag.js index 392555c..ce08f0c 100644 --- a/src/observe-tag.js +++ b/src/observe-tag.js @@ -10,6 +10,11 @@ const byDistanceFromIndex = targetIndex => (indexA, indexB) => { return diffFromTargetA < diffFromTargetB ? -1 : 1 } +const parentAndChildrenElements = node => { + const children = node.querySelectorAll('*') + return [node, ...children] +} + /** * This is a helper function for the dom creation. * This function observes any state values used when making the tag, and allow it to update @@ -27,23 +32,27 @@ module.exports = tagFunction => { tagName: null, selectionStart: null, selectionEnd: null, - selectionDirection: null + selectionDirection: null, + scrollLeft: null } // remove oldTag first so that we unobserve before we re-observe if (oldTag) { // if there was focus, we need to figure out what element has it - const children = oldTag.querySelectorAll('*') - const parentAndChildrenNodes = [oldTag, ...children] - removedElementWithFocusData.index = parentAndChildrenNodes.findIndex(element => element === document.activeElement) + const allElements = parentAndChildrenElements(oldTag) + removedElementWithFocusData.index = allElements.findIndex(element => element === document.activeElement) // 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] + // get the actual element + const removedElementWithFocus = allElements[removedElementWithFocusData.index] + + // copy over the data removedElementWithFocusData.tagName = removedElementWithFocus.tagName removedElementWithFocusData.selectionStart = removedElementWithFocus.selectionStart removedElementWithFocusData.selectionEnd = removedElementWithFocus.selectionEnd removedElementWithFocusData.selectionDirection = removedElementWithFocus.selectionDirection + removedElementWithFocusData.scrollLeft = removedElementWithFocus.scrollLeft } const emptyDiv = document.createElement('div') @@ -67,29 +76,17 @@ module.exports = tagFunction => { // if an element had focus, reapply it if (removedElementWithFocusData.index >= 0) { - const children = tagResult.querySelectorAll('*') - const elementAndChildren = [tagResult, ...children] - - const newElementAtIndex = elementAndChildren[removedElementWithFocusData.index] - const newElementAtIndexMatches = newElementAtIndex && (elementAndChildren[removedElementWithFocusData.index].tagName === removedElementWithFocusData.tagName) - - let elementToGiveFocus + const allElements = parentAndChildrenElements(tagResult) - // if the elementAtIndex matches, set that to be the element to give focus - if (newElementAtIndexMatches) { - elementToGiveFocus = newElementAtIndex - } - - // if the tagName doesn't match (or exist), it means the number of children probably changed... - // we can still try to find it though, we'll look through the children (in order of nodes closest to original index) and find a tag that matches + // we'll look through the elements (in order of nodes closest to original index) and find a tag that matches. + // this means if it didn't move, we'll get it right away, + // if it did, we'll look at the elements closest to the original position const nodeMatchesTagName = node => node.tagName === removedElementWithFocusData.tagName - if (!newElementAtIndexMatches) { - elementToGiveFocus = elementAndChildren - .map(toIndicies) - .sort(byDistanceFromIndex(removedElementWithFocusData.index)) - .map(toNodes(elementAndChildren)) - .find(nodeMatchesTagName) - } + const elementToGiveFocus = allElements + .map(toIndicies) + .sort(byDistanceFromIndex(removedElementWithFocusData.index)) + .map(toNodes(allElements)) + .find(nodeMatchesTagName) // if the element / child exists, focus it if (elementToGiveFocus !== undefined) { @@ -104,6 +101,9 @@ module.exports = tagFunction => { removedElementWithFocusData.selectionDirection ) } + + // also set the scrollLeft (since this is reset to 0 by default) + elementToGiveFocus.scrollLeft = removedElementWithFocusData.scrollLeft } } From b6d947910b5ebcbc93d9ed35164805d7fad8e091 Mon Sep 17 00:00:00 2001 From: JRJurman Date: Sun, 18 Jul 2021 03:46:46 -0400 Subject: [PATCH 6/8] better comments --- docs/badges/cjs.svg | 4 ++-- src/observe-tag.js | 8 ++++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/docs/badges/cjs.svg b/docs/badges/cjs.svg index d812f8f..4d7abf8 100644 --- a/docs/badges/cjs.svg +++ b/docs/badges/cjs.svg @@ -14,7 +14,7 @@ cjs cjs - 28.96 kB - 28.96 kB + 29.15 kB + 29.15 kB diff --git a/src/observe-tag.js b/src/observe-tag.js index ce08f0c..07f93e3 100644 --- a/src/observe-tag.js +++ b/src/observe-tag.js @@ -1,15 +1,19 @@ const { observe } = require('@nx-js/observer-util') const { TRAM_TAG_REACTION, TRAM_TAG_NEW_EFFECTS, TRAM_TAG_CLEANUP_EFFECTS } = require('./node-names') -// helper functions for sorting +// functions to go to nodes or indicies (made for .map) const toIndicies = (node, index) => index -const toNodes = elementAndChildren => index => elementAndChildren[index] +const toNodes = allNodes => index => allNodes[index] + +// sorting function that prioritizes indicies that are closest to a target +// e.g. target = 3, [1, 2, 3, 4, 5] => [3, 2, 4, 1, 5] const byDistanceFromIndex = targetIndex => (indexA, indexB) => { const diffFromTargetA = Math.abs(indexA - targetIndex) const diffFromTargetB = Math.abs(indexB - targetIndex) return diffFromTargetA < diffFromTargetB ? -1 : 1 } +// get an array including the element and all it's children const parentAndChildrenElements = node => { const children = node.querySelectorAll('*') return [node, ...children] From 0275c31779d54a2ca86e4746a40dc91bfd979c2e Mon Sep 17 00:00:00 2001 From: JRJurman Date: Sun, 18 Jul 2021 03:52:12 -0400 Subject: [PATCH 7/8] fix scrollTop while we are here --- docs/badges/cjs.svg | 4 ++-- docs/badges/umd.svg | 4 ++-- src/observe-tag.js | 7 +++++-- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/docs/badges/cjs.svg b/docs/badges/cjs.svg index 4d7abf8..17fb586 100644 --- a/docs/badges/cjs.svg +++ b/docs/badges/cjs.svg @@ -14,7 +14,7 @@ cjs cjs - 29.15 kB - 29.15 kB + 29.33 kB + 29.33 kB diff --git a/docs/badges/umd.svg b/docs/badges/umd.svg index c2d7de2..7b79eca 100644 --- a/docs/badges/umd.svg +++ b/docs/badges/umd.svg @@ -14,7 +14,7 @@ umd umd - 21.27 kB - 21.27 kB + 21.33 kB + 21.33 kB diff --git a/src/observe-tag.js b/src/observe-tag.js index 07f93e3..4542627 100644 --- a/src/observe-tag.js +++ b/src/observe-tag.js @@ -37,7 +37,8 @@ module.exports = tagFunction => { selectionStart: null, selectionEnd: null, selectionDirection: null, - scrollLeft: null + scrollLeft: null, + scrollTop: null } // remove oldTag first so that we unobserve before we re-observe @@ -57,6 +58,7 @@ module.exports = tagFunction => { removedElementWithFocusData.selectionEnd = removedElementWithFocus.selectionEnd removedElementWithFocusData.selectionDirection = removedElementWithFocus.selectionDirection removedElementWithFocusData.scrollLeft = removedElementWithFocus.scrollLeft + removedElementWithFocusData.scrollTop = removedElementWithFocus.scrollTop } const emptyDiv = document.createElement('div') @@ -106,8 +108,9 @@ module.exports = tagFunction => { ) } - // also set the scrollLeft (since this is reset to 0 by default) + // also set the scrollLeft and scrollTop (since this is reset to 0 by default) elementToGiveFocus.scrollLeft = removedElementWithFocusData.scrollLeft + elementToGiveFocus.scrollTop = removedElementWithFocusData.scrollTop } } From 6d19044459243dc661c247eb102234947dc986e0 Mon Sep 17 00:00:00 2001 From: JRJurman Date: Sun, 18 Jul 2021 11:17:24 -0400 Subject: [PATCH 8/8] slight change to sort --- src/observe-tag.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/observe-tag.js b/src/observe-tag.js index 4542627..94bdaa5 100644 --- a/src/observe-tag.js +++ b/src/observe-tag.js @@ -10,7 +10,7 @@ const toNodes = allNodes => index => allNodes[index] const byDistanceFromIndex = targetIndex => (indexA, indexB) => { const diffFromTargetA = Math.abs(indexA - targetIndex) const diffFromTargetB = Math.abs(indexB - targetIndex) - return diffFromTargetA < diffFromTargetB ? -1 : 1 + return diffFromTargetA - diffFromTargetB } // get an array including the element and all it's children