Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid HTML in prerendered shiny documents #1912

Closed
3 tasks done
dakep opened this issue Oct 7, 2020 · 11 comments · Fixed by #1942
Closed
3 tasks done

Invalid HTML in prerendered shiny documents #1912

dakep opened this issue Oct 7, 2020 · 11 comments · Fixed by #1942
Labels
bug an unexpected problem or unintended behavior theme: shiny regarding shiny support in R Markdown document

Comments

@dakep
Copy link
Contributor

dakep commented Oct 7, 2020

rmarkdown::run() on a Rmd document with runtime: shiny_prerendered serves invalid HTML.

Consider the follwing Rmd file invalidhtml/invalidhtml.Rmd with contents

---
title: "Invalid HTML"
output: html_document
runtime: shiny_prerendered
---

This creates invalid HTML.

Running this document with

rmarkdown::run("invalidhtml/invalidhtml.Rmd")

Will serve a document with the follwing invalid HTML (I removed most content for brevity)

<!DOCTYPE html>
<html>
<head>
  <meta http-equiv="Content-Type" content="text/html; charset=utf-8"/>
  <script type="application/shiny-singletons"></script>
  <!-- text snippets removed -->
</head>
<body><!DOCTYPE html>

<html>

<head>
  <!-- text snippets removed -->
</head>
  <!-- text snippets removed -->
</body>
</html></body>
</html>

The problem is that rmarkdown is rendering the Rmd file to a standalone HTML document, and returning the whole HTML document in the shiny app's UI function. The UI function, however, expects only the body of the document.

I am not sure if this issue has not been discovered until now, or if it's accepted for simplicity (I think most major browsers accept this invalid HTML).


R session info:

R version 4.0.2 Patched (2020-07-13 r78838)
Platform: x86_64-apple-darwin17.0 (64-bit)
Running under: macOS Catalina 10.15.6

Locale: en_US.UTF-8 / en_US.UTF-8 / en_US.UTF-8 / C / en_US.UTF-8 / en_US.UTF-8

Package version:
  base64enc_0.1.3 digest_0.6.25   evaluate_0.14   glue_1.4.2     
  graphics_4.0.2  grDevices_4.0.2 highr_0.8       htmltools_0.5.0
  jsonlite_1.7.1  knitr_1.30      magrittr_1.5    markdown_1.1   
  methods_4.0.2   mime_0.9        rlang_0.4.7     rmarkdown_2.4.1
  stats_4.0.2     stringi_1.5.3   stringr_1.4.0   tinytex_0.26   
  tools_4.0.2     utils_4.0.2     xfun_0.18       yaml_2.2.1     

Pandoc version: 2.7.3

By filing an issue to this repo, I promise that

  • I have fully read the issue guide at https://yihui.org/issue/.
  • I have provided the necessary information about my issue.
    • If I'm asking a question, I have already asked it on Stack Overflow or RStudio Community, waited for at least 24 hours, and included a link to my question there.
    • If I'm filing a bug report, I have included a minimal, self-contained, and reproducible example, and have also included xfun::session_info('rmarkdown'). I have upgraded all my packages to their latest versions (e.g., R, RStudio, and R packages), and also tried the development version: remotes::install_github('rstudio/rmarkdown').
    • If I have posted the same issue elsewhere, I have also mentioned it in this issue.
  • I have learned the Github Markdown syntax, and formatted my issue correctly.

I understand that my issue may be closed if I don't fulfill my promises.

@cderv
Copy link
Collaborator

cderv commented Oct 7, 2020

Hi, thank you for the feedback.

The problem is that rmarkdown is rendering the Rmd file to a standalone HTML document, and returning the whole HTML document in the shiny app's UI function. The UI function, however, expects only the body of the document.

I am not sure to follow here.
AFAIK, you can pass a full html document to ui component in shiny::runApp as a tagList object.
See https://shiny.rstudio.com/articles/html-ui.html
And this is what shiny_prerendered_app() is doing with the rendered html file.

Will serve a document with the follwing invalid HTML

Where do you see this invalid html ? I can't reproduce this on my end, but not sure I look in the right place. Can you point me in the correct direction ?

@dakep
Copy link
Contributor Author

dakep commented Oct 7, 2020

Where do you see this invalid html ? I can't reproduce this on my end, but not sure I look in the right place. Can you point me in the correct direction ?

