Remove innerHTML assignments from js #4065

LuomaJuha · 2024-11-07T14:23:37Z

InnerHTML assignment is pretty costly and usually it is adviced to not be used, so I created this pr to remove or adjust code to use proper assignments. Requires littlebit testing and adjustments.

TODO:

Tests (Tests are not passing so will have to see and fix the broken parts.)
Work still to be done

demiankatz

Thanks, @LuomaJuha! I may not have a chance to review this in detail right away... but in the meantime, I see that a PR I just merged has introduced a conflict here. Do you mind resolving it? Sorry for the inconvenience!

LuomaJuha · 2024-11-08T07:56:30Z

@demiankatz Sure! Will resolve in short future

demiankatz

Thanks, @LuomaJuha -- just a couple of questions.

Also, the full test suite is not passing for me on this branch. I'm seeing:

There were 2 errors:

1) VuFindTest\Mink\CombinedSearchTest::testCombinedSearchResultsMixedAjaxDOIs with data set "all ajax" (true, true)
Exception: Failed to call getText on '#combined_Solr____one .doiLink a' after 6 attempts.

/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:794
/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:713
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/CombinedSearchTest.php:275

2) VuFindTest\Mink\HoldingsTest::testItemStatusFull with data set "custom template test" (true, 'On Shelf', 'On Shelf', 'success', 'msg', true, true, true)
Exception: Selector '.js-status-test.hidden' remains accessible

/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:599
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/HoldingsTest.php:274

--

There was 1 failure:

1) VuFindTest\Mink\RecordVersionsTest::testVersionsTabInit
Element not found: .result-links .qrcode img index 0
Failed asserting that null is of type object.

/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:527
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/RecordVersionsTest.php:130

I've re-run all of the failing test classes individually to confirm that these failures occur consistently in my environment. I suspect some (possibly all) of it has to do with the tricky code related to executing scripts included in AJAX responses.

themes/bootstrap3/js/common.js

demiankatz · 2024-11-12T13:21:38Z

