dashboard processes any hidden Div as cell computation output causing problem with our parsed HTML table processing

olivroy commented 2 months ago

Bug description

A gt table with cols_label(md()) disappears silently in quarto dashboard. It shows as expected in other quarto document types. So I am wondering if the bug is on gt's side, or could it be fixed in Quarto.

Open issue in gt https://github.com/rstudio/gt/issues/1605

Steps to reproduce

---
title: "dash-test"
format: dashboard
editor: visual
editor_options: 
  chunk_output_type: console
---

## Heading

```{r}
#| title: a title0
library(gt)
exibble |> 
 gt::gt() |>
  cols_label(
    num = md("**here**")
  )



### Expected behavior

Table should be present, or an error should be signaled that the table is malformed or something

### Actual behavior

The table disappears silently from output.

### Your environment

IDE: RStudio 2024.04 on Windows (also reproducible on quartopub.

### Quarto check output

```bash
Quarto 1.4.553
[>] Checking versions of quarto binary dependencies...
      Pandoc version 3.1.11: OK
      Dart Sass version 1.69.5: OK
      Deno version 1.37.2: OK
[>] Checking versions of quarto dependencies......OK
[>] Checking Quarto installation......OK
      Version: 1.4.553
      Path: ~\AppData\Local\Programs\RStudio\resources\app\bin\quarto\bin
      CodePage: 1252

[>] Checking tools....................OK
      TinyTeX: (external install)
      Chromium: (not installed)

[>] Checking LaTeX....................OK
      Using: TinyTex
      Path: ~\AppData\Roaming\TinyTeX\bin\windows\
      Version: 2024

[>] Checking basic markdown render....OK

[>] Checking Python 3 installation....(None)

      Unable to locate an installed version of Python 3.
      Install Python 3 from https://www.python.org/downloads/

[>] Checking R installation...........OK
      Version: 4.3.2
      Path: C:/PROGRA~1/R/R-43~1.2
      LibPaths:
        - ~/AppData/Local/R/win-library/4.3
        - ~/R/R-4.3.2/library
      knitr: 1.46
      rmarkdown: 2.26

[>] Checking Knitr engine render......OK

cderv commented 2 months ago

In the produced HTML we have a hidden class on the cell div. So CSS applies and the code is hidden.

Somehow, a card is not created in this case, and cell results indersted into it.

This seems related to the data-qmd feature @rich-iannone . If we remove <div data-qmd="**here**"> from the gt table in the intermediate md, then it renders ok.

I don't know why it triggers on dashboard only, but I noticed that gt will always use <div data-qmd= to pass the information, whereas it should be <span data-qmd= in some cases. This is a HTML table processing in Quarto that gt leverages, and it seems it causes issue in Dashboard that are not in HTML but still using a <div> inside a <th> does not seem right, and <span> should be prefered.

Fixing the interemediate .md like this work @rich-iannone but then there is a double processing because of the other div

<div data-qmd="**here**"><div class='gt_from_md'><p><strong>here</strong></p>
</div></div>

Let's discuss more in https://github.com/rstudio/gt/issues/1605 maybe;

I'll keep that open to understand why the <div> creates the issue in dashboard format when created from a computation cell.

