Fix conflict between collapse=TRUE and htmlwidgets

[dmurdoch]

As reported in my comment to #2172, collapse = TRUE is incompatible with including htmlwidgets output in a document. This patch fixes it by recording which sections of chunk output have special classes (as htmlwidgets sections do), and only attempting to collapse the plain sections.

It's a little messy, because the collapsing is implemented in a chunk hook after the code has been coalesced into one big string. I added attributes identifying which parts of that string shouldn't be touched.

[yihui]

Thanks! This looks too complicated for me to maintain, so I wonder if there is a simpler solution. For example, look for the pattern ([lang] is optional attributes)

```[lang]\n...```\n```[lang]\n```

and substitute with

```[lang]\n...\n...```

Since the pattern ```[lang] won't match ```{=lang}, HTML widgets shouldn't be touched.

[dmurdoch]

I tried a pattern matching approach first and it seemed too slow: sometimes the strings being examined can hold thousands of characters, e.g. the JSON for a complicated rgl scene. But I might have got the pattern wrong.

I agree that my current attempt is complicated. I'll take another look and see if I can improve it.

[dmurdoch]

This code uses a simpler approach. It just wraps the special results in markers that won't match what the markdown collapse code is looking for.

There's some chance that the special block will contain the marker and my cleanup will break it (but the markers have a time stamp; the next run should be okay), or that the special block will contain the pattern that the markdown chunk hook is looking for.

I think these are both unlikely, so I think this is a better patch than the previous one.

[dmurdoch]

One other comment: I removed the previous test code and didn't add a new test, because I don't know how to put together a test for this approach. It would need to do a visual evaluation of the final result of processing a full document.

[cderv]

My first idea would be like you to modify the collapse processing we have

knitr/R/hooks-md.R

Lines 214 to 217 in 77970b0

	if (isTRUE(options$collapse)) {
	r = sprintf('\n([%s]{3,})\n+\\1((\\{[.])?%s[^\n]*)?\n', fence_char, tolower(options$engine))
	x = gsub(r, '\n', x)
	}

I don't know if we could try something simpler but currently we are indeed trying to match this pattern with our current regex ("\n([]{3,})\n+\1((\{[.])?r[^\n]*)?\n"`

```
    
```r

This is indeed not correct when we have some output like htmlwidgets which will use raw code block that are not meant to be collapse into a code chunk

```r
# R code here producing HTML to insert asis
```
```{=html}
<!-- raw html in there -->
```
```r
# another R code with code result
```

```
# the result in code markup
```

We would need to parse and match blocks differently. We don't have a good parser for that.

So the wrapping with special content is a good trick that rely only on the fact our current regex expect only newline after the backticks ``` - so just adding something after that for htmlwidget output which as raw block would work.

This is a bit fragile though.

Fun Fact @yihui (or not 😅 ) : collapse = TRUE with htmlwidget wokrs with Quarto because Quarto is already wrapping (for other purpose) any knit_asis output, and especially HTMLwidgets by using hooks and replacement function. Example of what knitr receive before collapse.

::: {.cell-output-display}
```{=html}
<raw html here>
```
:::

This is also for me another reason to do that at sew method or hooks level because those function can be easily replace by tools like quarto and we would not create conflict (unlike modifying at the block.R level I think)

Not that the global issue is not with htmlwidget only, but any R code that would produce a asis output using special Pandoc syntax raw block. Probably more content does not work with our simple collapse processing I guess - surprised that this was not reported before.

I am surprised that this was not reported before. Probably because a few people are using chunks mixing HTMLwidgets output and standard R output, and using collapse = TRUE on them. I guess if we test every possible output with collapse = TRUE, we could have other suprises 😅

[cderv]

This will target also other class that HTMLwidget object, isn't it ? If we want to wrap the sewed output of HTMLwidget we could target a more scope class couldn't we ?

attr(,"class")
[1] "knit_asis"            "knit_asis_htmlwidget"

[cderv]

Following previous comment, if we go with this trick, I think it would be better to do this directly in sew.knit_asis when it inherit knit_asis_htmlwidget ? we can target thinks there.

knitr/R/output.R

Lines 481 to 485 in 77970b0

	if (inherits(x, 'knit_asis_htmlwidget')) {
	options$fig.cur = plot_counter()
	options = reduce_plot_opts(options)
	return(add_html_caption(options, x))
	}

This would be the best place to add some content to the sewed output for html widget I think when collapse is true

We could then remove the special where we do the collapse processing, or use a special pattern that we register and clean all pattern in block.R as you did

[dmurdoch]

I just discovered that it's not just htmlwidgets, also output from browsable(HTML( .... )). In that case the class wasn't changed, so my original test missed it. I've just committed a test that finds those cases.

I think we don't want this to be specific, we want to recognize anything that isn't simple input or output and protect that from being modified.

The problem with changing sew is that sew is a generic, so you'd have to track down all packages that use it and change them too. rgl provides a method, but I could adapt if necessary. I don't know if any other packages do.

Perhaps changing whatever function knitr uses to wrap asis objects?

[cderv]

I think we don't want this to be specific, we want to recognize anything that isn't simple input or output and protect that from being modified.

All those output should have the knit_asis() class I believe. Do you have an example that miss that ?

Perhaps changing whatever function knitr uses to wrap asis objects?

We are handling asis object in sew.knit_asis() - that would be where to wrap such output. If you are referring to the the raw HTML part ```{=html} , this is handled by htmltools directly through htmltools:::html_preserve()

