Change the title attribute of summary if details is open

Question

I have this JavaScript code that functions as intended, although as a beginner, I do not know of a shorter, more efficient way of writing it.

The code is a simple toggle that changes the title attribute of the <summary> HTML tag from hide to show when the user clicks on the <summary> to open the <details>.

I used loops since there are several of these in my page:

var i,l;
var detailsElem = document.getElementsByTagName('details'),
summaryElem = document.getElementsByTagName('summary'),
summElemLen = summaryElem.length;
detlElemLen = detailsElem.length;
function detailsToggle() {
 for (i = 0; i < summElemLen; i++) {
 if (detailsElem[i].open === true) {
 summaryElem[i].title = 'hide';
 }
 else {
 summaryElem[i].title = 'show';
 }
 }
}
for (i = 0; i < detlElemLen; i++) {
 detailsElem[i].ontoggle = function() {detailsToggle()};
}

Is there a less verbose way of achieving the same function?

Answer 1

detailsElem[i].ontoggle = function() {detailsToggle()};

It's better to use addEventListener. The problem is that doing it this way replaces anything assigned to ontoggle, potentially blowing away any handler put by other code.

var detailsElem = document.getElementsByTagName('details'),
summaryElem = document.getElementsByTagName('summary'),
summElemLen = summaryElem.length;
detlElemLen = detailsElem.length;

Personally recommending a var per variable. That way, you can easily add/remove lines without worrying about continuation. One such example is summElemLen ending with a ; and not a ,. This will cause detlElemLen to be defined on the global scope, and open to collision. This minor oversight can cause a lot of headache in the long term.

Also, using getElementsByTagName is very broad. Do you really want to take all elements? Or just a subset? If it's the latter, use a class name instead.

if (detailsElem[i].open === true) {

if statements expect a boolean. detailsElem[i].open is already a boolean. No need to do === true, it's redundant. true === true is always a true.

Lastly, one flaw in the code is that once you toggle one detail element, you toggle all summary titles regardless of whether they belong to the element you clicked.

---

The following is what I consider shorter, more explicit and takes advantage of the array API. We get all detail elements, loop through and attach a handler. Handler checks if a summary is present and only sets a title if there is one.

const slice = Array.prototype.slice
// querySelectorAll to select elements using a CSS selector.
// It returns an array-like structure (indexed, has length, no array API)
// To take advantage of array APIs, we use slice to convert to array.
const details = slice.call(document.querySelectorAll('.detail'))
// Use forEach to loop through each detail element found
details.forEach(detail => {
 // Add event handler when toggle is fired
 detail.addEventListener('toggle', event => {
 // Look for the summary with querySelector. It's like querySelectorAll
 // Except it returns either an element or null, instead of an array.
 const summary = detail.querySelector('summary')
 // No summary, do nothing.
 if(!summary) return;
 // Otherwise, assign title depending on detail state.
 summary.title = detail.open ? 'hide' : 'show'
 })
})

• Blindman67: Why use Array.slice? Would it not be more concise to use the spread operator? eg const details = [...document.querySelectorAll(query)];
• Joseph: @Blindman67 old habits.