bslib icon indicating copy to clipboard operation
bslib copied to clipboard

Scrolling disabled after opening modal from within another modal

Open Teebusch opened this issue 2 years ago • 11 comments

Opening a modal using a button in another modal disables scrolling because it leaves the html body tag with (among other things) style = overflow: hidden.

  • This happens regardless of whether the first modal gets closed with removeModal() before opening the second modal.
  • This happens regardless of whether fade is set to TRUE or FALSE
  • This does not happen with shiny::pageFluid(), only with bslib::page_fluid() and potentially other bslib layout functions (not tested)
  • The pattern of opening a modal from within another modal is recommended in the examples in the Shiny documentation, so this is expected to work (in the example, a call to removeModal() before opening the second modal is also not necessary). See example with confirmation modal here.

The temporary styles on the body tag are set to prevent scrolling when a modal is open, but they do not get removed properly under these circumstances.

The reason seems to be that data-bs-padding-right and data-bs-overflow data attributes are set on the body tag when the modal is opened but they don't get removed. There seems to be some listener that looks for these attributes and, based on their content, sets the temporary styles.

I'm not entirely sure if this is purely bslib's "fault" or if this is a bootstrap issue. In any case, it is easy to miss this bug when implementing a standard Shiny pattern (a confirmation modal, see above) and can severely break an app.

library(shiny)
library(bslib)

