Fix: re-enable the trickle button when the last popup closes (fixes #267)

[swashbuck]

Fixes #267

Fix

• The trickle button now re-enables when the last popup closes, instead of staying disabled forever and making the page impossible to complete.
• Both places that asked "is a popup open?" during popup:closing now count the popup stack and discount the popup that is closing, since it has not been removed from the stack yet at that point.

Why not simply revert #264

#264 exists to fix #263, unlocking behind popups on returning visits, so a straight revert would bring that back. The problem is not that #264 consulted a11y. That is the right source of truth, and a per-view counter starting at zero is exactly what made #263 possible. The problem is when it consults it.

Core triggers popup:closing before the closing popup is taken off the stack, so a handler on that event always sees the closing popup as still open. a11y.isPopupOpen cannot express "is anything else still open" at that moment, which is what the guard actually wants to know. Counting the stack can express it.

Fixing only the early-return guard is not enough. The next thing that runs is the recalculation, which asks the same question at the same moment and computes "disabled" again, so the button stays stuck. Both sites have to account for the closing popup.

onParentComplete also reads a11y.isPopupOpen, but it fires on a completion change rather than during popup:closing, so it reads the stack outside the closing phase and is left alone. finish() is left alone too: by the time it runs the button is finished, and the enable path that then applies does not consult the popup state at all.

A flag is used rather than an extra method parameter because updateButtonState is bound directly to an event elsewhere in the view, so a new parameter would receive that event's arguments and quietly become truthy. The flag is set and cleared synchronously around a single synchronous call, so nothing can interleave and leave it set.

Dependencies

None. a11y.popupStack has been available since core 6.28.0, well below this plugin's minimum framework version.

Testing

Tested in Chrome 151 on macOS against Adapt Framework 5.56.2 with core 6.78.3, and also against core 6.77.1, the version that framework ships with. Both behave the same before and after.

The quickest check needs no course JSON at all:

1. Load a course with a trickled block whose step is unlocked and whose Continue button is enabled.
2. In the console, run require('core/js/drawer').open(); then require('core/js/drawer').close();
3. Before this change the button is disabled and never recovers. After it, the button is disabled while the drawer is open and enabled again once it closes.

Then the routes learners actually take, all confirmed fixed:

1. Question feedback. A trickled block containing a question with _canShowFeedback: true. Answer it, submit, dismiss the feedback popup. The button should be clickable, and clicking it should reveal the next block.
2. Drawer. Open the drawer from the navigation bar on a trickled page, then close it. The button should be usable again.
3. Page incomplete prompt. With _pageIncompletePrompt enabled, try to leave an incomplete trickled page and answer "No". The page should still be completable, which is the whole point of staying on it.

Worth also checking these, all verified:

• Nested popups. With a feedback popup open, open a second popup over it. Closing the top one should leave the button disabled, because one is still open. Closing the last one should enable it.
• No regression of Unlocking behind popup on returning visits #263. With a popup already open, unlock the step underneath it. The button should stay disabled while the popup is up and only enable once it closes.
• No popup involved. A trickled block whose question does not show feedback should behave exactly as before.

Posted via collaboration with Claude Code

[swashbuck]

@oliverfoster Good question, and it is a fair alternative. I went and measured it rather than reasoning about it, because my first instinct turned out to be half wrong. Summary: it would fix trickle, but it changes the meaning of a public getter during a window that any listener can stretch, and it would conflict with this PR rather than sit alongside it.

It would work for trickle. Both sites this PR touches only ever ask about popups, so a closing-aware isPopupOpen would give the right answer at both, including the nested case (closing the top popup while another is open still reports open). No plugin change needed, and it would immunise any future listener against the same trap.

The catch is that the closing window is not short. Core does await wait.queue() immediately after triggering popup:closing, so any listener that registers a wait extends the window. Trickle already does exactly that: in the isAwaitingPopupClose path it calls wait.begin() inside the handler and does not call wait.end() until after controller.continue() has rendered the next content.

Measured in a sandbox (Framework 5.56.2, core 6.78.3):

• Default window between popup:closing and popup:closed: 3 to 4 ms.
• With a wait registered in the handler, the same way trickle does it: 803 ms, and it would be longer with heavy media.

During that window a dialog popup can still be genuinely modal. At 802 ms into the extended window I checked the drawer's element: matches(':modal') was still true. That is not an accident of my test, it is by design. In drawerView.js a11y.popupClosed() is called before the hide animation, with an explicit comment and a display: block so that the HTMLDialogElement.close() inside it does not hide the dialog early. So for the drawer, popup:closing fires while the dialog is still open and still modal.

Where I was wrong: this is not true for notify popups. In notifyPopupView.js the close animation runs, then this.remove(), and only then a11y.popupClosed(). So a notify popup is already out of the DOM by the time popup:closing fires. The two popup types have opposite orderings, which is worth knowing on its own, and means a core redefinition has to be correct for both.

It is observable in a core primitive, not just in theory. isReadable() gates on isPopupOpen at a11y.js:667, and it is what focusFirst() and focusNext() traverse with. Probing a background leaf element at popup:closing with the drawer open:

• today: isReadable() returns null, meaning skip, do not focus this
• with closing popups excluded: returns true, meaning focusable

So the change would let focus traversal walk into background content while a modal dialog is still up. Over 3 ms that is unlikely to matter. Over 803 ms, with content rendering and controller.scroll() calling focusFirst() in that same window, it is a plausible focus escape.

There is also a11y/scroll.js, which gates on isPopupOpen and then immediately reads popupStack[length - 1] as "the open popup". If isPopupOpen were changed without popupStack, those two disagree; if both were changed, that line starts pointing at the wrong popup. Whichever way it goes, isPopupOpen and popupStack need to move together.

The part that decided it for me: the two fixes conflict, they do not stack. If core later excludes the closing popup from popupStack, the guard in this PR breaks silently. Closing the top of two popups would report a length of 1, 1 > 1 is false, so trickle would carry on and enable the button while a popup is still open. That is the nested bug in reverse, and nothing would fail loudly. So this is genuinely an either/or, not a belt-and-braces situation.

Suggestion. Land this as the fix for the regression, since it is contained to one plugin and the bug currently makes pages impossible to complete. If you want the core-level fix, I would rather it were additive than a redefinition: either pass the closing layer as an argument, Adapt.trigger('popup:closing', $closingElement), or expose a separate count that already discounts it. That gives every listener a way to ask "is anything else still open", which is the actual question here, without changing what isPopupOpen means for existing callers. Happy to raise that on core, and if it lands, the flag in this PR should be reverted rather than left in.

One thing worth noting either way: trickle is currently the only listener for popup:closing anywhere in the bundle I tested, so nothing else is relying on the current behaviour yet.

Posted via collaboration with Claude Code

[swashbuck]

@oliverfoster Correcting the last line of my previous comment, where I said "trickle is currently the only listener for popup:closing anywhere in the bundle I tested, so nothing else is relying on the current behaviour yet". The first half holds up and is now on firmer ground. The second half was wrong, and it happens to be the half that matters for your suggestion.

I widened the search: 175 local plugin clones, plus GitHub code search across current master in this org and one other.

Listeners for popup:closing: trickle only. In both orgs the only other hit is core's own trigger in a11y/popup.js. So nothing else is exposed to the ordering trap today.

Readers of isPopupOpen, the getter you would be redefining:

• core, a11y.js:667, inside isReadable()
• core, a11y/scroll.js:188
• trickle, the three sites in TrickleButtonView.js
• one further plugin, in a private repo outside this org

That last one is why I was wrong. It does not listen to popup:closing, so my search missed it, but it does depend on what isPopupOpen currently means. Reduced to the relevant lines:

if (a11y.isPopupOpen) {
  this.listenToOnce(Adapt, 'popup:closed', this.onPageScrollTo.bind(this, selector, settings));
  return;
}

It uses the getter to defer a scroll until the popup has gone, and waits for popup:closed to retry. Under a closing-aware isPopupOpen, a call landing inside the closing window would see no popup and scroll immediately instead of deferring, which is the opposite of what that guard is for. The path is reachable from trickle: controller.scroll() calls router.navigateToElement(), which triggers page:scrollTo at router.js:558, which is the event that plugin listens for. I could not measure it end to end because the plugin is not installed in my sandbox, so treat it as a code path worth checking rather than a demonstrated failure.

The general point stands regardless of which plugin it is: isPopupOpen is currently used elsewhere as a "wait until the popup has gone" gate, paired with popup:closed. That is the idiom that avoids the trap this PR fixes, and it is the idiom a redefinition would invert.

Worth saying explicitly, so the list above does not look worse than it is: the hits in hotgraphic, narrative, hotgrid and a few others are all private this._isPopupOpen instance flags on their own views. They have nothing to do with the core getter.

Readers of popupStack: core only, in the same two files. Zero plugins anywhere I searched.

That gives a clean comparison of blast radius, which I think is the useful part:

• the approach in this PR reads popupStack, which no plugin anywhere reads, so it cannot affect anything else
• changing isPopupOpen touches two core call sites and one other plugin, one of which uses it in a way that would invert

None of that makes the core route wrong, and it is still the more complete fix for the underlying footgun. It does mean it needs that other consumer checked and popupStack kept consistent, rather than being a one line change. Which is the argument for doing it deliberately and separately, rather than as the fix for this regression.

Posted via collaboration with Claude Code

[danielghost]

This issue wasn't actually introduced by #264, as at the time that was written the plugin was still listening to popup:closed. #233 switched to popup:closing which then caused onPopupClosed to return early:

adapt-contrib-trickle/js/TrickleButtonView.js

Lines 93 to 94 in 0cb46eb

	async onPopupClosed() {
	if (a11y.isPopupOpen) return;

Two different PRs were merged around the same time, with one affecting the other.

Could the changes made for #233 instead add popup:closing as an additional event and move the relevant code added to onPopupClosed into onPopupClosing?

[danielghost]

As the event for this listener changed in #233 which subsequently caused an issue with #264, which would have otherwise been flagged before the merge, I would recommend this method changes to onPopupClosing to reflect the trigger.

Will require changes to:

adapt-contrib-trickle/js/TrickleButtonView.js

Line 76 in 0cb46eb

'popup:closing': this.onPopupClosed

adapt-contrib-trickle/js/TrickleButtonView.js

Line 185 in 0cb46eb

'popup:closing': this.onPopupClosed

[danielghost]

Extract the guard condition into a named predicate?

Suggested change

	// This fires before the closing popup is removed from the stack, so more
	// than one means another popup is still open behind it
	if (a11y.popupStack.length > 1) return;
	const isAnotherPopupOpen = (a11y.popupStack.length > 1);
	if (isAnotherPopupOpen) return;

[danielghost]

You could argue that the wrong value is set here (remains disabled) when this method is reached via finish in the following condition:

adapt-contrib-trickle/js/TrickleButtonView.js

Lines 103 to 108 in 1d107a8

	if (this.isAwaitingPopupClose) {
	this._isWaiting = true;
	wait.begin();
	// Had completed with an open popup, perform final part of finishing
	return this.finish();
	}

However, this only seems to be reachable when the button is disabled in the trickle config, at which point it makes no difference to the button state anyway and it returns early in this condition:

adapt-contrib-trickle/js/TrickleButtonModel.js

Lines 173 to 180 in 1d107a8

	calculateButtonState(isButtonDisableForced = false, isButtonHiddenForced = false) {
	if (!this.isEnabled()) {
	this.set({
	_isButtonVisible: false,
	_isButtonDisabled: !this.isStepUnlocked()
	});
	return;
	};

If the method was ever reached for a button enabled in the config, this may need changing. Is it worth correcting this now?