Remove accordion icon added by JS and add to twig instead

[AWearring]

The Accordion Pane icons are currently being added using Javascript, this strikes me as being bad practice therefore this PR removes them from being added via localgov-accordion.js and instead adds them via the Accordion Pane Twig file instead (paragraph--localgov-accordion-pane.html.twig)

See issue #199

[ctorgalson]

@AWearring I'm completely on-board with the idea: it'd make the whole accordion a lot simpler to style and adapt to individual designs.

However, this change alters the markup by making the icon a sibling of the <button> where it's currently a child.

This has the potential break existing CSS customisation, and to silently introduce (visual) accessibility issues by breaking existing :hover / :focus styles.

The approach I usually adopt for this kind of thing is something like this (simplified) code;

{# Twig #}
<div class="accordion-pane__title">
  {{ heading_text }}
  {# The button starts out hidden #}
  <button hidden aria-expanded="false" aria-controls="{{ control_id }}">
    {{ heading_text }}
    <span class='accordion-icon' aria-hidden="true"></span>
  </button>
</div>

// Javascript
init: function init(accordion, index) {
  // Un-hide the button.
  button.hidden = false;
  
  // Rest of init() routine...
}

Unfortunately, my approach is a thread that, if you tug at it, starts unraveling things: for example, we'd then need to sync the value of the button's id attribute with the individual pane's some way or other (both in Twig, both still in javascript, or one in Twig and one in javascript).

If it's of interest, I've done a bit of work on a proof of concept that includes this component and solves this specific problem re: localgovdrupal/localgov_base#600 to try to address a related problem. There's no public code today, but I'm hoping to talk about it for a few minutes on Thursday's Technical Drop in call.

[AWearring]

@ctorgalson Thanks - I can see the issue and I can see it is much bigger than I first realised.

The main reason for me looking into it is that on the 3 sites I maintain we already have an icon included in our twig templates and the pending v1.7.0 localgov_base release would have automatically added the new icons to our sites - I've now got round this immediate problem by patching out the line of JS adding the icons. We will need a better and more permanent solution.

[AWearring]

@ctorgalson
I've updated the PR so that the code for the button is now also in the twig template, leaving the JS to handle adding the id to the buttons

{% block paragraph %}
  <div{{ attributes.addClass(classes) }}>
    {% block content %}
      <div class="accordion-pane__title">
        <{{ heading_level }}>
          <button aria-expanded="false">
            {{ heading_text }}
            <span class='accordion-icon' aria-hidden='true'></span>
          </button>
        </{{ heading_level }}>
      </div>
      <div class="accordion-pane__content">
        {{ content.localgov_body_text }}
      </div>
    {% endblock %}
  </div>
{% endblock paragraph %}

I believe this will work and not break any existing CSS customisation.

[AWearring]

@markconroy @ekes @finnlewis
If this PR is [now] acceptable I think that it should be merged and approved before localgov_base v1.7.0 (localgovdrupal/localgov_base#622) is released as that release will add these icons to all sites whether they are wanted or not - even if a site has overwritten the twig template with its own icons (as in the case for West Lindsey, North Kesteven and Rutland).

[ctorgalson]

@AWearring I was just looking. I can't test until the morning, but I think this almost perfectly duplicates what the js does with two small remaining exceptions.

The first omission is that the button is not hidden for users who might not have js enabled (or if some other js on the page breaks this script). This is a potential problem in that situation because the button has an aria-expanded attribute that doesn't correspond to anything that a user who depends on them can actually do.

As I mentioned above, I usually handle this by adding the hidden attribute to the button, and button.hidden = false; at the beginning of the javascript. There are other ways to handle it of course, but (I just remembered!) this is what I did in my recent changes to this module:

• Twig
• Javascript

The second thing, also very minor, is because of how the Accordion (and its javascript) is used alongside Tabs (and their javascript), it's necessary to un-do everything that you do in create() in destroy().

[AWearring]

@ctorgalson Thanks for your comments

• I've added hidden to the button in twig
• Added button.hidden = false; to JS
• Amended destroy() to un-do all the changes I made in create()

However, I'm not sure adding hidden to the button actually does anything as the button will always be visible with or without JS turned on and with JS turned off the accordion panes are all open. If it is required that the button doesn't exist without JS I'm not sure how this can be achieved.

[ctorgalson]

@AWearring

Your changes look good.

However, I'm not sure adding hidden to the button actually does anything as the button will always be visible with or without JS turned on and with JS turned off the accordion panes are all open. If it is required that the button doesn't exist without JS I'm not sure how this can be achieved.

I can't take a look until tomorrow, but browsers do hide items with hidden by default, so this is almost certainly a rule like this in CSS somewhere:

button { display: block; }

...and can be corrected with something like this:

button:not([hidden]) { display: block; }

[AWearring]

@ctorgalson
Ah, Yes that makes sense. I couldn't work out why it was still showing even though it was hidden.

[AWearring]

@ctorgalson
In localgov_base CSS we have:

.accordion-pane__title button {
  display: inline-flex;
  align-items: center;
  justify-content: space-between;
}

So maybe we need to have the opposite to your suggestion:

.accordion-pane__title button[hidden] {
	display: none;
}

However the issue we'd then have with that is that we'd lose the Heading contained within the button too

[ctorgalson]

@AWearring Good catch regarding the need to hide the title text. AFAIK, that's really the only solution in this situation.

It does reveal a couple of potential issues though (sorry!):

1. lines 181-184 need to be removed altogether as no longer relevant (this would only have an effect if .destroy() ran--then the button would be hidden, but there would be no title text),
2. if a Twig template is in use where the heading title does not have that span.accordion-pane__heading wrapper, the javascript gets halfway through the first button/pane combo and then fails; I think the two suggestions I've made here fix that issue (this won't happen on sites that have not customized the template, but any site that has runs this risk).

[ctorgalson]

Suggested change

	heading.hidden = true;
	if (heading) {
	heading.hidden = true;
	}

[ctorgalson]

Suggested change

	button.hidden = true;
	button.hidden = true;
	if (heading) {
	heading.hidden = false;
	}

[AWearring]

@ctorgalson
Thanks
I also realised that if a customised Twig template is being used, in addition to the heading title potentially not having the span.accordion-pane__heading wrapper, the button won't be defined (as it would be expected that these would be generated by the JS). I think I have now mitigated against both of these scenarios. I have also removed lines 181-184.

[ctorgalson]

This all looks good to me with the caveat that some sites that have already customized the accordion pane template may see visible heading text alongside the buttons.

But as it's a reasonably minor side-effect, and quite easy to correct, I think this is a better solution overall than introducing more complexity to the js (like, e.g., detecting if the heading text is not already wrapped in a span, then wrapping/unwrapping it on create()/destroy()).

[AWearring]

@ctorgalson Thanks for your input and assistance.

In my testing using a copy the West Lindsey site which is using a customised Twig template, that is very close to the original only adding in a Font Awesome icon after the heading, the only visual issues are that all the panes are open and the Font Awesome icon is visible. I think I can live with that and as you say it is a fairly easy to correct. I believe this is better than the newly defined icons in localgov_base appearing unexpectedly.

[markconroy]

This looks good to me.

If we are moving to adding the icon via a twig template, I'd like us to add it as an inline SVG so we have more control over it with CSS. And maybe have the icon beside the button instead of inside the button, and the text inside the button instead of before the button ... but I think these are nice enhancements that we can consider in follow-up issues perhaps.

---

Thanks to Big Blue Door for sponsoring my time to work on this.

[ctorgalson]

And maybe have the icon beside the button

One vote slightly in favour of keeping the icon in the button for simplicity of styiing focus states (I know we can use :focus-within / :hover etc. on the parent wherever the icon ends up, so it's no hill to die on)..

[markconroy]

Ideally, we could in the near-future remove all this JS and just use the <details><summary> elements instead.