fix: guard TOC navigation against re-entry (#63)

[erseco]

Summary

Speculative fix for #63. Tagging this draft because I have not been able to reproduce the issue locally on Moodle 5.0, but the code path I'm patching is the only thing in module.js that matches every detail of the bug report.

Root-cause hypothesis

`exescorm_activate_item()` (the function that loads the SCO matching the clicked TOC entry) calls `exescorm_tree_node.closeAll()` and later `exescorm_tree_node.openAll()` to collapse and re-expand the whole YUI TreeView. Those calls mutate node selection state and YUI fires additional `select` events while the tree settles, which the `tree.after('select', ...)` handler then interprets as a fresh navigation request and calls `exescorm_activate_item()` again with a different node (typically a sibling).

That maps cleanly onto every detail Ignacio described:

• "in many cases you don't see the section you clicked on" → second `activate_item()` call replaces the iframe src.
• "the iframe content is loaded more than once: a page seems to load and then the content is refreshed" → exactly two iframe creations per click.
• "clicking 'a 2 1' loads 'a 2 2'" → next sibling is the most likely target of a spurious `select` after a closeAll/openAll on a parent.

It also explains why I can't reproduce on Moodle 5.0: YUI TreeView's exact event ordering changed between versions, and the spurious `select` may simply not fire on the build I tested.

Fix

Single-flag re-entry guard:

• `exescorm_navigating = true` set right before `closeAll()` (covers the `select()`/`closeAll()`/`openAll()` window).
• `tree.after('select', ...)` returns early when the flag is set, so only the genuine user click drives navigation.
• `exescorm_navigating = false` cleared right after `openAll()`.

17 added lines, no behavioural change for the non-buggy path.

Test plan

• Reproduce on Moodle 4.5 first: install the example SCORM ZIP, open it, click "a 2 1" and verify the iframe stops at `html/a-2-1.html` (no second load to `a-2-2.html`).
• Click each TOC entry once and verify the iframe loads exactly once per click (DevTools → Network).
• Click a parent that has children (e.g. "a 2"): the parent's content loads once and the tree stays expanded.
• Repeat the previous three checks on Moodle 5.0 to make sure the guard doesn't break the working flow.

Sibling repo

`mod_exeweb` doesn't share this code path (its viewer renders the package as a single iframe with no TOC), so no parallel PR is needed there. Per `AGENTS.md`'s twin-plugin checklist, audited `mod_exeweb/amd/src/editor_modal.js` and confirmed.

---

Moodle Playground Preview

The changes in this pull request can be previewed and tested using a Moodle Playground instance.

⚠️ The embedded eXeLearning editor is not included in this preview. You can install it from Modules > eXeLearning SCORM > Configure using the "Download & Install Editor" button. All other module features (ELPX upload, viewer, preview) work normally.

[ignaciogros]

The navigation menu works correctly, including keyboard support. Thank you!

But there are still some issues with the pagination buttons:

“Previous”, “Next”, and “Level up” work as expected, but “Previous within this level” and “Next within this level” do not.

Example:
Publish the sample SCORM and go to section “b”. The expected behavior when clicking “Next within this level” would be to navigate to section “c”, but this does not happen (in my tests, nothing happens). Additionally, when being on “b”, “Previous within this level” is disabled. This may be because index.html is not at the same level. In other cases, those buttons behave like “Previous” and “Next”, without respecting the hierarchical level.

If this becomes too problematic, I suggest temporarily hiding these buttons and releasing the next version with only “Previous” and “Next”.

There is also a JavaScript error when loading the page, but it does not appear to affect functionality:
[moodle-exe-bridge] Missing __MOODLE_EXE_CONFIG__

[ignaciogros]

Hi @erseco,

Could you please take a look at it? Navigation may be problematic. I think that if it is not possible to resolve this soon, it might be a good idea to hide the #nav_skipprev and #nav_skipnext buttons.

What do you think?

Thank you.

[erseco]

@ignaciogros thanks for the thorough testing — you were right, and I've now fixed the "within this level" buttons (#nav_skipprev / #nav_skipnext) rather than hiding them.

Root cause

This was a pre-existing bug, separate from the re-entry guard. exescorm_skipprev/exescorm_skipnext rely on prevsibling/nextsibling metadata in scoes_nav. That metadata is filled in by exescorm_update_siblings() in module.js, which grouped entries by treating each existing scoes_nav key as a candidate parentscoid.

The catch: top-level sections (a, b, c) have no parentscoid at all — they are the roots of the displayed organization, and the organization node itself has no launchable URL so it isn't in scoes_nav. They therefore never matched any group and never got prevsibling/nextsibling. That's exactly why:

• on b, "Next within this level" did nothing (no nextsibling → skipnext returned null), and
• "Previous within this level" was disabled (no prevsibling).

Deeper levels (e.g. a 1 / a 2 / a 3) did work because those children carry a parentscoid, which is why the behaviour looked inconsistent.

Fix

exescorm_update_siblings() now groups every entry by its parentscoid value, bucketing the parentless top-level roots together under a sentinel key, then links each group as siblings. Deeper levels produce the same groups as before, and exescorm_get_siblings() still only fills in undefined prevsibling/nextsibling, so server-computed links are preserved. The #63 re-entry guard is untouched.

Verified on Moodle 4.5 (Docker, your sample SCORM)

• On b → "Next within this level" now navigates to c, "Previous within this level" to a.
• a correctly has no previous-sibling target and c no next-sibling target (first/last at that level).
• Deeper-level "within this level" navigation still works.
• TOC menu (the Wrong content displayed when clicking on navigation links #63 fix) still loads the correct page once per click — no regression.

The [moodle-exe-bridge] Missing __MOODLE_EXE_CONFIG__ console line is unrelated to this code path (harmless, separate component) — happy to track it in its own issue.

Could you re-test when you have a moment? Re-requesting your review. Thanks!

[ignaciogros]

Thanks, @erseco.

It works much better now, but I think there are still some issues.

Please download the example SCORM ZIP and go to page "b1" (or another page that is the first one of its level). You'll see that the "Previous within this level" button is enabled, when there's no previous page in that level.

It happens the same with some of the last pages of a level: go to "a 2 2", and you'll see that the "Next within this level" button es enabled, but if you go to "b 2", it's not.

[erseco]

@ignaciogros thanks again — you nailed it. Fixed in 05d077e and re-tested on Moodle 5.0.5 (Docker) with your sample SCORM.

Root cause

The disabled state of the "within this level" buttons (#nav_skipprev / #nav_skipnext) was derived from exescorm_skipprev() / exescorm_skipnext(). Those functions climb to the parent when the current item has no same-level sibling — plain "Previous"/"Next" reuse them as their climbing engine — so at a level boundary the button resolved to a parent-level node and stayed enabled. That's also why it was inconsistent: on a 2 2 the parent (a 2) had a reachable next sibling so it looked navigable, while on b 2 it didn't.

Fix

The disabled state of each "within this level" button is now gated on the authoritative prevsibling / nextsibling metadata in scoes_nav, i.e. the button is disabled exactly when there is no previous/next sibling at the current level. When a sibling exists the behaviour is unchanged; Previous / Next / Level up and the #63 re-entry guard are untouched.

Verified (your sample SCORM, all 12 TOC nodes)

• b 1, a 2 1, b 1, c 1 (first of their level) → "Previous within this level" now disabled.
• a 2 2, a 3, b 2, c, c 2 (last of their level) → "Next within this level" now disabled (consistently this time).
• When enabled, the buttons jump to the correct sibling: a 2 → a 3 / a 1, b ⇄ c.
• Every TOC click still loads its page exactly once — no Wrong content displayed when clicking on navigation links #63 regression.

Before / after, on b 1 (first of its level — "Previous within this level" = « ):

Before (wrongly enabled)	After (correctly disabled)
« solid blue / clickable	« greyed out

Re-requesting your review. Thanks!

[ignaciogros]

Thank you!