The steps that work for me to see the invalid HTML are

  1. Run the interactive document with rmarkdown::run("invalidhtml/invalidhtml.Rmd") as above
  2. Either (a) open the URL in any browser and view the page source (not in the development tools, but the actual source) or (b) use cURL, e.g., curl {SHINY_URL}. Firefox is helpful here as it highlights invalid HTML in bold red.

The difference to the examples you pointed to is that rmarkdown:::shiny_prerendered_app() serves the HTML as character vector with attribute attr(*, "html")= logi TRUE, not as tag list. The article uses shiny::htmlTemplate() to let shiny know the UI is a standalone HTML document (it appends the class html_document). I guess shiny then takes care of merging the contents in <head> and <body> with the content from shiny.

@gadenbuie
Copy link
Member

I can reproduce this, and the issue was separately reported in rstudio/learnr#498. This issue doesn't visibly impact the page (browsers do everything they can not to fail), but the HTML syntax is invalid: there should be only one <head></head> or <body></body> element.

@ghost
Copy link

ghost commented Feb 24, 2021

It's impacting all the prerendered shiny documents. It also has negative impact in terms of SEO

@cderv
Copy link
Collaborator

cderv commented Feb 24, 2021

Thanks for the precisions. I understand now better what is happening. We can't reproduce anymore with the first code above because having a document with runtime: shiny_prerendered without server context will now throw an error.

With the learn example I could reproduce and look more into it. The fix in #1942 help a lot to see where the issue is. I just want to get to the bottom of it and see if it impacts also runtime: shiny. I'll check with the shiny team to confirm what should be the full fix for this, as I don't yet the full codebase for shiny support in rmardown.

Thanks for your patience.

@gadenbuie
Copy link
Member

We can't reproduce anymore with the first code above because having a document with runtime: shiny_prerendered without server context will now throw an error.

Yesterday I added one context = "server" chunk to reproduce without learnr. It seems like the entire pre-rendered html document is being sent, rather than processing the head and body content, which I believe is wheat #1942 addresses.

I did not see similar behavior with runtime: shiny.

@cderv
Copy link
Collaborator

cderv commented Feb 24, 2021

Yes if I add some chunk for server context I can reproduce. This is what I have done this morning to investigate further.

This issue is highlighting the fact that using Rmd, a full HTML document is rendered and it should be dealt correctly after. I do not see the same with runtime: shiny but the full rendered document HTML code will be passed to shiny::renderUI() - I do not know if this is problematic or not. It seems that it's handled correctly. (The source HTML will have a shiny::uiOutput() with runtime: shiny that is why it is not exactly the same)

@ghost
Copy link

ghost commented Feb 24, 2021

I tried using #1942 but it's a few commits behind master. When I use it with learnr, it returns object 'kbd' not found. I guess it's because 18n_translations has been added recently

@jcheng5
Copy link
Member

jcheng5 commented Feb 25, 2021

I think runtime: shiny is technically doing the wrong thing too, in the sense that it's starting with a full HTML document, and then shoving another full HTML document inside of the uiOutput. It's just not as obvious as in the shiny_prerendered case, because for shiny, "View Source" is too early (document has not yet been injected) and DOM Inspector is too late (the browser or maybe jQuery has already ignored the spurious doctype, <html> and <head>).

For the same reasons though, I think fixing the shiny_prerendered case is the higher priority.

@cderv
Copy link
Collaborator

cderv commented Feb 25, 2021

Thanks @jcheng5 for this comment. It confirms what I thought about runtime: shiny and that is good to know! I'll look at it more closely after fixing the shiny_prerendered thanks to @dakep PR!

@github-actions
Copy link

This old thread has been automatically locked. If you think you have found something related to this, please open a new issue by following the issue guide (https://yihui.org/issue/), and link to this old issue if necessary.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 31, 2021
cderv added a commit that referenced this issue Feb 11, 2022
Special comment `<!-- HEAD_CONTENT -->` is now added in `$header-includes$` and there is not need anymore to add it in template. This will make it work with any custom template. As a reminder, this comments is used by htmltools to insert head content - which shiny does for html dependencies found during knitting. They will now be inserted at the same place as with non shiny document.

This offers a better fix than #2064 that aimed to be an improvement over #1942 to fix #1912. #2064 workaround is still kept though as safety measure but should not be needed

Co-authored-by: Christophe Dervieux <christophe.dervieux@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug an unexpected problem or unintended behavior theme: shiny regarding shiny support in R Markdown document
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants