Multiple dialogs open / have to click multiple times

Created on 20 August 2024, 3 months ago
Updated 19 September 2024, 2 months ago

Problem/Motivation

Spun from πŸ› Multiple dialogs open Postponed
This affects 2.x AND also 8.x-1.x
When the timer runs out and the dialog opens up after the first time this happens, there will be multiple dialogs open.

Steps to reproduce

  • Install and configure the module (lower the timeout time, to make it easy to test)
  • Go to main page and wait
  • The first time the dialog opens click the button to stay logged in
  • When the dialog opens for the second or the nth time, there will be multiple dialogs. Spamming the stay logged in button will still correctly reset the timer

Proposed resolution

Check if there is an open dialog in the background and return it instead of creating a new one.

πŸ› Bug report
Status

Needs review

Version

2.0

Component

Code

Created by

πŸ‡¨πŸ‡¦Canada joseph.olstad

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @joseph.olstad
  • Pipeline finished with Success
    3 months ago
    Total: 442s
    #259495
  • Issue was unassigned.
  • Status changed to Needs review 3 months ago
  • πŸ‡¨πŸ‡¦Canada joseph.olstad

    attributing contribution

  • πŸ‡ΊπŸ‡ΈUnited States scott_earnest

    Upon testing I am finding that the dialog only appears once, and clicking "yes" will make it go away as fixed by the patch.

    Unfortunately I am seeing a deprecation. I find that if you don't click anything, nothing happens. The expected behavior would be to auto-logout as defined by the "Timeout padding" setting.

    Moving back to "needs work". I will try to help with a solve thank you for the patch.

  • Status changed to Needs work 3 months ago
  • πŸ‡ΊπŸ‡ΈUnited States scott_earnest
  • πŸ‡¨πŸ‡¦Canada joseph.olstad

    @scott_earnest

    I just re-tested patch #4 and the MR

    by clicking nothing, I was logged out.

  • Status changed to Needs review 3 months ago
  • πŸ‡¨πŸ‡¦Canada joseph.olstad

    There's no code additional change to make, the logout behavior is optional, please set the use alt logout method option to change the behavior as desired.

    It's in the autologout settings.

  • Assigned to joseph.olstad
  • Status changed to Active 3 months ago
  • πŸ‡¨πŸ‡¦Canada joseph.olstad

    I'm actually still reviewing this and may retool the patch using something similar to what I mentioned in comment #8

  • πŸ‡¨πŸ‡¦Canada joseph.olstad

    Appears that clicking "No" does the same thing as clicking "Ok".

    However closing the dialog with the (X) works as expected.

    also, the dialog should only last 60 seconds

  • Pipeline finished with Success
    3 months ago
    Total: 478s
    #265341
  • Pipeline finished with Failed
    3 months ago
    Total: 493s
    #265457
  • Pipeline finished with Failed
    3 months ago
    Total: 418s
    #265488
  • Pipeline finished with Failed
    3 months ago
    Total: 456s
    #265494
  • Issue was unassigned.
  • πŸ‡¨πŸ‡¦Canada joseph.olstad

    MR is now functional however the automated tests need fixing and don't jive with the functionality actually working this way.

  • πŸ‡¨πŸ‡¦Canada joseph.olstad

    Patch to match the latest MR

  • πŸ‡¨πŸ‡¦Canada joseph.olstad

    Ok, so patch 14 mostly works, however the dialog does not automatically vanish after the grace period has elapsed.

  • Assigned to joseph.olstad
  • Issue was unassigned.
  • Status changed to Needs review 3 months ago
  • πŸ‡¨πŸ‡¦Canada joseph.olstad

    New patch

  • Pipeline finished with Failed
    3 months ago
    Total: 470s
    #266206
  • πŸ‡ΊπŸ‡ΈUnited States dan.d

    @joseph.olstad, about a week ago I started looking into this issue reported @ https://www.drupal.org/project/autologout/issues/3468020 πŸ› Multiple clicks to extend session needed Closed: duplicate . Since then, you've been leading this conversation here. I want to contribute, if you don't mind, with setting a bit of context based on my observations.

    Clean Installation

    First of all, I've decided to troubleshoot this issue in an "untainted" environment, so I started off with a clean installation of Drupal 10.3.x. Then I installed and enabled Autologout module. I set the timeout to the lowest-allowed number (60 seconds) and tested the feature on a single page (no page reloads to re-fresh the session). This is a good case when you have a micro frontend (or a collection of such) running on a single page and you expect your visitors/users to spend some time there.

    The Hidden Element

    During the initialization the script adds a hidden element in the DOM. Which needs to be added once (the reason for its creation is irrelevant at this point). However, I've noticed that there are multiple instances of the element being added.

    Nesting of the Hidden Element

    There seems to be an issue with the AJAX handler, as well as a redundant creation of simultaneously running background timers. It's possible this is where the actual issue originates. If you confirm the continuation of a session via the auto logout prompt (modal), you will notice that the number of nested hidden elements increases with each interaction (with the modal).

    The Growing Number of Modal Overlays

    At the same time, you will notice that each consecutive aoutologout prompt will trigger a greater number of modals (roughly equal to the number of nested hidden elements). At some point the number went over 200 modals stacked on top of each other. Which, as you can imagine, will lead to issues with rendering and, as a result, session expiration.

    Further Actions:

    I believe we will have to refactor and modernize the JavaScript implementation for this module, concentrating on the issues related to AJAX handlers and the background timers racing against each other. I will create a fork for this project and will keep on updating this thread with my further discoveries.

  • πŸ‡¨πŸ‡¦Canada joseph.olstad

    Hi @dan.d , sounds like you tested without patching which is consistent with what we also saw without patching. Wondering, have you tried patch #17 or the latest merge request patch?

    With this patch we no longer get multiple dialogs and our language prefix issue is resolved. This patch should work regardless of if you're using a language prefix or even multisite should also work using this patch. I explained it in comment #17.

  • Status changed to Needs work 3 months ago
  • πŸ‡ΊπŸ‡ΈUnited States dan.d

    Hi @joseph.olstad! I've applied the patch #17 πŸ› Multiple dialogs open / have to click multiple times Active and I'm observing the following behavior, similar to what was described in comment #14 πŸ› Multiple dialogs open / have to click multiple times Active .

    Context

    Installed D10.3.x. Installed and enabled autologout module. Applied patch #17. Set Timeout value @60 seconds and Timeout padding @20 seconds.

    Observed inconsistent behavior

    Upon the expiration of the allowed inactivity period (~1 min), the prompt modal displays with the options to keep the session going (Yes) or to immediately log out (No).

    1. The expected behavior is to auto logout the user if there's no interaction with the modal. Unfortunately, the modal stays open and the user session can be extended by clicking on the confirmation button ("Yes" default label) at any time beyond the timeout padding time.

    2. There was a case when the user got logged out automatically without an interaction with the modal within the proper timeframes. But I couldn't replicate this behavior in my consecutive attempts. This will require further investigation.

  • πŸ‡¨πŸ‡¦Canada joseph.olstad

    @dan.d , when I was testing this firefox would not let go of the cached autologout.js for some reason. Can you please re-try in a different web browser completely (other than the one you normally use?) ?.

    It could be cache related. This functionality is working for us.

    With that said, I have other patches.

                "drupal/autologout": {
                    "3308456 - Secure attribute for cookie.": "https://www.drupal.org/files/issues/2024-08-14/3308456-30.patch",
                    "3456716 - TypeError LoggerChannelInterface.": "https://git.drupalcode.org/project/autologout/-/merge_requests/62.patch",
                    "3469258 - Multiple dialogs open.": "https://www.drupal.org/files/issues/2024-08-27/3469258-17.patch",
                    "3372010 - Undefined array key in AutologoutSubscriber->onRequest().": "https://www.drupal.org/files/issues/2024-06-18/autologout_3372010-29.patch"
                },
    
    

    You might want to re-try using all of these patches.

  • Status changed to Needs review 3 months ago
  • πŸ‡¨πŸ‡¦Canada joseph.olstad

    If it's configuration related, you could compare with our configuration:

    enabled: true
    timeout: 60
    max_timeout: 172800
    padding: 30
    logout_regardless_of_activity: false
    no_individual_logout_threshold: true
    role_logout: false
    role_logout_max: false
    redirect_url: /
    include_destination: true
    no_dialog: false
    message: 'We are about to log you out for inactivity. If we do, you will lose any unsaved work. Do you need more time?'
    inactivity_message: 'You have been logged out due to inactivity.'
    inactivity_message_type: status
    modal_width: 450
    enforce_admin: true
    jstimer_format: '%hours%:%mins%:%secs%'
    jstimer_js_load_option: false
    use_alt_logout_method: false
    use_watchdog: true
    dialog_title: 'Autologout Alert'
    disable_buttons: false
    yes_button: ''
    no_button: ''
    whitelisted_ip_addresses: ''
    cookie_secure: true
    cookie_httponly: false
    cookie_samesite: Lax
    cookie_lifetime: 31536000
    
  • πŸ‡¨πŸ‡¦Canada joseph.olstad

    new patch

  • Pipeline finished with Failed
    3 months ago
    Total: 460s
    #272718
  • πŸ‡¨πŸ‡¦Canada joseph.olstad

    I've tested the latest patch #23 with the improvements from 3395581 merged in and:

    Use case:

    Open two browser windows, same session

    • Keep activity going on the right side window (manually) on my right screen, do not touch left screen.
    • As long as the activity is performed before the 30 minute timeout, the time dialog does not show up on either tab, once it's openned however, it doesn't close other dialogs that may be opened if the session timeout has elapsed there could be two dialogs openned at the same time but each doesn't know about the other.
    • The good part is, that dialog won't show up as long as there is activity in "one" of the open tabs/windows before 30 minutes (session limit).

    Summary:

    I think this is good enough for now, as a followup issue we can make a new ticket to investigate closing other possibly open dialogs by pressing OK on one of them.

  • πŸ‡΅πŸ‡ΉPortugal HLopes

    I reworked the js code to use a shared worker, so we can keep the dialog actions sync'ed across tabs.

    Missing a lot of functionality still, just experimenting.

  • πŸ‡¨πŸ‡¦Canada joseph.olstad

    Thanks for that and it does look promising however as I explained verbosely above multiple times

    $front_url = Url::fromRoute('<front>')->toString();
    

    Which is passed into js drupalSettings.autologout.front_url

    is more reliable than
    drupalSettings.path.baseUrl

    What this does is make this solution compatible with the language_selection_page module. Normally admin/* routes are commonly configured in the language_selection_page "whitelist" however this module uses non-admin routes. My changes ensure that this module works for everyone.

    If you have a look at my MR you'll see this.

    It would be great also rather than blast the autologout.js and replace it with autologout2.js in a patch file if you made a new MR based off of my MR and continue the work forward, keep two branches going so that we can test your new approach vs the current approach and work through to make it stable.

  • Pipeline finished with Failed
    2 months ago
    Total: 395s
    #286470
  • I've tested the latest version of this branch in a vanilla d10 with no other autologout patches, everything appears to be working correctly. Multiple dialog issue is fixed.

  • πŸ‡ΊπŸ‡ΈUnited States dan.d

    I will have to break my comment up into 3 parts since I'm responding to multiple posts by multiple users.

    @dmrupp,
    1. Which branch did you test with? The opening description of the issues states: This affects 2.x AND also 8.x-1.x.
    2. Were you able to repeat the session refresh without a page reload (using AJAX) multiple times?

    @hlopes,
    I like your approach! But it seems like it goes well beyond the scope initially described by Joseph. This module is due for some good refactoring, perhaps we can start a completely new branch for it. I'd be interested in contributing to it.

    @joseph.olstad,
    to keep things to the point and to provide the most generic and non-intrusive solution to the issue, I'm suggesting some JS refactoring with a bit of a code clean-up. If you want to take a look and see if this is something we can work with, I'm attaching the patch.

    In this patch, I'm addressing the following issues:

    1. Creation of poorly-managed background timers (which led to an increased number of background processes)
    2. Changing the handling of AJAX responses on success (which led to the stacking responses and a growing number of pop-ups with each consecutive AJAX trigger)
    3. Check attachment of the behavior to make sure that we initialize it once (which led to the creation of multiple DOM elements for the timer container)

    ---
    I will also update my branch with this and will create an MR, once it passes the validation locally. In the meantime, I will try testing it in my projects.

  • πŸ‡¨πŸ‡¦Canada joseph.olstad

    @dan.d, I've adapted your version into my branch.

  • Pipeline finished with Failed
    2 months ago
    Total: 633s
    #290700
  • @dan.d: I only tested the 2.x version. I believe I was able to extend the session multiple times without any page refreshes, but I can test again to verify.

    Considering there are other issues currently being worked on to replace jQuery with Drupal Dialogs ( https://www.drupal.org/project/autologout/issues/3339695 ✨ Use Drupal.dialog call instead of jQuery dialog RTBC ) I don't know that it makes sense to do a larger refactor of the JS in this branch.

  • πŸ‡ΊπŸ‡ΈUnited States dan.d

    @dmrupp, thanks for the update! I'll take another look, but the same behavior was observed on both branches for me.

    ...and I do agree with you on that the current fix should be very minimal. However, after familiarizing myself with the current implementation, I believe, that a switch from the jQuery modal to the D dialog should be a relatively straightforward process.

    @joseph.olstad, that's great! I'm glad it turned out to be of use to us ;)

  • πŸ‡΅πŸ‡ΉPortugal HLopes

    Indeed it's out of scope, altought touches the 'sync dialogs between tabs' point.

    Feel free to create a new branch, just thought that it's a good approach and started experimenting.

    Decided to just add a new js file because was just a POC and not sure about SharedWorker support accross browsers and as such one can keep both approaches and just let the user select which one to use (I've worked in a place where older browsers being supported was important)

  • πŸ‡ΊπŸ‡ΈUnited States dan.d

    My team has been testing this implementation, and we've discovered a small issue with auto-refresh for the session. I'm ready to make an adjustment, but I wanted to run the assumed logic behind the feature.

    Current state:

    Right now we've got an activity tracker that check whether a user has been active within the last 30 seconds. If so and the main timer is running out (the session is coming to an end), it will automatically re-new the session, without showing the confirmation modal.

    So, the issue arrises when a user has been active for an hour and then takes a short break for a minute, looks away and his session expires just because he/she wasn't active within the last 30 seconds of the session.

    Proposed solution:

    Start a background timer that checks the activity status every 30 seconds. If there was an action within the last 30 seconds, extend the session in the background. Otherwise, let the session expire after a set interval. This solution will send a request to the server every 30 seconds to extend the session while the user is active on the page.

    + We can also adjust that period based on the actual length of the session, let's say 1/10 of the session period. So, for a 20 minute session we'd "check in" with the server every 20 minutes.

    Let me know what you think, and I'll update the branch.
Production build 0.71.5 2024