Feat: Treeview: open by default

[lnorby]

Summary

I would like to use the Treeview in a CMS, for a multi-level sidebar navigation, where the active page is highlighted. If the page is deep inside the menu, I want all the parent items opened by default.

Solution

Maybe the aria-expanded="true" attribute in the html should not be ignored.

[NickDJM]

@lnorby What you do mean by the aria-expanded value shouldn't be ignored?

[lnorby]

I mean, if I have an html code like this:

<ul id="example-menu">
  <li><a href="/about">About</a></li>
  <li class="dropdown">
    <a href="#" aria-expanded="true">Projects ▼</a>
    <ul>
      <li><a href="/projects/awesome">Awesome project</a></li>
      <li><a href="/projects/not-so-awesome">Not-so-awesome project</a></li>
    </ul>
  </li>
  <li><a href="/contact">Contact me</a></li>
</ul>

The submenu of "Projects" should be open by default. Now it is ignored and overwritten.

But it's just my idea, if you have a better one, you are welcome.

[NickDJM]

Oh I get it! I like that idea. It shouldn't be hard to implement either.

When the toggles initialize they can just check to see if they already have that property set and open themselves if they do.

[NickDJM]

@lnorby PRs are welcome if you want to take a shot at this. Otherwise I can take a look once I am done working on #242.

[lnorby]

I guess the initialize function in the BaseMenuToggle class needs to be modified like this:

initialize() {
  // Add WAI-ARIA properties.
  this.dom.toggle.setAttribute("aria-haspopup", "true");
  
  if (!this.dom.toggle.hasAttribute("aria-expanded")) {
    this.dom.toggle.setAttribute("aria-expanded", "false");
  }

  // If the toggle element is a button, there's no need to add a role.
  if (!isTag("button", { toggle: this.dom.toggle })) {
    this.dom.toggle.setAttribute("role", "button");
  }

  // Ensure both toggle and menu have IDs.
  if (
    this.dom.toggle.id === "" ||
    this.elements.controlledMenu.dom.menu.id === ""
  ) {
    const randomString = Math.random()
      .toString(36)
      .replace(/[^a-z]+/g, "")
      .substr(0, 10);

    let id = this.dom.toggle.innerText.replace(/[^a-zA-Z0-9\s]/g, "");
    let finalID = randomString;

    if (
      !id.replace(/\s/g, "").length &&
      this.dom.toggle.getAttribute("aria-label")
    ) {
      id = this.dom.toggle
        .getAttribute("aria-label")
        .replace(/[^a-zA-Z0-9\s]/g, "");
    }

    if (id.replace(/\s/g, "").length > 0) {
      id = id.toLowerCase().replace(/\s+/g, "-");

      if (id.startsWith("-")) {
        id = id.substring(1);
      }

      if (id.endsWith("-")) {
        id = id.slice(0, -1);
      }

      finalID = `${id}-${finalID}`;
    }

    this.dom.toggle.id = this.dom.toggle.id || `${finalID}-menu-button`;
    this.elements.controlledMenu.dom.menu.id =
      this.elements.controlledMenu.dom.menu.id || `${finalID}-menu`;
  }

  // Set up proper aria label and control.
  this.elements.controlledMenu.dom.menu.setAttribute(
    "aria-labelledby",
    this.dom.toggle.id
  );
  this.dom.toggle.setAttribute(
    "aria-controls",
    this.elements.controlledMenu.dom.menu.id
  );

  if (this.dom.toggle.getAttribute("aria-expanded") === "true") {
    // Make sure the menu is expanded on initialization, but do not emit the expand event.
    this._expand(false);
  } else {
    // Make sure the menu is collapsed on initialization, but do not emit the collapse event.
    this._collapse(false);
  }
}

The problem with this simple approach is that now both the MenubarToggle and the DisclosureMenuToggle classes check the aria-expanded attribute as well, which I think is not desired.

The TreeviewToggle class should use it's own initialize function, but to avoid code duplication (id generation and setting other default aria attributes), the initialize function in the BaseMenuToggle class needs to be sliced up into separate, reusable functions.

I don't want to make such modifications in your code, so I guess I will leave it to you. :)

[NickDJM]

Fair enough, with some of the code duplication that happens it's pretty small- this however would be a nightmare to keep consistent with the base class.

I can probably split it out into something to setup the aria attributes/roles/ids, then the initialize method would just call that and then see if it needs to expand/collapse based on the aria-expanded value.

[NickDJM]

Or maybe having a method for ID generation and a method for aria/roles would make it more future-proof.

[NickDJM]

I have some time to look into this now. I'll see about splitting the methods mentioned above out and then we'll check for aria-expanded to see if a menu should open or not.

[NickDJM]

So I've been looking into this over the past few days. There are a few things to think about before we implement this:

1. What happens if you add aria-expanded="true" to a submenu toggle when the parent menu is not open?
2. How does this work with the main menu controller?
3. How does this all work with the standard open/close tab order with the treeview?
4. Do we just implement this functionality into all menus? I could see if being handy.

When I have more time I'll look into this more, but for now I have the bandwidth to make the breaking change required (splitting the initialize function into separate methods). Once this is done I'll merge it, since I'd like the get a release for 4.0.0 out this month.

The feature allowing aria-expanded to control the open status on load might need to be a 4.1.0 feature.

[NickDJM]

To answer some questions I had after further exploring this:

1. I don't think it matters if you add aria-expanded to a submenu toggle when the parent menu isn't open. Treeviews allow for this anyways, so it should be fine to just respect it.
2. Still requires investigation.
3. It appears it doesn't break anything in treeviews- however it seems to super break other menu types.
4. As stated in 3, this feature seemed nice for all menus, but there's too many factors to take into account for little benefit. It will only be implemented in Treeviews as the original issue states.

I have a functioning version in #284 but I still need time to figure out all the ins and outs of it and I'll need to write new tests for treeviews.

It looks like I might have some time to work on this more in the coming weeks, but things might change.

[NickDJM]

@lnorby if you need this feature ASAP, you can apply the PR diff to your project: https://patch-diff.githubusercontent.com/raw/NickDJM/accessible-menu/pull/284.diff

Just know I haven't fully tested it, so it might be buggy/change over time.

[NickDJM]

This is now merged. It will go out in the next release.

[NickDJM]

v4.1.0 is out and includes this. Closing.