================================================================================================ PROPOSAL - [MINOR] functional - p2j.sso_reauth.js showSessionExpiredPage Keystroke swallowing is not actually fixed: the keyboard listeners are never detached ================================================================================================ REVIEW FINDING (abridged) "the keystroke-swallowing fix is incomplete - the symptom its own comment names ('the first Tab towards the Go to Login link') still occurs. p2j.keyboard.init registers onkeydown/onkeypress/onkeyup on document in capture phase and nothing on the teardown path detaches them, so on the replaced page GUIKeyboardReader.onkeydown still runs: rejectKeyEvent is false because excludedContainer (the cached #change-os-pass node) was detached by the document.body.innerHTML = '' wipe, p2j.isDialogDisplayed() is false once cleanupReauthUi and the new disposeConnectionDialogs() have emptied the dialogs set, and permittedKeyStrokes returns false for Tab (also F3 and every Ctrl/Alt combination), so p2j.consumeEvent preventDefaults it - every Tab, not just the first, leaving the link reachable only by mouse. CHUI mode behaves the same via sendKey. Note the disposal is what re-enables the swallowing in the one case where it changes anything." FILES src/com/goldencode/p2j/ui/client/driver/web/res/p2j.keyboard.js (new detach API) src/com/goldencode/p2j/ui/client/driver/web/res/p2j.sso_reauth.js (call it) ------------------------------------------------------------------------------------------------ 1. ROOT CAUSE ------------------------------------------------------------------------------------------------ r16675 addressed the wrong owner of the problem. disposeConnectionDialogs() removes the *dialogs'* capture-phase triggers, but the swallowing on the session-expired page comes from the driver's own document-level listeners, which p2j.keyboard.init installed and which nothing ever removes: p2j.keyboard.js:348-359 document.addEventListener('keydown', ..., true) document.addEventListener('keypress', ..., true) document.addEventListener('keyup', ..., true) window.addEventListener('blur', ..., true) (+ 'input', 'compositionstart', 'compositionend' in GUI mode) The removal code for exactly these listeners already exists - p2j.keyboard.js:319-336 - but it is inlined at the top of init() and only runs when init() is called again. There is no way for any other module to ask for a teardown. Consequently, after showSessionExpiredPage() wipes the body: - rejectKeyEvent() returns false: excludedContainer is a node reference cached at init (p2j.keyboard.js:348) and the wipe detached it, so contains(evt.target) is false for everything. - p2j.isDialogDisplayed() is false, because cleanupReauthUi() and the new disposeConnectionDialogs() between them emptied the dialogs set - the new disposal call is precisely what *re-enables* the swallowing in the one case where the old code happened to escape it. - permittedKeyStrokes() returns false for Tab (and F3, and every Ctrl/Alt combination), so p2j.consumeEvent() preventDefaults it. Result: every Tab is swallowed, not just the first, and the "Go to Login" link is mouse-only. CHUI mode reaches the same place through sendKey. ------------------------------------------------------------------------------------------------ 2. PROPOSED FIX ------------------------------------------------------------------------------------------------ 2.1 Extract the existing removal block into a public detach API ---------------------------------------------------------------- p2j.keyboard.js - lift lines 319-336 out of init() into a named function and export it. Pure refactoring plus one export; no behaviour change for the existing init() path. + /** + * Detach every document and window level key listener installed by init. + *

+ * Called by init before it re-registers, and by any caller which is about to replace the page + * content: the listeners are registered on document in the capture phase, so they outlive the + * elements they were installed for and would go on consuming keystrokes on the replaced page - + * including Tab, which permittedKeyStrokes never permits. + */ + me.detachListeners = function() + { + if (me.keyboardReader) + { + document.removeEventListener('keydown', me.keyboardReader.onkeydown, true); + document.removeEventListener('keypress', me.keyboardReader.onkeypress, true); + if (me.keyboardReader.oninput) + { + document.removeEventListener('input', me.keyboardReader.oninput, true); + } + if (me.keyboardReader.onCompositionStart) + { + document.removeEventListener('compositionstart', + me.keyboardReader.onCompositionStart, + true); + } + if (me.keyboardReader.onCompositionEnd) + { + document.removeEventListener('compositionend', me.keyboardReader.onCompositionEnd, true); + } + document.removeEventListener('keyup', me.keyboardReader.onkeyup, true); + window.removeEventListener('blur', me.keyboardReader.onfocuslost, true); + + me.keyboardReader = null; + } + + excludedContainer = null; + }; and init() becomes: me.init = function(cfg) { // input element inputText = document.getElementById(cfg.clipboard.input); - if (me.keyboardReader) - { - ... 18 lines of removeEventListener ... - } + me.detachListeners(); // init mapping p2j.keymap.init(cfg); ... Note on `me.keyboardReader = null` and `excludedContainer = null`: init() reassigns both immediately afterwards, so the init path is unaffected. Clearing them matters for the teardown path - a stale keyboardReader would let a *newly added* listener (from anywhere) still dispatch into a driver whose DOM is gone, and a stale excludedContainer keeps a detached node alive. 2.2 Call it from showSessionExpiredPage ---------------------------------------- p2j.sso_reauth.js, in showSessionExpiredPage(), immediately before the body wipe: document.body.innerHTML = ''; becomes + // The driver's key listeners are registered on document in the capture phase, so wiping the body + // does not remove them: GUIKeyboardReader.onkeydown would still run on this page, and since + // permittedKeyStrokes rejects Tab it would consume every Tab towards the "Go to Login" link. + // Disposing the connection dialogs above removes their triggers but not these. + try + { + p2j.keyboard.detachListeners(); + } + catch (e) + { + console.error('SSO re-auth: detaching the keyboard listeners failed', e); + } + document.body.innerHTML = ''; The try/catch matches the surrounding style (the disposeConnectionDialogs() call directly above is guarded the same way) and keeps a failure here from aborting the page replacement. 2.3 Correct the comment that overstates what the dialog disposal achieves ------------------------------------------------------------------------- The comment introduced by r16675 above the disposeConnectionDialogs() call claims the dialog triggers "would swallow the first keystroke on this page - including the first Tab towards the 'Go to Login' link". That is not what happens: the driver listener swallows *every* Tab, and the dialog triggers were in fact the only thing suppressing it (via isDialogDisplayed()). Rewrite it to state what the disposal is actually for: - // The connection dialogs are parented on document.body, so they must be disposed of before it is - // wiped. Otherwise the wipe only detaches their nodes and leaves the widgets registered, each - // still holding a capture-phase window keydown trigger that would swallow the first keystroke on - // this page - including the first Tab towards the "Go to Login" link. + // The connection dialogs are parented on document.body, so they must be disposed of before it is + // wiped: the wipe only detaches their nodes and leaves the widgets registered, still holding a + // capture-phase window keydown trigger and still willing to run show/hide against nodes that are + // no longer in the document. Keystroke handling on the replaced page is a separate matter - see + // the detachListeners call below. ------------------------------------------------------------------------------------------------ 3. ALTERNATIVES CONSIDERED ------------------------------------------------------------------------------------------------ (a) Make permittedKeyStrokes() permit Tab. Rejected: it is deliberately restrictive for the 4GL client (p2j.keyboard.js:58 revision note "return true for all key strokes except TAB key"), and changing it would affect every live session, not the expired page. (b) Point excludedContainer at the new container. Rejected: it would rely on rejectKeyEvent() as a whitelist mechanism it was not designed to be (its purpose is the #change-os-pass form), and it would leave keypress/keyup/blur handlers running against a dead driver. (c) Navigate away instead of replacing the body. That is what already happens on the normal path (redirectToLogout() navigates within one fetch RTT) and is why this finding is MINOR: the expired page is only interactive when p2j.embedded is set, or when the logout fetch rejects or hangs - and unlike fetchAuthEndpoint() it has no timeout. Adding a timeout to that fetch is a worthwhile separate change, but it does not make the page correct in embedded mode. ------------------------------------------------------------------------------------------------ 4. RISK AND VERIFICATION ------------------------------------------------------------------------------------------------ Risk: low, and confined to the teardown path. The refactor in 2.1 is behaviour-preserving for init() (the only current caller of the removal code). The only new exposure is that detachListeners() nulls me.keyboardReader, so any code calling p2j.keyboard.keyboardReader after a detach must tolerate null - after showSessionExpiredPage() the session is over and the socket is closed, but grep for `keyboardReader` before landing to confirm no module dereferences it unguarded on that path. Verification 1. Web GUI, force MSG_SSO_REAUTH and let the countdown expire with p2j.embedded set (so the page stays interactive). Press Tab: focus must reach the "Go to Login" link. Before the fix: nothing happens on any Tab press. 2. Same with the logout fetch blackholed (block /logout at the proxy) in non-embedded mode. 3. Web CHUI: same test; verify no sendKey traffic is attempted from the expired page. 4. Regression: a normal re-auth that *succeeds* must leave the keyboard fully working - that path goes through cleanupReauthUi() and never calls showSessionExpiredPage(), so detachListeners() must not be reached. Confirm by breakpoint or console trace. 5. Regression: a session that re-initialises the driver (reconnect) must still work, i.e. init() -> detachListeners() -> re-register.