themes/bootstrap3/js/check_item_statuses.js

      });
    } else {
      // Default case -- load call number and location into appropriate containers:
      el.querySelectorAll('.callnumber').forEach((callnumber) => {
-        callnumber.innerHTML = typeof(result.callnumberHtml) !== 'undefined' && result.callnumberHtml
-          ? result.callnumberHtml + '<br>'


I see that you're dropping the <br> here, which is probably a good thing. Just double-checking whether you confirmed that doing so has no visible side-effects.

Ill double check this one

demiankatz

Thanks, @LuomaJuha. I can't see any problems with this, and all tests are passing on my end now. However, Javascript is not my speciality, so I'll leave this open a couple of days in case others like @crhallberg or @EreMaijala have anything to add.

Also, you still have an unchecked "Tests" TODO item -- is there still something you're planning to work on on your end?

crhallberg

I think the replacement of innerHTML with textContent in the case of strings makes a lot of sense. I am curious where you read about the performance of .innerHTML = str.

Do you think it would be worth it to add a element builder function, like this very simple one?

crhallberg · 2024-11-12T21:13:36Z

themes/bootstrap3/js/common.js

    if (property === 'innerHTML') {
-      elm.innerHTML = tmpDiv.innerHTML;
+      elm.textContent = '';
+      elm.append(...tmpDiv.childNodes);


In this instance - and perhaps on line 313 as well - it may be clearer and more appropriate to use replaceChildren.

oh ye, it was added in 2020 i suppose, but I think it is enough for being used?

@EreMaijala, do you have an opinion on this? I know you're most concerned about certain older Safari versions...

I wouldn't use replaceChildren just yet.

On a second thought, let's use it and just provide a polyfill (example) for browsers that don't support it.

crhallberg · 2024-11-12T21:14:29Z

themes/bootstrap3/js/common.js

-        newElm = parentElm.lastElementChild;
-      }
+      elm.textContent = '';
+      elm.replaceWith(...tmpDiv.childNodes);


Probably don't need to reset textContent if we are replacing the whole element.

crhallberg · 2024-11-12T21:15:19Z

themes/bootstrap3/js/common.js

    // Set any attributes (N.B. has to be done before scripts in case they rely on the attributes):
-    Object.entries(attrs).forEach(([attr, value]) => newElm.setAttribute(attr, value));
-
-    // Append any scripts:


I'd prefer if we preserved this blank line and comment.

crhallberg · 2024-11-12T21:17:29Z

themes/bootstrap3/js/search.js

@@ -293,7 +293,7 @@ VuFind.register('search', function search() {
    loadingOverlay.classList = 'loading-overlay';
    loadingOverlay.setAttribute('aria-live', 'polite');
    loadingOverlay.setAttribute('role', 'status');
-    loadingOverlay.innerHTML = VuFind.loading();
+    VuFind.setInnerHtml(loadingOverlay, VuFind.loading());


Would it make more sense to have VuFind.loading and VuFind.spinner return elements instead of HTML and use append or replaceChildren here?

Maybe it would be even better to have loadingElements and spinnerElements methods instead of changing the existing loading and spinner for backward-compatibility. We could always mark those as deprecated and remove them later if they're not widely used.

Sure. ill adjust a lttilebit more, might take a while but in close future :) will request re reviews when adjusted

crhallberg · 2024-11-12T21:20:18Z

themes/bootstrap3/templates/combined/results-list.phtml

+        const recordTotalSpan = document.createElement('span');
+        recordTotalSpan.classList.add('record-total');
+        recordTotalSpan.textContent = '(' + count + ')';
+        template.append(recordTotalSpan);


If we're creating the element directly, we don't need this template any more.

LuomaJuha · 2024-12-11T08:02:19Z

been little busy, but havent forgotten. Will try to adjust this week to match reviews :)

demiankatz

I haven't thoroughly reviewed everything yet, but I did see one thing that looked like there might be an opportunity for simplification/clarification.

demiankatz · 2024-12-13T14:29:40Z

themes/bootstrap5/js/common.js

@@ -293,46 +348,35 @@ var VuFind = (function VuFind() {
   * @param {string}  property Target property ('innerHTML', 'outerHTML' or '' for no HTML update)
   */
  function setElementContents(elm, html, attrs = {}, property = 'innerHTML') {
-    // Extract any scripts from the HTML and add them separately so that they are executed properly:
+    const tmpDiv = el('div', '', {}, stringToNodes(html));


It feels a little weird that we use stringToNodes here to create a div and pull out its contents, and then we use el to create a different div and put those elements back inside... and then down below, we pull the elements back out of the second div on various code paths. Would it be better to refactor this to just use the result of stringToNodes directly without creating a second container? I realize this would prevent querySelectorAll from being used below, but is there perhaps some other way of filtering the array to achieve the same effect? Or, since stringToNodes is only used in one place, should we consider defining it differently or giving it an argument to just avoid stripping the container in the first place?

hmm, could try and have a look!

Hmm, backtracked a littlebit here, as stringToNodes was used in only this place and added the assignment to innerhtml here. Idea is that there is one path to use and if any sanitization is required then its easier to manage the content when its all set with the same function.

demiankatz

Thanks for the simplification, @LuomaJuha. This looks pretty clear to me now, and all tests are passing, so I'm merging it!

LuomaJuha added 3 commits November 7, 2024 16:17

Removed innerHTML assignments from js code

15dc200

Search results with js works again

c9efd2b

Adjusted returns comment

941854e

demiankatz requested changes Nov 7, 2024

View reviewed changes

Merge branch 'villanova-dev' into remove-calls-to-innerhtml

67a1155

LuomaJuha requested a review from demiankatz November 12, 2024 11:31

demiankatz requested changes Nov 12, 2024

View reviewed changes

LuomaJuha added 3 commits November 12, 2024 17:09

Adjusted string to nodes to be internal function, no reason to export it

e2a61b8

Fixed script appending

31eccbc

Fixed qr code image appending

631eb06

demiankatz added the architecture pull requests that involve significant refactoring / architectural changes label Nov 12, 2024

demiankatz added this to the 11.0 milestone Nov 12, 2024

demiankatz approved these changes Nov 12, 2024

View reviewed changes

demiankatz requested a review from crhallberg November 12, 2024 21:09

crhallberg requested changes Nov 12, 2024

View reviewed changes

demiankatz mentioned this pull request Nov 13, 2024

Disable default cover images #4061

Merged

LuomaJuha added 8 commits December 12, 2024 13:28

Adjusted from reviews, added El function for element creation

be8d0dc

Merge branch 'villanova-dev' into remove-calls-to-innerhtml

35e508d

Reverted html creator function, added polyfills.js

1de8939

Adjusted qrcodelinks

2c109b2

Removed last changes from using vufind.el

91b2a0e

Fix qr code not applying properply

e7f2fd4

Revert remove <br> tag

a8b68c7

Readded element creation function

7912542

LuomaJuha requested review from crhallberg and demiankatz December 13, 2024 10:31

demiankatz requested changes Dec 13, 2024

View reviewed changes

crhallberg approved these changes Dec 16, 2024

View reviewed changes

Removed stringToNodes

46b48ce

LuomaJuha requested a review from demiankatz December 17, 2024 13:28

demiankatz approved these changes Dec 17, 2024

View reviewed changes

demiankatz merged commit 9799997 into vufind-org:dev Dec 17, 2024
6 checks passed

demiankatz mentioned this pull request Jan 8, 2025

Fix broken combined search jump menu. #4180

Merged

ThoWagen mentioned this pull request Jan 9, 2025

Generalize DOI linker system to support other identifier types #3918

Open

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove innerHTML assignments from js #4065

Remove innerHTML assignments from js #4065

LuomaJuha commented Nov 7, 2024 •

edited

Loading

demiankatz left a comment

LuomaJuha commented Nov 8, 2024

demiankatz left a comment

demiankatz Nov 12, 2024

LuomaJuha Nov 13, 2024

demiankatz left a comment

crhallberg left a comment

crhallberg Nov 12, 2024

LuomaJuha Dec 12, 2024

demiankatz Dec 12, 2024

EreMaijala Dec 12, 2024

EreMaijala Dec 12, 2024

crhallberg Nov 12, 2024

crhallberg Nov 12, 2024

crhallberg Nov 12, 2024

demiankatz Nov 13, 2024

LuomaJuha Nov 13, 2024

crhallberg Nov 12, 2024

LuomaJuha commented Dec 11, 2024

demiankatz left a comment

demiankatz Dec 13, 2024

LuomaJuha Dec 16, 2024

LuomaJuha Dec 17, 2024

demiankatz left a comment

Remove innerHTML assignments from js #4065

Remove innerHTML assignments from js #4065

Conversation

LuomaJuha commented Nov 7, 2024 • edited Loading

demiankatz left a comment

Choose a reason for hiding this comment

LuomaJuha commented Nov 8, 2024

demiankatz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demiankatz left a comment

Choose a reason for hiding this comment

crhallberg left a 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

Choose a reason for hiding this comment

LuomaJuha commented Dec 11, 2024

demiankatz left a 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

demiankatz left a comment

Choose a reason for hiding this comment

LuomaJuha commented Nov 7, 2024 •

edited

Loading