- Issue created by @Anybody
- Merge request !163Issue #3532249 by anybody: Uncaught TypeError: toolbarElement.querySelector(...) is null → (Merged) created by Anybody
- 🇩🇪Germany Anybody Porta Westfalica
Added a check for the querySelector() elements to be present.
- 🇫🇷France dydave
Thanks a lot Julian (@anybody) for raising this issue and contributing the necessary code changes to fix it 🙏
The resulting error is documented in the issue summary, but I would personally be quite curious to know how you were able to trigger it.
Could you please provide a bit more information on the context or setup and update the following section of the issue summary?
Steps to reproduce
I "guess" the IDs could have been changed in the theme, or with some other potential overrides, but I would have thought the dependency to the Toolbar module should ensure the IDs should be present in the HTML markup of the page (?!) 🤔
Otherwise, I have reviewed and tested locally the changes in the merge request and they seem to work fine, so great job! 👍
However, since I was unable to reproduce the error in the first place, I'm not sure whether the patch really fixes the issue 😅
Therefore, to force the error, I added a line of JS to change the HTML ID of the element:
For testing, added locally here:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/js/admin_too...document.getElementById('toolbar-bar').id = 'testing123a';
On 3.x got the following error:
drupal.js?v=11.2.0:64 Uncaught TypeError: Cannot read properties of null (reading 'insertAdjacentHTML') at admin_toolbar.toggle_shortcut.js?sywaq9:70:53 at Array.forEach (<anonymous>) at Object.attach (admin_toolbar.toggle_shortcut.js?sywaq9:21:62) at drupal.js?v=11.2.0:166:24 at Array.forEach (<anonymous>) at Drupal.attachBehaviors (drupal.js?v=11.2.0:162:34) at big_pipe.js?v=11.2.0:152:10 at big_pipe.js?v=11.2.0:183:3
Tested on branch:
3532249-uncaught-typeerror
from the MR and the error disappeared.
The toggle button is not displayed and the error seems to have been fixed.We don't have any tests in place for this, so I don't see any more related changes that should be added to this MR.
Therefore, I'm going to add a quick unrelated commit to this merge request and get it merged ASAP 👌
Any feedback, comments or questions would be greatly appreciated!
Thanks again very much for your help Julian (@anybody)! 🙂
- 🇫🇷France dydave
OK, I thought while we were at it, we might as well check for
toolbarElement
:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/js/admin_too...So I wrapped the whole block in a
if
statement, just like you did for the other ones.Tested locally with the same trick mentioned above at #4 and it seemed to work fine: Fixed the error as well if the ID
toolbar-administration
is changed:document.getElementById('toolbar-administration').id = 'testing123a';
Same results as above.
Added an extra commit from: #3527462-10: Import of strings skipped due to malformed HTML tag
→ , see: #3525938-7: [Meta] Roadplan for Admin Toolbar 3.7 → .
And the.eslintignore
file from 📌 GitlabCI: Fix ESLINT validation errors Active .After fixing a Stylelint validation error, the tests on the Merge request came back green 🟢
Therefore, I went ahead and merged the changes above at #5 🥳
Since I don't see any follow-up to this issue at this point, it could probably be marked as Fixed for now.
Feel free to let us know if you have any comments, suggestions or concerns on any aspects of the committed changes or this ticket in general, we would surely be glad to help.
Thanks again very much Julian (@anybody)! Automatically closed - issue fixed for 2 weeks with no activity.