Fix `pat-autotoc` fieldset ID and refactor TOC navigation

[petschki]

@cihanandac I had to refactor the id generation because I've overseen that the PR #1468 rewrites the fieldset ids with auto-generated values (which we only want if we have vertical TOC or colliding fieldset Ids) ... see https://jenkins.plone.org/job/pull-request-6.1-3.10/974/ for robot test details, which depend on various fieldset Ids (eg #fieldset-settings)

[cihanandac]

I was also not aware that there are tests that are checking for the fieldset ids :) I have tried this version and it seems like it correctly matches the tab ids with the fieldset counter parts.

Also, refactored parts looks good and easier to read.

Thanks @petschki

[petschki]

OK, somehow this autotoc implementation confused me ... sorted some things for clarification:

The old "autotab" implementation iterates over the levels selector, generates a nav > a tab element with a generated unique ID and adds a click event which is linked to the corresponding section visibility. So the "tab#id" and the "section#id" are completely separated of each other. Furthermore the section#id was untouched (this is mandatory for some robottests!) The generated tab markup looked like this:

A bit strange, that the href of each tab points to its own id.

Now we have the Bootstrap markup implementation (https://getbootstrap.com/docs/5.3/components/navs-tabs/#javascript-behavior) ... we need to add things like data-bs- and aria- attributes so that we can use Bootstrap.Tab library.

So this markup

<form>
<fieldset id="fieldset-default">
<legend id="fieldsetlegend-default">Default</legend>
</fieldset>
</form>

should end in

<form>
<nav>
<ul class="nav nav-tabs" id="myTab" role="tablist">
  <li class="nav-item" role="presentation">
    <a href="#fieldset-default" class="nav-link active" id="autotoc-item-autotoc-0-tab" data-bs-toggle="tab" data-bs-target="#fieldset-default"  role="tab" aria-controls="fieldset-default" aria-selected="true">Default</button>
  </li>
</ul>
</nav>
<div class="tab-content" id="myTabContent">
  <fieldset class="tab-pane fade show active" id="fieldset-default" role="tabpanel" aria-labelledby="autotoc-item-autotoc-0-tab" tabindex="0">...</div>
</div>
<form>

Thats straight forward, but currently, there are also robottests which rely on the "old" autogenerated paneID selector (see https://github.com/plone/Products.CMFPlone/blob/master/Products/CMFPlone/tests/robot/test_contentbrowser.robot#L126) which needs to be changed accordingly (/cc @1letter)

Theres one more thing: this old issue with the nested tabs and non-unique ID problem https://github.com/plone/mockup/issues/1242 ... maybe someone has an idea how we could handle this?

[1letter]

I have here a Draft PR for robottest fixes. But i would wait for the final markup decision and implementation.

https://github.com/plone/Products.CMFPlone/pull/4197

[petschki]

I have now reverted the autotoc merge and released mockup 5.4.2 ... this branch is now rebased with the changes from #1468 and I will continue to work on the autotoc fixes.

staticresources with the tinymce fixes from #1476 will get released soon when https://github.com/plone/plone.staticresources/pull/385 is green.

[petschki]

I've now refactored this a bit to not break the current id generation.

Regarding nested autotocs: this is not easy to solve automaticall inside the pattern, because the pattern parsing goes "inside out" -> the deepest pattern gets rendered first. So we do not know in which section of the parent autotoc pattern we are and cannot generate a unique ID accordingly.

The @@theming-controlpanel is an example for the nested autotoc. My proposal to fix this is to add the IDPrefix option explicitely to the "sub autotoc" in order to get unique tab ids. The only question then is, how to directly access the tab of the sub autotoc with the browser hash 🤨

[petschki]

As usual: the new autotoc implementation had some sideeffects on schemaeditor ... see schemaeditor--implementation.js and https://github.com/plone/plone.schemaeditor/pull/139

luckily there were some robottests which complained ;)

[cihanandac]

@petschki I can see that your last commits fixes the fieldset ids' problem. Old fieldset ids' are kept and they are correctly linked from the tabs 🎊

About nested autotocs: I have also tried to implement the prefix solution by adding the elements' index but the browser hashes are indeed not working on the nested elements. I have realized this problem after adding the commit 🤯, thus I have removed this commit but basically if was like the following:

const autotocInstances = $(".pat-autotoc"); self.instanceIndex = autotocInstances.index(self.$el);

[petschki]

@cihanandac I have a basic idea how to solve this.

It will work if we add the parent tabID to the IDPrefix of the subtabs ... then we can check with browserHash.indexOf(tabID) == 0 if the browser hash starts with the current tab Id.

But this means, that we need the parent tabID in the options.IDPrefix of the sub autotoc and this cannot be done automatically in the pattern. I'm fine with that, as long as we document how to achieve this.

For example the theming controlpanel could then look like this:

If we update the options here https://github.com/plone/plone.app.theming/blob/master/src/plone/app/theming/browser/controlpanel.pt#L336 to

data-pat-autotoc="section:fieldset;levels:legend;IDPrefix:autotoc-item-autotoc-1-subitem-"

when you then click on the subitem the browser hash is #autotoc-item-autotoc-1-subitem-autotoc-2 and we can mark parent and sub tab as active.