fix: guard TOC navigation against re-entry (#63)#69
Conversation
When the user clicks a TOC entry, `exescorm_activate_item()` calls
`exescorm_tree_node.closeAll()` and later `openAll()` to collapse and
re-expand the entire YUI TreeView. Those operations 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). The visible symptoms reported in #63 — the iframe loading
twice and "click X, see Y" — line up with that pattern.
Add an `exescorm_navigating` flag set right before the first state
change and cleared after `openAll()`. The select handler returns early
when the flag is set, so only the original user click drives the
navigation.
The fix is defensive: I have not been able to reproduce locally on
Moodle 5.0, but every signal in the report (intermittent, multiple
iframe loads, sibling shown instead of clicked item) points at
re-entry through this code path. If the symptom persists after
upgrading we'll need to capture a console trace inside the failing
session.
ignaciogros
left a comment
ignaciogros
left a comment
There was a problem hiding this comment.
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__
2 participants
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.jsthat 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:
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:
17 added lines, no behavioural change for the non-buggy path.
Test plan
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.