Remove duplicate code from veritcal-tabs.js in Claro

Postponed on #1936708: Current element values missing from vertical tabs when shown in 2-column layout and #3081500: Accessibility bugs with vertical tabs as these may lead to structural changes that would make any work prior to that potentially useless.

Discussed with @bnjmnm and @ckrina that we shouldn't block this on #3081500: Accessibility bugs with vertical tabs.

Is there a reason js- prefixed classes have to be used in this scenario? @nod_ advised in olivero to use data-attributes for javascript selectors where possible:

we prefer using data- attribute for JS related selection, that way themers are free to do whatever they need with classes without breaking the JS. It came up a few times the past weeks so something must not be clear on core side.

Essentially: js- prefix: better than a "simple" class, dedicated data- attribute = best.

As for the MR from #1, changing just the javascript files won't work, you'll also need to edit the twig markup to add those classes in as well. Additionally, there are no Drupal.theme functions added. This still has quite a ways to go before being ready for review.

Done & ready for review

I also don't like the idea to add processing classnames (js prefixed, or processing data-attributes) into twig template

I'm just thinking that data attributes like data-olivero-tabs or other similar which were hardcoded in twig template, with disabled javascript these attributes gives nothing

Normally we can catch needed classname on js side and provide all related js prefixed classnames or data-attributes which are necessary to have for js functionality

I wanted to point out that the reason this is a Claro stable blocker is that Claro has a copy of the vertical tabs JavaScript. The goal of this issue is to be able to remove that, except what comes to the markup overrides. We should probably update the title and issue summary to make that clear.

Tests are passing! I reviewed the code (I only modified the tests in the MR), and tested the vertical tabs in Seven and Claro.

I did find a preexisting issue, but will open a separate issue against that.

All looks good!

I think it's not apparent based on the issue summary / title, but the goal of this issue is that all of the vertical tabs logic is duplicated in Claro. On top of the changes here, we need to get rid of the duplicated code and ensure that works.

+1 the "needs issue summary update". Among other things, it is specifically requesting js- prefixed classes, but data attributes are fine as well. It may be worth clarifying that the presentation classes can remain intact, we just want to add js-prefix or data attributes for functionality.

This also strips out the majority of Claro's vertical tabs logic, and I think much of it still needs to be there. My comments on the MR elaborate on some of this, but the top of core/themes/claro/js/vertical-tabs.es6.js describes all the fixes provided. I'm not sure that much can be removed until the issues fixed in that file are addressed in core, but a more targeted extraction can certainly happen.

There's a meta tracking most/all(?) of the issues that would need to land for the big Claro extract
👉 #3081521: Refactor vertical tabs, remove vertical-tabs.js replacement when core issues are resolved

We need to update the issue summary with the latest approach for this. I closed #3081521: Refactor vertical tabs, remove vertical-tabs.js replacement when core issues are resolved based on discussion with @bnjmnm, @ckrina, @saschaeggi and @Gábor Hojtsy to focus on doing the minimum required work for removing the duplicate code from vertical-tabs.js in Claro – even if that means that we are causing accessibility regressions within Claro, so long as the experience takes the vertical tabs on par with the core experience. The accessibility issues with vertical tabs are not specific to Claro, and therefore they should not have an impact on Claro's stability.

If we end up removing the bespoke Claro vertical tab code, then we can probably also close #3272316: Claro's vertical tabs' aria states don't always properly reflect state at mobile

Re #28 that jogged my memory and you're right, we decided Claro regressing a bit is fine as long as it does not introduce problems not present in other themes.

I mention in the thread, but there is one bug introduced by the full removal that should be accounted for (unless it turns out to be an issue with my review instance). With this MR, if I load a page with vertical tabs on a wide viewport, then reduce to narrow, the inactive vertical tabs disappear entirely. I suspect a bit of CSS could address that, however.

HELLOOOOO from DrupalCon! :D

I was asked to weigh in here by @lauriii and @ckrina, after having the situation explained to me.

I'm in favour of removing the vertical-tabs.js overrides because a) it sounds like according to @mherchel they cause some ARIA problems, and b) falling back to default core behaviour sounds like the quickest way to get us stable Claro fastest. BUT it would be great not to lose those improvements, so if we could spin off a sub-issue for them to pursue after Claro's stable, that sounds great.

Review and tested. Looks good with no regressions vs Seven.

RTBC per tests passing! 🎉

Committed dffe17c and pushed to 10.0.x and 9.4.x. Thanks!

🥳🥳🥳