Individual collapsible areas with accessibility in mind

Question

I've been learning Javascript by creating reusable individually collapsible areas (not using jQuery) and I was wondering if someone could review it and let me know if there is anything they would do differently?

Also it would be good if it was usable for screen readers so any usability feedback would be appreciated. If this requires using more CSS and less JS then examples would be helpful.

https://codepen.io/mandrewnz/pen/QgKyjq

HTML

<div class="js-collapsible-region closed">
    <h2>Title</h2>
    <p>Content goes here</p>
    <div>
        <button class="js-collapsible-btn">Click to show some text</button>
    </div>
    <div class="collapsible-area">
        <p>Suprise</p>
    </div>
</div>

JS

/* adding classlist.js polyfill separately */

var btn = document.getElementsByClassName('js-collapsible-btn');

function findAncestor (el, cls) {
    while ((el = el.parentElement) && !el.classList.contains(cls));
    return el;
}

var openHolder = function(e) {
    var region = findAncestor(this, 'js-collapsible-region'),
        hiddenText = this.childNodes[1];

    // set text to reflect collapsible holder status
    hiddenText.textContent == "(collapsed)" ? hiddenText.textContent = "(expanded)" : hiddenText.textContent = "(collapsed)";

    if (region.classList.contains('closed')) {
        region.classList.remove('closed');
        region.classList.add('open');
    } else {
        region.classList.remove('open');
        region.classList.add('closed');
    }
};

// Create visually hidden text in each link to indicate state
for (var i = 0; i < btn.length; i++) {
    var span = document.createElement('span');
    var currentBtn = btn[i];

    span.classList.add('visuallyhidden');

    span.innerHTML = '(collapsed)';
    currentBtn.appendChild(span);

    // add click event to open holder
    btn[i].addEventListener('click', openHolder, false);
}

Answer 1

Everything looks great, and I can see what you are trying to achieve.

Only things I can recommend is to clean your code, make it as simple as possible, to make it easy to read and come back on later.

For example this line here:

while ((el = el.parentElement) && !el.classList.contains(cls));

In the way it is used, it's okay, but it doesn't explain well what the intention is, instead, try to be more "if this, then", very straight forward. Instead of using a while you could use recursion instead, removing more confusion.

Another thing is, you may want to expand your variable names. You probably will put this code through a minifier either way, so no need to keep things short.

function findAncestor(element, findClass) {
   if (element.classList.contains(findClass)) {
      return element;
   }
   if (!element.parentElement) {
      // Did we hit body?
      return false;
   }
   // We didn't find it, so check the parent.
   return findAncestor(element.parentElement, findClass);
}

Now we can read this function, and know exactly whats happening, without making any assumptions.

The next function is great, and works well, but we could make this more accessible using the [hidden] attribute along with a .hidden class, this is so we don't need to worry about compatibility.

We only want to use the parent as a light wrapper, from that parent we can find the child we actually want to show/hide

You can do a bit of reading on [hidden] here https://www.paciellogroup.com/blog/2012/05/html5-accessibility-chops-hidden-and-aria-hidden/

function openHolder() {
   var region = findAncestor(this, 'js-collapsible-region');
   var content = region.querySelector('.collapsible-area');

   if (content.classList.contains('hidden')) {
      content.classList.remove('hidden');
      content.removeAttribute('hidden');
   } else {
      content.classList.add('hidden');
      content.setAttribute('hidden', '');
   }
}

We also want to leave state out of the DOM if it isn't visual state (like we can check if something is hidden, but we could also track that another way).

We then simply need to add our even listeners on like we did before:

var btn = document.getElementsByClassName('js-collapsible-btn');
for (var i = 0; i < btn.length; i++) {
    // add click event to open holder
    btn[i].addEventListener('click', openHolder, false);
}

We can also change this to a "functional" approach to remove the loop (which we needed to handle manually)

Array.from(document.getElementsByClassName('js-collapsible-btn'))
   .forEach(function(element) {
      element.addEventListener('click', openHolder, false);
   });

The last thing we should do, is instantly tell the screen reader is hidden but having the content marked as hidden to start with:

<div class="js-collapsible-region">
    <h2>Title</h2>
    <p>Content goes here</p>
    <div>
        <button class="js-collapsible-btn">Click to show some text</button>
    </div>
    <div class="collapsible-area hidden" hidden>
        <p>Suprise</p>
    </div>
</div>

<div class="js-collapsible-region">
    <h2>Title</h2>
    <p>Content goes here</p>
    <div>
        <button class="js-collapsible-btn">Click to show some text</button>
    </div>
    <div class="collapsible-area hidden" hidden>
        <p>Suprise</p>
    </div>
</div>

We also will need to change the styles to match

.js-collapsible-region .collapsible-area {
    overflow: hidden;
}

.js-collapsible-region .collapsible-area.hidden {
    height: 0px !important;
}

.js-collapsible-region .collapsible-area:not(.hidden) {
    transition: height .3s linear;
}

You should note that the attribute hidden applies the styles display: none, which you may not want, so you can change your styles to override the way you want them to work.

Here is the resulting codepen https://codepen.io/anon/pen/KqgmNY

For some fun, since we are using ES6 with classList anyway, have a look at this version https://codepen.io/anon/pen/mwrmpL

Answer 2

Did you consider using one class on the container - like closed and having a CSS ruleset that hides the inner content whenever that class is applied? If you did that, then you wouldn't have to add and then later remove the open class.

---

The following line looks a bit complex:

hiddenText.textContent == "(collapsed)" ? hiddenText.textContent = "(expanded)" : hiddenText.textContent = "(collapsed)";

This could be simplified by moving the assignment outside of the ternary operator:

hiddenText.textContent = hiddenText.textContent == "(collapsed)" ? "(expanded)" : "(collapsed)";

---

You should be able to simplify the add/remove class method calls by utilizing Element.classList.toggle()

---

If you utilize ecmascript-6 features like FabianCook's answer suggested, it may be wise to consider performance. For example, instead of calling Array.from() to put items into an array, use the spread syntax. And instead of using a traditional for loop, use a for...of loop to avoid having to use the bracket syntax to reference elements in the array/collection.