Figure environment messes up css grid

[andrewpbray]

When you add a caption to a sticky image, the .sticky-col blows up in width and shrinks the .narrative-col. These work:

:::{#cr-trees}
this works: ![a caption](trees.png)
:::

:::{#cr-trees}
![](trees.png)
:::

This does not:

:::{#cr-trees}
![a caption](trees.png)
:::

That is, it appears that a block figure with a caption does not work while an inline figure with a caption and a block figure without a caption do work.

[jimjam-slam]

if I run an example with both a working and a non-working example in the section, I see that:

1. The non-working sticky is a figure that has both a p.page-columns.page-full (with an img inside it) and a figcaption, and
2. All of the sticky's ancestors, from the sticky-col-stack and the .sticky-col all the way up to the main for the entire page, also get .page-columns.page-full.

.page-columns seems to be the one wreaking layout havoc, as it creates a new display: grid (so we have a whole stack of nested grids that aren't wanted).

I'm not whether it's our filter, Quarto or an interaction between them that's going rogue and adding these classes, but that's the root of the issue I think! If it's Quarto, perhaps we can get in early and get the offending ones into a shape that stops this behaviour. Or maybe we can craft an example that reproduces these extra classes without Closeread at all?

Test case: working example + non-working example

```` --- title: Sticky Block Types format: closeread-html: code-tools: true toc: true keep-md: true --- Just about any type of element that can be included in a Quarto doc can be flagged as a sticky. This demo shows many examples. ## Images ::::{.cr-section} This is an image that includes a caption. @cr-images-2 :::{#cr-images-2} This works:  ::: What about this, though? @cr-images-3 :::{#cr-images-3}  ::: :::: ````

[jimjam-slam]

A similar example without Closeread doesn't create the offending classes:

Test case: three examples with plain HTML format

```` --- title: Test test test format: html --- Oh hello! Here are some images with classes: :::{#working} This works:  ::: This also works (no caption:) :::{#alsoworking}  ::: But this doesn't: :::{#notworking}  ::: ````

EDIT: this test case does have .page-columns on .quarto-main, which makes sense, since I think it sets up the primary grid system. Note that the equivalent for part of the page is .column-page, not .page-columns.

[jimjam-slam]

When I render the non-Close read example, the generated HTML file contains a JavaScript DOMContentLoaded event listener that contains this block:

const xrefs = window.document.querySelectorAll('a.quarto-xref');
const processXRef = (id, note) => {
    // Strip column container classes
    const stripColumnClz = (el) => {
      el.classList.remove("page-full", "page-columns");
      if (el.children) {
        for (const child of el.children) {
          stripColumnClz(child);
        }
      }
    }
    stripColumnClz(note)
    # ... additional function definition relating to cross-ref processing, I think
}

The function appears to remove our rogue classes from both the element itself and all its children. But it identifies relevant crossrefs using querySelectorAll('a.quarto-xref').

So it seems like Quarto adds these extra classes for some reason during render and then strips them back out of cross-references and their children on document load using JavaScript. If our crossrefs are being removed by our Lua filters during render (because we don't need the citation itself anymore), it's possible this JS code wouldn't identify them, leaving those classes in.

The question is, is it better to leave the citations in and simply mark them up with some additional class that hides them from visual (and screen reader) while still allowing the Quarto JS to process them, or do we try to write an additional Lua filter late in the render process that removes those classes from children?

I'm going to try to inject some logging into the Quarto JS code to see which crossrefs are identified and stripped in normal operation. It seems weird that the entire main would receive these classes, but it also doesn't seem like it would be a ref that gets identified and stripped. Plus, it's ancestors of the offending image that get the classes, not siblings, so it feels like something has gone wrong travelling up the tree, not down?

[jimjam-slam]

Apologies for the rubberducking comments! Here's the culprit in Quarto's HTML render code:

    const ensureInGrid = (el: Element, setLayout: boolean) => {
      if (processEl(el)) {
        // Add the grid system. Children of the grid system
        // are placed into the body-content column by default
        // (CSS implements this)
        if (
          !el.classList.contains("quarto-layout-row") &&
          !el.classList.contains("page-columns")
        ) {
          el.classList.add("page-columns");
        }

        // Mark full width
        if (setLayout && !el.classList.contains("page-full")) {
          el.classList.add("page-full");
        }

        // Process parents up to the main tag
        const parent = el.parentElement;
        if (parent) {
          ensureInGrid(parent, true);
        }
      }
    };

It traverses up to main from a given element, adding the offending classes along the way. It does not appear to be adding any classes to the non-CR test case but fires repeatedly for the CR test case (note that I've inserted logging statements directly before each classList.add() in the above block here in my Quarto):

> closeread extension retrieved

[ 1/14] guide/authoring-tools.qmd
[ 2/14] guide/index.qmd
[ 3/14] guide/components.qmd
[ 4/14] guide/layouts.qmd
[ 5/14] guide/focus-effects.qmd
[ 6/14] gallery/index.qmd
[ 7/14] gallery/examples/auden-poem/index.qmd
WARN: Adding .page-columns to el
WARN: Adding .page-full to el
[ 8/14] gallery/examples/minards-map/index.qmd
WARN: Adding .page-columns to el
WARN: Adding .page-full to el
WARN: Adding .page-columns to el
WARN: Adding .page-full to el
[ 9/14] gallery/demos/highlighting/index.qmd

processing file: index.qmd

output file: index.knit.md

WARN: Adding .page-columns to el
WARN: Adding .page-full to el
[10/14] gallery/demos/sticky-blocks/index.qmd
WARN: Adding .page-columns to el
WARN: Adding .page-full to el
WARN: Adding .page-columns to el
WARN: Adding .page-full to el
WARN: Adding .page-columns to el
WARN: Adding .page-full to el
WARN: Adding .page-columns to el
WARN: Adding .page-full to el
WARN: Adding .page-columns to el
WARN: Adding .page-full to el
WARN: Adding .page-columns to el
WARN: Adding .page-full to el
WARN: Adding .page-columns to el
WARN: Adding .page-full to el
WARN: Adding .page-columns to el
WARN: Adding .page-full to el
WARN: Adding .page-columns to el
WARN: Adding .page-full to el
[11/14] gallery/demos/zooming/index.qmd
WARN: Adding .page-columns to el
WARN: Adding .page-full to el
[12/14] gallery/demos/ojs-variables/index.qmd
WARN: Adding .page-columns to el
WARN: Adding .page-full to el
[13/14] index.qmd
WARN: Adding .page-columns to el
WARN: Adding .page-full to el
[14/14] reference/index.qmd

Output created: _site/index.html

So it's happening to various degrees with most of our Closeread files, but especially (and most obviously, layout-wise) to the sticky-blocks demo.

[jimjam-slam]

The context this code runs in is designed to ensure that column layout elements have access to the global Quarto grid system (when we've been operating on the assumption that we're overriding Quarto's grid system within our .cr-section by introducing a new display: grid). It:

• runs getColumnLayoutElements() to identify potential column layouts
• excludes body and div#quarto-content
• adds the missing classes for the remaining column layout elements and their parents up to main

getColumnLayoutElements() is just querySelectorAll(kColumnSelector), and the selector is up near the top:

const kColumnSelector =
  '[class^="column-"], [class*=" column-"], aside:not(.footnotes):not(.sidebar), [class*="margin-caption"], [class*=" margin-caption"], [class*="margin-ref"], [class*=" margin-ref"]';

It includes .column-*, which would make sense for .cr-section: we're using it as it's intended, I think. And for most of our pages, that's where it's firing:

[ 7/14] gallery/examples/auden-poem/index.qmd
WARN: >>> Identified column layout to inject classes!
WARN: >>> Tag name: DIV
WARN: >>> Tag ID: <no id>
WARN: >>> Tag classlist: cr-section, column-screen, sidebar-left
WARN: Adding .page-columns to el
WARN: Adding .page-full to el
[ 8/14] gallery/examples/minards-map/index.qmd
WARN: >>> Identified column layout to inject classes!
WARN: >>> Tag name: DIV
WARN: >>> Tag ID: <no id>
WARN: >>> Tag classlist: cr-section, column-screen, sidebar-left
WARN: Adding .page-columns to el
WARN: Adding .page-full to el
WARN: Adding .page-columns to el
WARN: Adding .page-full to el

But when we have an offending image, it has .column-screen on it, which causes it to be identified by this column layout code that propagates the other classes up:

[10/14] gallery/demos/sticky-blocks/index.qmd
WARN: >>> Identified column layout to inject classes!
WARN: >>> Tag name: IMG
WARN: >>> Tag ID: <no id>
WARN: >>> Tag classlist: img-fluid, figure-img, column-screen
WARN: Adding .page-columns to el
WARN: Adding .page-full to el
WARN: Adding .page-columns to el
WARN: Adding .page-full to el
WARN: Adding .page-columns to el
WARN: Adding .page-full to el
WARN: Adding .page-columns to el
WARN: Adding .page-full to el
WARN: Adding .page-columns to el
WARN: Adding .page-full to el
WARN: Adding .page-columns to el
WARN: Adding .page-full to el
WARN: Adding .page-columns to el
WARN: Adding .page-full to el
WARN: Adding .page-columns to el
WARN: Adding .page-full to el
WARN: Adding .page-columns to el
WARN: Adding .page-full to el

I don't think there's any way we can prevent .column-screen being added to the img, so I think we might need some sort of late render process (something that runs after that HTML bootstrap, or potentially in JS on doc load) that removes .column-screen from images and other elements within the .cr-section (because these explicitly should not lay out based on the Quarto layout system). If we can't get in early enough to stop the propagation, we might need to strip the other classes out too, @andrewpbray.

[jimjam-slam]

I tried writing a filter to get in as late as possible in hopes of catching this class:

contributes:
  formats:
    html:
      filters:
        - closeread-before.lua
        - closeread.lua
        - closeread-after.lua
        - at: post-render
          path: closeread-final.lua

-- closeread-final.lua... closeread-before.lua and closeread-after.lua look the same
function print_image_classes(el)
  quarto.log.output("Checking classes on image...")
  for k, v in pairs(el.classes) do
    quarto.log.output("Class: " .. v)
  end
end

quarto.log.output(">>> FINAL <<<")

return {
  Image = print_image_classes
}

But no dice:

[10/14] gallery/demos/sticky-blocks/index.qmd
>>> BEFORE <<<
>>> AFTER <<<
>>> FINAL <<<
Checking classes on image...
Checking classes on image...
Checking classes on image...
Checking classes on image...
Checking classes on image...
Class: img-fluid
Checking classes on image...
Class: img-fluid
WARN: >>> Identified column layout to inject classes!
WARN: >>> Tag name: IMG
WARN: >>> Tag ID: <no id>
WARN: >>> Tag classlist: img-fluid, figure-img, column-screen
WARN: Adding .page-full to el
WARN: Adding .page-full to el
WARN: Adding .page-full to el
WARN: Adding .page-full to el
WARN: Adding .page-full to el
WARN: Adding .page-full to el
WARN: Adding .page-full to el
WARN: Adding .page-full to el
WARN: Adding .page-full to el

(note that the WARN: statements come from debugging statements I've injected into Quarto's HTML render bootstrapping script)

So we might need to use JS... although I'm a little worried about a race condition with the existing code, so we might need to remove all three classes, just to be safe.

[jimjam-slam]

I should note that I thought our Lua code might be adding .column-screen to the image inadvertently, but logging seems to show that that's not the case:

[10/14] gallery/demos/sticky-blocks/index.qmd
>>> Adding .column-screen to element. Pre-Existing attributes:
Attr {
  attributes: AttributeList {}
  classes: List {
    [1] "cr-section"
  }
  clone: function: 0x1406cbba0
  identifier: ""
}

[jimjam-slam]

See d5583d1 in #82 for a first-pass fix! It appears to work, but I'm concerned that elements between main and .cr-section are still receiving classes they oughtn't, which could affect pages that mix .cr-sections and other content. Might need to reach out to the Quarto team to see if this is the best way, or if there's another way to protect content from this code @andrewpbray.

[andrewpbray]

Haha, I love a good rubberducking session

It's a bit late in the day for me to take a run at this, but I'll have a look either tomorrow morn or the following!

[jimjam-slam]

I have to say, I'm absolutely stumped — if I disable the table.insert statements, the image doesn't get the offending classes. So it clearly is us! But when I do logging on that function to see what it's operating on, it's clearly the section element... and yet, the image gets the class and not the section element 🤯

[10/14] gallery/demos/sticky-blocks/index.qmd
*** Making section based on element:
Div

List {[1] "cr-section"}
+++ Starting section class list:
{[1] "cr-section"}
--- New section class list:
{[1] "cr-section", [2] "column-screen", [3] "sidebar-left"}
WARN: >>> Identified column layout to inject classes!
WARN: >>> Tag name: IMG
WARN: >>> Tag ID: <no id>
WARN: >>> Tag classlist: img-fluid, figure-img, column-screen
WARN: Adding .page-full to el
WARN: Adding .page-full to el
WARN: Adding .page-full to el
WARN: Adding .page-full to el
WARN: Adding .page-full to el
WARN: Adding .page-full to el
WARN: Adding .page-full to el
WARN: Adding .page-full to el
WARN: Adding .page-full to el

[andrewpbray]

If you take a look at the most recent commit to the lua file (https://github.com/qmd-lab/closeread/commit/2798e0031d53170187f07da02f95e2d5397d5764), that was me trying to allow users to pass layout classes (I can't recall right now the precise use-case I had in mind 😞 ). Looking at this change, though, and thinking back, this issue with captioned figs predates this.

[andrewpbray]

I wonder if later in the Lua filter we're propagating classes down to sticky els and that class is coming with it (and... for some reason this only effects a particularly structured Figure with a caption). I'll try doing some logging at the very end of the filter to see if those classes show up.

If they don't, sure seems like it's gotta be an interaction between our AST and downstread Quarto processing.

[andrewpbray]

I can confirm your observation: at the end of our filter, .column-screen appears on the div with .cr-section but no where else (not on the figure / image).

[jimjam-slam]

Filed as https://github.com/quarto-dev/quarto-cli/issues/10899 now that we've made a proper reprex and confirmed this as a Quarto problem!

[jimjam-slam]

Considering Carlos's response, I wonder if we should simply manually add styles to the section in lieu of using column-section...

[jimjam-slam]

Based on discussion in https://github.com/quarto-dev/quarto-cli/issues/10899, I'm going to try replacing .column-screen with manual CSS styles to the same effect tonight. If it solves our issue and doesn't muck anything else up, that might be the easiest course, and it'll give Carlos the space to work the underlying issue on their end.

[jimjam-slam]

I've pushed a partial fix in #82. https://github.com/qmd-lab/closeread/pull/82/commits/65ec6a07a43690ea3d51ceadab3204bc1a8fef53 is the core of it: replacing .column-screen with its manual styles.

However, it turns out Quarto makes .page-column.page-full propagate up from figures and other content for a good reason: grid templates can only position their direct children. That means that, at minimum, the main element that contains the content of a Quarto doc needs to have those classes. https://github.com/qmd-lab/closeread/pull/82/commits/dcfda9cc43e6fccd6487fdfbaf43011f69b1177f adds them.

The following commit, https://github.com/qmd-lab/closeread/pull/82/commits/7e1a9a02bee8d862e0abf2d9b92ad98a5885fbaf, shows the impact of this on our sticky block demo. It works when .cr-section is used at the top of a page, but if you put one underneath a second-level header, it fails because Quarto takes the header and its associated content inside a section — now the .cr-section is no longer a direct descendent. if you add .page-columns.page-full to the section, things work again.

I think we can adapt the JS code I wrote in the second commit to work with sections, but there may be some edge cases to consider, like sidebar content. So this touches on #91 too. Perhaps the easy solution is the option of a less flexible layout that sticks to the current document width? What do you think, @andrewpbray?

[jimjam-slam]

Ironically, we're basically reimplementing the Quarto logic that causes the problem in the first place (but exempting the .cr-section itself from it) 🤣

[andrewpbray]

I think switching from .column-screen to manual styling is the way to go here!

I left a few inline questions in the PR and as I've been rubber ducking those, I'm coming around to your realization: that we'll be in effect reimplementing the Quarto logic around layouts. The behavior that seems natural to me as a generic Quarto user would be:

1. If the document has no sidebars, the layout is analogous to .column-screen. We'd achieve that by adding the inline styles as you have in the filter and then being sure that the <main> and every .cr-section has the .page-full.page-columns classes added via js.
2. If the document has a sidebar, the js would not add .page-full.page-column classes to anything.

We could imagine in the future allowing the user to override 2 in a similar manner that Quarto provides element layout classes like .column-screen (maybe .cr-column-screen), which would moderate the behavior of the .js.

Does this make sense?

[jimjam-slam]

@andrewpbray I think that makes sense! I would also argue that it might be worth restricting layouts to overlay ones (ie. no CR sidebars) if there's a Quarto sidebar on the page, but that's a bit more radical — what do you think?

We also need to work out how to detect the presence of a sidebar at render time (I would assume it's much easier at page load time!).

In the mean time, I've pushed a quick commit to revert to using the new class .cr-column-screen to mimic .column-screen. Still need to add JS logic (likely lifting the Quarto logic wholesale!) to propagate .page-full.page-columns back.

We probably also need some tests for when the Quarto user specifies a different page layout for the whole document in the YAML!

[andrewpbray]

I would also argue that it might be worth restricting layouts to overlay ones (ie. no CR sidebars) if there's a Quarto sidebar on the page, but that's a bit more radical — what do you think?

Instead of forbidding it, how about changing .overlay-center to be the default if there's a sidebar? One reason for not removing sidebar stuff whole hog is that I can imagine a user with a sidebar who's written some nifty JS to hide that col when the user clicks a button and they'd like the layout to respond by filling that space. In that setting, a sidebar layout might work just fine.

This is a weakly held preference, though - I'm game for being more prescriptive!

[andrewpbray]

I guess I had imagined detecting the sidebar during render, but it's possible that the document will have meta data accessible to the filter that will allow us to infer that. I'll take a peek.

[jimjam-slam]

Instead of forbidding it, how about changing .overlay-center to be the default if there's a sidebar? One reason for not removing sidebar stuff whole hog is that I can imagine a user with a sidebar who's written some nifty JS to hide that col when the user clicks a button and they'd like the layout to respond by filling that space. In that setting, a sidebar layout might work just fine.

Yes, I like this idea!

[jimjam-slam]

I guess I had imagined detecting the sidebar during render, but it's possible that the document will have meta data accessible to the filter that will allow us to infer that. I'll take a peek.

I'm honestly not sure — it could depend on several options. There must be at least some JS going at run time, though, because Quarto temporarily hides the TOC from the sidebar when you scroll past content positioned in the caption!

[andrewpbray]

Well there is some signal available in the lua filter. docs/gallery/demos/sticky-blocks/index.qmd shows this under the ast meta:

include-before: List {
    [1] Blocks {
      [1] RawBlock {
        clone: function: 0x7fb6e013ef20
        format: "html"
        show: function: 0x7fb6e01370d0
        text: "<div id="quarto-search-results"></div>
  <header id="quarto-header" class="headroom fixed-top">
    <nav class="navbar navbar-expand-lg " data-bs-theme="dark">
      <div class="navbar-container container-fluid">
      <div class="navbar-brand-container mx-auto">
    <a class="navbar-brand" href="/index.html">
    <span class="navbar-title">closeread</span>
    </a>
  </div>
            <div id="quarto-search" class="" title="Search"></div>
          <button class="navbar-toggler" type="button" data-bs-toggle="collapse" data-bs-target="#navbarCollapse"
  aria-controls="navbarCollapse" role="menu" aria-expanded="false" aria-label="Toggle navigation"
  onclick="if (window.quartoToggleHeadroom) { window.quartoToggleHeadroom(); }">
  <span class="navbar-toggler-icon"></span>
</button>
          <div class="collapse navbar-collapse" id="navbarCollapse">
            <ul class="navbar-nav navbar-nav-scroll me-auto">
  <li class="nav-item">
    <a class="nav-link" href="/index.html"> 
<span class="menu-text">Home</span></a>
  </li>  
  <li class="nav-item">
    <a class="nav-link" href="/guide/index.html"> 
<span class="menu-text">Guide</span></a>
  </li>  
  <li class="nav-item">
    <a class="nav-link" href="/gallery/index.html"> 
<span class="menu-text">Gallery</span></a>
  </li>  
  <li class="nav-item">
    <a class="nav-link" href="/reference/index.html"> 
<span class="menu-text">Reference</span></a>
  </li>  
  <li class="nav-item dropdown ">
    <a class="nav-link dropdown-toggle" href="#" id="nav-menu-help" role="link" data-bs-toggle="dropdown" aria-expanded="false" >
 <span class="menu-text">Help</span>
    </a>
    <ul class="dropdown-menu" aria-labelledby="nav-menu-help">    
        <li>
    <a class="dropdown-item" href="https://github.com/qmd-lab/closeread/issues"><i 
  class="bi bi-bug" 
  role="img" 
>
</i> 
 <span class="dropdown-text">Report a Bug</span></a>
  </li>  
        <li>
    <a class="dropdown-item" href="https://github.com/qmd-lab/closeread/discussions"><i 
  class="bi bi-chat-right-text" 
  role="img" 
>
</i> 
 <span class="dropdown-text">Ask a Question</span></a>
  </li>  
    </ul>
  </li>
</ul>
          </div> <!-- /navcollapse -->
            <div class="quarto-navbar-tools">
    <a href="https://github.com/qmd-lab/closeread"  title="Closeread GitHub" class="quarto-navigation-tool px-1" aria-label="Closeread GitHub"><i class="bi bi-github"></i></a>
</div>
      </div> <!-- /container-fluid -->
    </nav>
</header>
<!-- content -->
<div id="quarto-content" class="quarto-container page-columns page-rows-contents page-layout-full page-navbar">
<!-- sidebar -->
<!-- margin-sidebar -->
    <div id="quarto-margin-sidebar" class="sidebar margin-sidebar">
        <div id="quarto-toc-target"></div>
    </div>
<!-- main -->
<main class="content" id="quarto-document-content">
"
        walk: function: 0x7fb6e0142e70
      }
    }
  }

Writing lua to parse that string for quarto sidebars doesn't seem ideal - it'd be easier/safer to parse the DOM in js seems like - but layout classes do seem like something that ideally we'd do at the filter stage. And maybe the set of relevant classes is sufficiently stable and specific so that direct grepping the stringf or quarto-margin-sidebar would work. What is your inclincation: tackling this in the filter or the JS?

You're definitely right about the JS at runtime: you can see elsewhere in the meta the JS that's inserted into the HTML file via a RawBlock that does exactly this.

[jimjam-slam]

Just noting following our discussion that we're going to make a separate PR for the sidebar stuff (#91)!

[jimjam-slam]

Merged!