[dmurdoch]

You asked if all examples are "knit_asis". I am not sure. Some examples (e.g. htmltools::browsable(htmltools::HTML("html code")) don't start out with that class, and don't have that class after sew() is called, but they might have that class at some step in between.

I think the idea of wrapping the objects at that point is definitely worth a try, but I don't know where that code should go. Feel free to make changes on my "collapsebug" branch, or let me know if you want me to test a branch somewhere else.

[cderv]

(e.g. htmltools::browsable(htmltools::HTML("html code")) don't start out with that class, and don't have that class after sew()

The result of this call should have the knit_asis class when sew is called. This is because knit_print() will be called to print this and it will use htmltools:::knit_print.html which will apply knitr::asis_output(). I can also confirm that in debug mode when looking for the result of such chunkj

---
output: html_document
---
    
```{r}
htmltools::browsable(htmltools::HTML("html code"))
```

> x
[1] "\n```{=html}\nhtml code\n```\n"
attr(,"class")
[1] "knit_asis"
attr(,"knit_cacheable")
[1] NA

I think the idea of wrapping the objects at that point is definitely worth a try, but I don't know where that code should go. Feel free to make changes on my "collapsebug" branch, or let me know if you want me to test a branch somewhere else.

Ok sure I can implement my idea. Waiting for @yihui opinion on my thoughs and comments first.

Thanks @dmurdoch !

[dmurdoch]

I've just pushed a revision to the PR based on your suggestion. It appears to work for my test cases. You probably want to review the class names I chose.

Here's my test file.

---
title: "Vignette Title"
author: "Vignette Author"
date: "`r Sys.Date()`"
output: rmarkdown::html_vignette
vignette: >
  %\VignetteIndexEntry{Vignette Title}
  %\VignetteEngine{knitr::rmarkdown}
  %\VignetteEncoding{UTF-8}
---

```{r setup, include = FALSE}
knitr::opts_chunk$set(
  collapse = TRUE,
  comment = "#>"
)
```

```{r}
# This is before the widget
x <- 2
x

library(leaflet)
leaflet() %>% addTiles()

# This is some more code after the widget
x <- 1
x

# And another widget:
leaflet() %>% addTiles()
```

```{r}
library(htmltools)
# this is before the HTML
x <- 1
x

browsable(HTML("some html"))

# This is after the HTML

x <- 2
x

browsable(HTML("more html"))

# This is after the second HTML

x <- 3
x
```


```{r}
library(rgl)
rgl::setupKnitr(autoprint = TRUE)

# this is before the rgl
x <- 1
x

plot3d(1:10, 1:10, 1:10)

# This is after the rgl

x <- 2
x

plot3d(1:10, 1:10, 1:10)

# This is after the second rgl

x <- 3
x
```

[yihui]

The current revision looks good to me. I need to check reverse dependencies to see if it would break any of them. We could be more conservative and apply the ::: div only when options$collapse is TRUE. Not sure if it's worth it. I'll know after the reverse dependency checks are done. Thanks!

[cderv]

We could be more conservative and apply the ::: div only when options$collapse is TRUE

Yes I think this needs to be scoped to collapse TRUE.

Let me precise my previous remarks though...

I just gave the example of Quarto using divs with ::: to wrap to show an example. But Quarto adds the divs syntax for a purpose - such syntax will add AST node in Pandoc to deal with.
If we do the same, we add another layer in AST for Pandoc that we don't really use and can break other tools like Quarto.

My previous comment was mainly about doing your wrapping idea in another place closer to htmlwidget sewing and related to collapse part. So I think you idea of wrapping with a special token / line / knitr string is a good one. I believe we need this for collapse only so we could add and then remove when collapse is applied.

I don't think we need to keep such wrapping in the knitr output. @yihui this would mean no impact on reverse dependencies I believe.

Unless we think having this wrapping is beneficial for other purposes.

Hope my thoughts are shared clearer this time. Sorry for any misleading.

[dmurdoch]

I think it's difficult to remove anything after the collapsing has been done, because at that point you don't know where the boundaries are. So if you are inserting something that you plan to remove, it should contain some unique token. I used the current time in a previous attempt, but a random value using knitr's copy of the RNG would probably be better.

[cderv]

So if you are inserting something that you plan to remove, it should contain some unique token. I used the current time in a previous attempt, but a random value using knitr's copy of the RNG would probably be better.

Yes I agree with that. I'll think about this and discuss with @yihui. Thanks for the PR @dmurdoch !

[yihui]

Thanks for sharing the ideas! I think I fully understand them now. The key is a unique token that is applied when collapse = TRUE and removed later. I'm fine with a random string or a timestamp.

[dmurdoch]

Sounds like you have this under control. I won't make any more changes.

[yihui]

Yes, I can carry on from here. Thank you!