Figure environment messes up css grid #81
Comments
|
if I run an example with both a working and a non-working example in the section, I see that:
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
|
|
A similar example without Closeread doesn't create the offending classes: Test case: three examples with plain HTML format
EDIT: this test case does have |
|
When I render the non-Close read example, the generated HTML file contains a JavaScript 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 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 |
|
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 So it's happening to various degrees with most of our Closeread files, but especially (and most obviously, layout-wise) to the |
|
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
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 But when we have an offending image, it has I don't think there's any way we can prevent |
|
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: (note that the 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. |
|
I should note that I thought our Lua code might be adding |
|
See d5583d1 in #82 for a first-pass fix! It appears to work, but I'm concerned that elements between |
|
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! |
|
I have to say, I'm absolutely stumped — if I disable the |
|
If you take a look at the most recent commit to the lua file (2798e00), 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. |
|
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. |
|
I can confirm your observation: at the end of our filter, |
|
Filed as quarto-dev/quarto-cli#10899 now that we've made a proper reprex and confirmed this as a Quarto problem! |
jimjam-slam
added
the
filed-upstream
|
Considering Carlos's response, I wonder if we should simply manually add styles to the section in lieu of using |
|
Based on discussion in quarto-dev/quarto-cli#10899, I'm going to try replacing |
|
I've pushed a partial fix in #82. 65ec6a0 is the core of it: replacing However, it turns out Quarto makes The following commit, 7e1a9a0, shows the impact of this on our sticky block demo. It works when 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? |
|
Ironically, we're basically reimplementing the Quarto logic that causes the problem in the first place (but exempting the |
|
I think switching from 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:
We could imagine in the future allowing the user to override 2 in a similar manner that Quarto provides element layout classes like Does this make sense? |
|
@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 We probably also need some tests for when the Quarto user specifies a different page layout for the whole document in the YAML! |
Instead of forbidding it, how about changing This is a weakly held preference, though - I'm game for being more prescriptive! |
|
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. |
Yes, I like this idea! |
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! |
|
Well there is some signal available in the lua filter. 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 You're definitely right about the JS at runtime: you can see elsewhere in the |
|
Just noting following our discussion that we're going to make a separate PR for the sidebar stuff (#91)! |
|
Merged! |
When you add a caption to a sticky image, the
.sticky-colblows up in width and shrinks the.narrative-col. These work::::{#cr-trees} this works:  ::::::{#cr-trees}  :::This does not:
:::{#cr-trees}  :::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.
The text was updated successfully, but these errors were encountered: