Conversation
Fixes two _showEndOfPage end-of-page button bugs: - #200: the button could navigate into locked content. controller.scroll() now bails out of cross-page navigation when the target content object (or the resolved scrollTo model) is _isLocked. - #201: a button left unclicked on a previous visit stayed hidden yet still counted towards page completion, so the page could never complete on revisit. TrickleButtonModel.shouldCompleteOnRevisit() detects this state (incomplete, step unlocked, not locked-on-revisit, trickle killed) and the view completes the button on revisit so it no longer blocks completion. Note: as a consequence of the #201 fix, clicking the end-of-page button is no longer mandatory for page completion on revisit. The first-visit click-to- navigate behaviour is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
swashbuck
changed the title
The end-of-page button is the only trickle button that can be left incomplete once all page content is complete: other step buttons must be clicked to progress (so are already complete), and disabled buttons auto-complete on init. Guarding shouldCompleteOnRevisit() with isLastInContentObject() documents that intent and avoids touching any other button on revisit. No behaviour change for #201 in practice. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
should the last button display locked? with this pr does it just do nothing? |
|
Good catch — as it stands the button renders enabled, and the click completes it then silently no-ops when Making it display locked is straightforward ( Before I do that though — this runs into #202, which asks what this button is actually for. I've put the specifics there. The #201 fix in this PR assumes one answer, and if the other is right, that fix should be Posted via collaboration with Claude Code |
| if (!this.isLastInContentObject()) return false; | ||
| if (this.isStepLockedOnRevisit()) return false; | ||
| if (!this.isStepUnlocked()) return false; | ||
| return controller.isKilled; |
There was a problem hiding this comment.
From my Claude to yours. I get where it is coming from with avoiding a race condition. Not sure if it would actually happen.
shouldCompleteOnRevisit() leans on controller.isKilled, but that getter is !this.isStarted || _isTrickleKilled, and _isTrickleStarted is only ever written in reset(). It's not in AdaptModel.trackable() either, so on a genuine first visit it's undefined, which makes isKilled return true. Nothing in the method actually tests that we're on a revisit.
That matters because of timing. reset() is async and parks on await wait.queue(), which resolves on a setTimeout(0), while child views get built under _.defer (setTimeout(1)). So in the clean case reset() wins and your fix works exactly as intended. But if any other plugin is holding a wait open across preRender (and wait.for is used in AdaptView.remove, contentObjectView.remove, and trickle's own onAdaptStart), the button initialises first and reads isKilled === true. I mocked the wait.js semantics to confirm the flip, happy to share the snippet.
When that lands and isStepUnlocked() is already true on arrival (last block's siblings complete, optional or unavailable), the button self-completes and cascades block to article to page. Page _isComplete is trackable, so it reaches the LMS. That's the inverse of #201: instead of a page that can't complete, we get one that completes itself.
Would it work to test _isTrickleKilled directly alongside an explicit revisit signal, and hang the check off trickle:started or trickle:killed rather than initialize()? That'd take the race out of it entirely rather than relying on winning it.
| const model = data.findById(scrollToId); | ||
| const contentObject = model.isTypeGroup('contentobject') ? model : model.findAncestor('contentobject'); | ||
| // Do not allow navigation into locked content (#200) | ||
| if (model.get('_isLocked') || contentObject.get('_isLocked')) return; |
There was a problem hiding this comment.
_scrollTo accepts ".className" and getScrollToId() strips the dot and hands back a class name, not an _id. You might want a if (!mode) return; guard above L174.
4 participants
What
Fixes two bugs with the
_showEndOfPageend-of-page trickle button.Fixes #200 — button could navigate into locked content
The end-of-page button performs forward cross-page navigation via
controller.scroll(), but did not check whether the target was locked.scroll()now bails out of the cross-page branch when the resolvedscrollTomodel or its content object is_isLocked, so the button can no longer take the learner into locked content.Fixes #201 — button could prevent page completion on revisit
The button is part of the page's completion data. If it was displayed but left unclicked, then the learner navigated away, the page could never complete: on return the button was hidden (trickle is no longer locking the page) yet still counted as incomplete, deadlocking completion.
TrickleButtonModel.shouldCompleteOnRevisit()detects this state (button incomplete, step unlocked, not locked-on-revisit, and trickle killed for the page) and the view completes the button on revisit so it no longer blocks page completion.As a consequence of the #201 fix, clicking the end-of-page button is no longer mandatory for page completion on revisit. First-visit click-to-navigate behaviour is unchanged.
Testing
Verified in a build with dedicated test pages:
_showEndOfPagebutton locking #200: completing a step-locking page and clicking the end-of-page button no longer enters a locked next page; navigation to an unlocked target still works._showEndOfPagecan prevent page completion #201: completing all content but leaving the end-of-page button unclicked, navigating away and returning now completes the page; first-visit behaviour and already-complete-page revisits are unaffected.Posted via collaboration with Claude Code