ui <- bslib::page_fluid(  # NB: this works with `shiny::fluidPage()`
  div(
    style = "min-height: 150vh;",
    shiny::checkboxInput("inp_fade_modals", "Fade Modals"),
    shiny::checkboxInput("inp_close_modals", "Close Modal 1 before Modal 2"),
    shiny::checkboxInput("inp_allow_easy_close", "Allow easy close"),
    shiny::actionButton("btn_open_modal", "Open Modal 1"),
    tags$ul(id = "log")
  ),
  div("Try to scroll past this.", style = "position: absolute; bottom: 0;"),
  div("Hello there!", style = "position: relative; bottom: 0;"),
  
  # Log the body tag to the interface for convenience
  
  tags$head(
    tags$script(HTML("
      $(document).ready(function() {
        const bodyTag = document.querySelector('body');
        
        logBodyTag = function() {
          let tag = bodyTag.outerHTML.split('>', 1)[0].substring(1)
          let hasError = !bodyTag.classList.contains('modal-open') && bodyTag.style.overflow == 'hidden';
          console.log(tag)
          
          let logEntry = $('#log').append(`<li>&lt;${ tag }&gt;</li>`)
          if (hasError) {
            logEntry.find('li:last').addClass('text-danger');
          }
        }
      
        logBodyTag()  // also log once at start
      
        const config = { attributes: true, childList: false, subtree: false };
        const observer = new MutationObserver(callback = logBodyTag)
        observer.observe(bodyTag, config);
      });
    "))
  )
)


server <- function(input, output) {
  
  # Opening a modal from within another modal disables scrolling on the 
  # body by leaving the html body tag with `style = overflow: hidden`
  
  observeEvent(
    input$btn_open_modal,
    {
      showModal(modalDialog(
        "Modal 1",
        footer    = tagList(
          modalButton("Cancel"),
          actionButton("btn_in_modal1", "Open Modal 2")
        ),
        fade      = input$inp_fade_modals,
        easyClose = input$inp_allow_easy_close
      ))
    }
  )
  
  observeEvent(
    input$btn_in_modal1,
    {
      if (input$inp_close_modals) removeModal()
      
      showModal(modalDialog(
        "Modal 2",
        footer    = tagList(
          modalButton("Cancel"),
          actionButton("btn_in_modal2", "Close Modal 2")
        ),
        fade      = input$inp_fade_modals,
        easyClose = input$inp_allow_easy_close
      ))
    }
  )
  
  observeEvent(
    input$btn_in_modal2,
    {
      # do something here ...
      removeModal()
    }
  )
  
}

shiny::shinyApp(ui, server)

Session Info


─ Session info ────────────────────────────────────────────────────────────────────────────────────────────
 setting  value
 version  R version 4.2.2 (2022-10-31 ucrt)
 os       Windows 10 x64 (build 19045)
 system   x86_64, mingw32
 ui       RStudio
 language (EN)
 collate  German_Germany.utf8
 ctype    German_Germany.utf8
 tz       Europe/Berlin
 date     2023-08-07
 rstudio  2023.06.1+524 Mountain Hydrangea (desktop)
 pandoc   NA

─ Packages ──────────────────────────────────────────────────────────────────────────────────────────────── package * version date (UTC) lib source bslib * 0.5.0.9000 2023-08-07 [1] Github (rstudio/bslib@1a763b8) cachem 1.0.6 2021-08-19 [1] CRAN (R 4.2.1) callr 3.7.0 2021-04-20 [1] CRAN (R 4.2.1) cli 3.3.0 2022-04-25 [1] CRAN (R 4.2.1) crayon 1.5.1 2022-03-26 [1] CRAN (R 4.2.1) curl 4.3.2 2021-06-23 [1] CRAN (R 4.2.1) devtools 2.4.4 2022-07-20 [1] CRAN (R 4.2.1) digest 0.6.29 2021-12-01 [1] CRAN (R 4.2.1) ellipsis 0.3.2 2021-04-29 [1] CRAN (R 4.2.1) fastmap 1.1.0 2021-01-25 [1] CRAN (R 4.2.1) fs 1.5.2 2021-12-08 [1] CRAN (R 4.2.1) glue 1.6.2 2022-02-24 [1] CRAN (R 4.2.1) htmltools 0.5.5 2023-03-23 [1] CRAN (R 4.2.3) htmlwidgets 1.5.4 2021-09-08 [1] CRAN (R 4.2.1) httpuv 1.6.5 2022-01-05 [1] CRAN (R 4.2.1) jquerylib 0.1.4 2021-04-26 [1] CRAN (R 4.2.1) jsonlite 1.8.0 2022-02-22 [1] CRAN (R 4.2.1) later 1.3.0 2021-08-18 [1] CRAN (R 4.2.1) lifecycle 1.0.1 2021-09-24 [1] CRAN (R 4.2.1) magrittr 2.0.3 2022-03-30 [1] CRAN (R 4.2.1) memoise 2.0.1 2021-11-26 [1] CRAN (R 4.2.1) mime 0.12 2021-09-28 [1] CRAN (R 4.2.0) miniUI 0.1.1.1 2018-05-18 [1] CRAN (R 4.2.0) pkgbuild 1.3.1 2021-12-20 [1] CRAN (R 4.2.1) pkgload 1.3.0 2022-06-27 [1] CRAN (R 4.2.1) prettyunits 1.1.1 2020-01-24 [1] CRAN (R 4.2.1) processx 3.6.1 2022-06-17 [1] CRAN (R 4.2.1) profvis 0.3.7 2020-11-02 [1] CRAN (R 4.2.1) promises 1.2.0.1 2021-02-11 [1] CRAN (R 4.2.1) ps 1.7.1 2022-06-18 [1] CRAN (R 4.2.1) purrr 0.3.4 2020-04-17 [1] CRAN (R 4.2.1) R6 2.5.1 2021-08-19 [1] CRAN (R 4.2.1) Rcpp 1.0.9 2022-07-08 [1] CRAN (R 4.2.1) remotes 2.4.2 2021-11-30 [1] CRAN (R 4.2.1) rlang 1.1.1 2023-04-28 [1] CRAN (R 4.2.3) rprojroot 2.0.3 2022-04-02 [1] CRAN (R 4.2.1) rstudioapi 0.13 2020-11-12 [1] CRAN (R 4.2.1) sass 0.4.7 2023-07-15 [1] CRAN (R 4.2.3) sessioninfo 1.2.2 2021-12-06 [1] CRAN (R 4.2.1) shiny * 1.7.1 2021-10-02 [1] CRAN (R 4.2.1) stringi 1.7.6 2021-11-29 [1] CRAN (R 4.2.0) stringr 1.4.0 2019-02-10 [1] CRAN (R 4.2.1) urlchecker 1.0.1 2021-11-30 [1] CRAN (R 4.2.1) usethis 2.1.6 2022-05-25 [1] CRAN (R 4.2.1) withr 2.5.0 2022-03-03 [1] CRAN (R 4.2.1) xtable 1.8-4 2019-04-21 [1] CRAN (R 4.2.1)

[1] C:/Users/[...]/AppData/Local/R/win-library/4.2 [2] C:/Program Files/R/R-4.2.2/library

Workaround

As a workaround, I use a wrapper for removeModal() that additionally calls a JS Function. Not pretty, but it seems to work:

# R - use this (instead of `removeModal()`) before opening another modal from within a modal
properlyRemoveModal <- function() {
  session <- shiny::getDefaultReactiveDomain()
  session$sendCustomMessage("properly-remove-modal", message = list())
  shiny::removeModal()
}

Load this function as part of your UI:

// JS
$(document).ready(function() {
  Shiny.addCustomMessageHandler("properly-remove-modal", (msg) => {
    ["data-bs-padding-right", "data-bs-overflow"].map((e) => {
      document.querySelector("body").removeAttribute(e)
    });
  });
});

Teebusch avatar Aug 07 '23 14:08 Teebusch

Looks like at some point Bootstrap intentionally dropped support for multiple modals. At the start of their docs for modals, it says:

Bootstrap only supports one modal window at a time. Nested modals aren’t supported as we believe them to be poor user experiences.

I also, personally, tend to agree with this stance. I don't know for sure, but Shiny might've promoted this idea in that past since it didn't support things like popovers, which we just recently added to bslib::popover(), and might be a better way to achieve the UX your looking for?

cpsievert avatar Aug 07 '23 14:08 cpsievert

Oh sorry, I think I misunderstood. The goal isn't necessarily to have multiple modals open at once, but to be able to open a new modal from a currently open modal. That should definitely work and we should probably do something to fix this

cpsievert avatar Aug 07 '23 15:08 cpsievert

Yes, this is not a problem with multiple nested modals, but with multiple modals that open subsequently -- an action in modal 1 triggers opening modal 2, as in my example above.

I agree that there should only ever be one modal at a time in the UI (that is an essential property of a modal vs., for example a popup window). But I don't see anything wrong with opening a new modal after closing the previous one. I don't believe this is a particularly uncommon or poor UX pattern

Teebusch avatar Aug 07 '23 15:08 Teebusch

Now that I'm looking at your reprex more carefully -- I'm not sure this is true:

This happens regardless of whether the first modal gets closed with removeModal() before opening the second modal.

If I change close_modal1_before_modal2 to TRUE, I'm able to scroll after closing the 2nd modal...am I missing something?

cpsievert avatar Aug 07 '23 15:08 cpsievert

Thanks for the great reprex, @Teebusch. I think the issues are a bit lower-level and stem from how opening and closing modals are handled in Shiny. The problem is present in fluidPage() as much as in page_fluid().

In short, when you click the second "Open Modal 2" button while the first modal is open, your observer sends two messages back-to-back to the server:

  1. Remove the active modal
  2. Open a new modal

They're evaluated in that order, but both messages are evaluated without knowing about each other. The remove modal message triggers some JavaScript that removes the current modal and the open a new modal message opens a new modal.

The issue is that removing the active modal takes about half a second, so the second modal is activated before the first is completely removed. There's some animation that takes place and Bootstrap's JavaScript cleans up the <body> tag once that animation completes, which is unfortunately conflicts with the open modal process.

You can see this in even in fluidPage(). The <body> tag isn't returned to its initial state after opening and closing all the modals (the padding should be restored):

<!-- start -->
<body>
<!-- open modal 1 -->
<body class="modal-open" style="padding-right: 14px;">
<!-- open modal 2 -->
<body class="modal-open" style="padding-right: 14px;">
<!-- closed modal 2 -->
<body class="" style="padding-right: 14px;">

In BS5, Bootstrap is a lot smarter about restoring the initial styles but it's the same problem.

<!-- start -->
<body>
<!-- open modal 1 -->
<body class="modal-open" style="overflow: hidden; padding-right: 14px;">
<!-- open modal 2 -->
<body class="modal-open" style="overflow: hidden; padding-right: 14px;" data-bs-overflow="hidden" data-bs-padding-right="14px">
<!-- close modal 2 -->
<body class="" style="overflow: hidden; padding-right: 14px;">

I'm pretty certain that the presence of the data-bs-overflow attributes indicates that the second modal thinks that those styles should remain on the <body> tag.

While a proper fix should need to be done in Shiny directly (possibly by having the showModal() dialog automatically call removeModal() and wait for it to complete), you can for now add a small sleep in your observer to get around this

  close_modal1_before_modal2 <- TRUE

  observeEvent(input$btn_in_modal1, {
    if (close_modal1_before_modal2) {
      removeModal()
      Sys.sleep(0.5)
    }

    showModal(modalDialog(
      "Modal 2",
      footer = actionButton("btn_in_modal2", "Close Modal 2"),
      fade = fade_modals
    ))
  }
)

With this, I consistently get the expected results

<!-- start -->
<body>
<!-- open modal 1 -->
<body class="modal-open" style="overflow: hidden; padding-right: 14px;">
<!-- open modal 2 -->
<body class="" style="">
<body class="modal-open" style="overflow: hidden; padding-right: 14px;">
<!-- close modal 2 -->
<body class="" style="">

gadenbuie avatar Aug 07 '23 15:08 gadenbuie

a possible css fix:

body { overflow: auto!important; }

while it can have minor side effects on the scrolling of the body that might not always be desirable, it will ensure that sequential modals do not break your page's scrolling as it overrules the overflow: hidden;

michaelhogersnplm avatar Aug 07 '23 15:08 michaelhogersnplm

If I change close_modal1_before_modal2 to TRUE, I'm able to scroll after closing the 2nd modal...am I missing something?

I thought I reproduced the issue with this set to TRUE, but I can't now. Could it be browser specific?

@Teebusch another, possibly more preferable option, could be to use uiOutput() and a state variable to update the modal contents rather than closing the modal. Of course, I'm realizing now, this is likely over-optimizing the reprex rather than the actual use case. But it's worth considering.

server <- function(input, output) {
  modal_state <- reactiveVal(0)

  observeEvent(input$btn_open_modal, {
    modal_state(1)
    showModal(modalDialog(
      uiOutput("modal_ui"),
      footer = actionButton("btn_in_modal1", "Open Modal 2"),
      fade   = fade_modals
    ))
  })

  output$modal_ui <- renderUI({
    req(modal_state() > 0)
    if (modal_state() == 1) {
      "Modal 1"
    } else if (modal_state() == 2) {
      "Modal 2"
    }
  })

  observeEvent(input$btn_in_modal1, {
    modal_state(modal_state() + 1)
  })

  observe({
    req(modal_state() == 3)
    # remove modal and reset modal state
    removeModal()
    modal_state(0)
  })
}

gadenbuie avatar Aug 07 '23 15:08 gadenbuie

If I change close_modal1_before_modal2 to TRUE, I'm able to scroll after closing the 2nd modal...am I missing something?

I thought I reproduced the issue with this set to TRUE, but I can't now. Could it be browser specific?

Indeed, I cannot reproduce it either. Maybe it was a strange context effect or I didn't reload the app properly? Apologies for the confusion!

So, closing the modal every time would be a way to avoid this. However, when doing that in combination with fade=TRUE the greyed out fullscreen background will briefly fade out before fading back in, which looks terrible.

I have updated the reprex in the original post a bit, to make it easier to try different variants.

--

Thank you all for the other suggestions, I will have a look at them tomorrow.

Teebusch avatar Aug 07 '23 16:08 Teebusch

I looked into the workarounds that were suggested above:

  • Popovers could be a nice way to implement a button with confirmation (like this), but it is not an alternative for all scenarios involving two subsequent modals. For example, if the second modal has a lot of content.

  • Using body { overflow: auto!important; } has the effect that scrolling the body is possible while the modal is visible. In my case, that's not always desirable (e.g., because I use some long scrollable modals)

  • Using Sys.sleep(0.5) between closing the first modal and opening the second modal is not a viable option in my case, because we use rsconnect with multiple users working on the same R process at the same time. Letting the process sleep will block the session of all users on the same process. Also, if the 2nd modal contains aconfirmation dialog, a half-a-second delay makes for a poor UX because it makes the app feel laggy.

  • Using removeModal() before opening the second modal does prevent the scroll blocking but causes visual artifacts because the greyed out fullscreen background will briefly disappear and reappear. This effect is especially prominent with fade = TRUE, but even with fade = FALSE this occasionally happens for a split second. The reason for the inconsistency is a race condition between the two javascript functions, as described above by @gadenbuie

  • In my original post I have suggested a workaround that tries to tackle the problem by removing the data- attributes from the body before removing the first modal and then opening the second modal. I use this with fade = FALSE and so far it seems to give acceptable results with no visual artifacts. However, it seems very hacky and I don't really understand why it works.

  • Using renderUI() and a state-variable to swap out the content of the modal, as suggested by @gadenbuie, could be a feasible solution, at least for smaller things. I think, for some scenarios, renderUI might cause some significant performance / bandwidth overhead, but I'm not sure. What keeps me from implementing this for my app is mainly that it means I'd have to rewrite large portions of my code.


Aside from the visual artifacts it causes, closing the first modal should not be necessary according to the examples in the Shiny documentation. People following the examples will introduce the scroll blocking bug into their app, and because it can be quite subtle, the bug can easily make it into production. So, I really hope for a fix for this on the Shiny / bslib side.

Teebusch avatar Aug 08 '23 13:08 Teebusch

  • In my original post I have suggested a workaround that tries to tackle the problem by removing the data- attributes from the body before removing the first modal and then opening the second modal. I use this with fade = FALSE and so far it seems to give acceptable results with no visual artifacts. However, it seems very hacky and I don't really understand why it works.

There's probably another race happening here. I'd expect that the important part is to remove the data-bs- attributes before closing the second modal. I'm pretty sure that Bootstrap is using those attributes to know which styles and classes to restore to the <body> tag when removing the modal. If they are there when the modal closes, they're kept on the body tag. That said, you're right that it's a little hacky and just removing these attributes isn't a great solution.

  • Using renderUI() and a state-variable to swap out the content of the modal, as suggested by @gadenbuie, could be a feasible solution, at least for smaller things. I think, for some scenarios, renderUI might cause some significant performance / bandwidth overhead, but I'm not sure. What keeps me from implementing this for my app is mainly that it means I'd have to rewrite large portions of my code.

I agree that it's probably a little tricky. With modals, each modal "provider" could be completely isolated, but with renderUI() you'd be introducing a connection between two possibly isolated areas of your app.

That said, renderUI() and showModal() do basically the same thing fundamentally, so there shouldn't be any performance or bandwidth differences.

gadenbuie avatar Aug 08 '23 14:08 gadenbuie

@gadenbuie I've been thinking about the renderUI() workaround a bit more, and I don't think this pattern scales all too well. The problem is, that, even in relatively simple cases (e.g., confirm deleting an item) one would have to update not just the main content of the modal, but also the title and the buttons in the footer. I suppose, this means you'd also need to put renderUI() (or textOutput()) in the title argument of showModal() and another uiOutput() in the footer argument and add the corresponding render functions in the server code.

In my particular case, I have more than 3 states (start/neutral, edit item, save edits, saving edits failed because of invalid value, confirm deletion of item, deletion confirmed) with distinct titles, buttons and modal content, so it seems that a nice sequence of some separate but related modals would turn into a mess of reactive values and render functions that are difficult to read and document.

Teebusch avatar Aug 09 '23 09:08 Teebusch