(it works when using Markdown syntax https://quarto.org/docs/dashboards/layout.html#cards)

cderv commented 2 months ago

I'll keep that open to understand why the
creates the issue in dashboard format when created from a computation cell.

So this happens because we do a specific iteration on Div inside Div of class cell where we look is there is "hidden" element

https://github.com/quarto-dev/quarto-cli/blob/283ed3ba786e99f7892342972a3bcc1a9bf3c527/src/resources/filters/quarto-post/dashboard.lua#L262-L284

It happens that our HTML parsing for table does introduced some scaffolding element with hidden class for this specific data-qmd processing https://github.com/quarto-dev/quarto-cli/blob/283ed3ba786e99f7892342972a3bcc1a9bf3c527/src/resources/filters/normalize/extractquartodom.lua#L15-L19

If <span data-qmd is used, it is not a problem because the Dashboard processing is only looking for hidden div.

So this is a conflict between feature.

Not sure of the scafold element needs to be hidden - our other scaffold_element function does not . Ths is related to

https://github.com/quarto-dev/quarto-cli/issues/9489)

Now we know what is happening we need to see what to do at which levels

olivroy commented 2 months ago

Thanks so much for looking into this and for the explanations! Honestly, it took me a while to figure out what actually caused this issue

cderv commented 1 month ago

@rich-iannone this is the issue we discussed about. Possibly this is to be done in Quarto and not just fixing in gt

olivroy commented 1 month ago

@cderv Hi! thanks for targeting this. I just observed a new regression... An example that worked with Quarto 1.4.554, but that no longer works with Quarto 1.4.555... To get around this, I ended up using cols_label() + html() instead of md() to create my gt table. However, my workaround no longer seems to work.

I am guessing this is related to including #9602 in the latest patch: Fix regression with parsed HTML tables not being correctly included in various format like pptx and typst.

The workaround I used to make it work with gt 0.10.0 + Quarto 1.4.554 was to use html() instead of md() inside cols_label() to wrap my column label. But the workaround stopped working with Quarto 1.4.555.

Works in 1.4.554, not in 1.4.555 (to be confirmed) with gt 0.10.1

---
title: "dash-test"
format: dashboard
editor: visual
editor_options: 
  chunk_output_type: console
---

## Heading

```{r}
#| title: a title0
library(gt)
exibble |> 
 gt::gt() |>
  cols_label(
    num = html("<strong>This text is important!</strong>")
  )

I may be mistaken as I still haven't installed 1.4.555 locally, but that's what I am seeing on CI.

cscheid commented 1 month ago

I can't repro on main:

olivroy commented 1 month ago

@cscheid Thanks for checking. Indeed doesn't seem to be an issue in the 1.5 latest prerelease. (However, my full dashboard still fails to render in 1.5 due to another issue #9796 , explanations below) If you can't repro on 1.4.555, I will try to find a better example when I have the chance to install that version. My real-life example is more complex. I will try coming up with something better when I have time. Note that this isn't an issue in 1.5, nor 1.4.554, only 1.4.555 as far as I can tell.

I just tested building my dashboard with latest prerelease. The table doesn't disappear, but elsewhere in my dashboard I have some blocks with eval: false (hack to avoid displaying some elements of my dashboard), but in 1.5 it creates holes in the dashboard. (issue tracked in #9796). So I am wondering what way of doing is future-proof? What way of doing is doomed to break all the time? Ideally, I'd like to avoid having to block my Quarto version as much as possible to pick-up bug fixes and general improvements

cderv commented 1 month ago

I can't repro on main:

@cscheid I am seeing now, this was not mentioned but the original issue is with dev version of gt. In current CRAN release version, md() is using commonmark to internally convert md to html and inserting in the table.

With current dev version, gt::md() is triggering the data-qmd, data-qmd-base64 support for HTML Tables when in Quarto context.

So to reproduce you need to install dev gt pak::pak("rstudio/gt").

And what I described above still applies

gt is setting the attributes on a Div
Quarto catch this div data-qmd attribute and scaffold it with a function that adds a hidden class too
Our dashboard processing process some Divs with hidden class from cell outputs.

I put the https://github.com/quarto-dev/quarto-cli/labels/needs-discussion labels because I think there are several levels here:

Don't scaffold with hidden for the extract dom processing https://github.com/quarto-dev/quarto-cli/blob/283ed3ba786e99f7892342972a3bcc1a9bf3c527/src/resources/filters/normalize/extractquartodom.lua#L15-L19 but rather with _quarto.ast.scaffold_element Related to #9489

--- a/src/resources/filters/normalize/extractquartodom.lua
+++ b/src/resources/filters/normalize/extractquartodom.lua
@@ -14,7 +14,7 @@ function parse_md_in_html_rawblocks()
   return {
     Div = function(div)
       if div.attributes.qmd ~= nil or div.attributes["qmd-base64"] ~= nil then
-        return _quarto.ast.make_scaffold(pandoc.Div, process_quarto_markdown_input_element(div))
+        return _quarto.ast.scaffold_element(process_quarto_markdown_input_element(div))
       end
     end,
     Span = function(span)

Shouldn't Dashboard processing being "scoped" somehow, and not process some Divs that are inside a Table element ? It seems undesired that this Div is catched by our dashboard processing

@olivroy thanks for sharing other feedback. It mixes a little topic, as it could be different.

I can't reproduce the error you see with

---
title: "dash-test"
format: dashboard
keep-md: true
---

## Heading

```{r}
#| title: a title0
library(gt)
exibble |> 
 gt::gt() |>
  cols_label(
    num = html("<strong>This text is important!</strong>")
  )



This gives me correct result with **gt** dev version, using quarto latest 1.5 - it fails in 1.4.555 

And I think you identified #9602 correctly, the patch https://github.com/quarto-dev/quarto-cli/commit/87a2fe8d39b67bce70a47e756e117688a882100b is using the scaffolding which has the hidden class, and trigger this exact bug 

1.5 don't have the issue because we use the other scaffold function that do not add the hidden class
https://github.com/quarto-dev/quarto-cli/blob/305bea0c55fac2ca2e3fd218df6f03014ef04e84/src/resources/filters/normalize/parsehtml.lua#L211

So this is the exact same issue and I did not pay attention enough to this... Sorry for the trouble; 

I'll rename the issue to make the real problem clear to us;

olivroy commented 1 month ago

@cderv Thanks again for your response! Thanks @cscheid for fixing #9796 and pushing v1.5.41. My dashboard renders again as expected with the pre-release version of Quarto and CRAN gt 0.10.1.

I can't reproduce the error you see with

@cderv Let me know if you plan to make other patches for Quarto 1.4 at some point (I will try following progress if possible), If so, I will open a new issue with a reprex. Otherwise I will just leave it as is for now, as I trust that you correctly identified the problem.

cscheid commented 1 month ago

I can't repro on main:

@cscheid I am seeing now, this was not mentioned but the original issue is with dev version of gt.

I see. Thanks for the clarification. I can repro it now.

gt is setting the attributes on a Div

Quarto catch this div data-qmd attribute and scaffold it with a function that adds a hidden class too

Our dashboard processing process some Divs with hidden class from cell outputs.

I put the needs-discussion Issues that require a team-wide discussion before proceeding further labels because I think there are several levels here:

Don't scaffold with hidden for the extract dom processing https://github.com/quarto-dev/quarto-cli/blob/283ed3ba786e99f7892342972a3bcc1a9bf3c527/src/resources/filters/normalize/extractquartodom.lua#L15-L19

but rather with _quarto.ast.scaffold_element

Agreed. Reviewing the code now, I don't understand why we have hidden in make_scaffold. We have a comment about unifying scaffold_element and make_scaffold. I think we should do that by systematically replacing make_scaffold with scaffold_element in all call sites (not now, but in the future)

Shouldn't Dashboard processing being "scoped" somehow, and not process some Divs that are inside a Table element ?

This is pretty intentional:

https://github.com/quarto-dev/quarto-cli/blob/3c0d9f2ceb496e3d22fafecb9de02ce047bb8c94/src/resources/filters/quarto-post/dashboard.lua#L283-L284

So I think the correct solution is really to just replace make_scaffold with scaffold_element.

cderv commented 1 month ago

Thanks.

This is pretty intentional:

I saw that one. I just don't know why we need to do the special processing that follows for hidden divs nested in cell div. I suspect this targeted some specific content and that hidden div inside tables were not part of if.

I guess now this won't happen again soon. But it could still happen if some html widget or else produce some raw html content with somewhere in the children a hidden div.

That is why I questioned this intentional identification. Anyhow if it happens I'll remember this one.

cscheid commented 1 month ago

I suspect this targeted some specific content and that hidden div inside tables were not part of if.

I agree, but the data-qmd scaffolding really shouldn't be hidden at all, so we don't need to make a call on that in order to fix this bug.

quarto-dev / quarto-cli

dashboard processes any hidden Div as cell computation output causing problem with our parsed HTML table processing #9696

Bug description

Steps to reproduce