Support complex figures. #7704

tarleb · 2021-11-20T11:27:59Z

It provides a specific representation for figures in the pandoc's AST.
It uses the SimpleFigure pattern synonym to replace the previous
construction:

[Para [Image ("",[],[]) [Str "CAP2"] ("../media/rId25.jpg","fig:")]]

tarleb · 2021-11-20T11:32:39Z

This is rebased and updated version of #7364. @argent0, can you take a look and check that I didn't changed something against your intentions?

There are mostly just small changes compared to the commit in #7364: The tests are updated to match the new native output, and the changes to the test suite have been reverted (as I think those would warrant a separate PR). Also, the Lua marshaling code for figures needed an update to match the new mechanism.

Thanks again for the great GSoC work @argent0!

jgm · 2021-11-20T17:53:28Z

Note: When this gets merged, we will also need a new pandoc-types release, and it would be a good idea at that point to do jgm/pandoc-types#91 as well.

MANUAL.txt

src/Text/Pandoc/Readers/HTML.hs

argent0 · 2021-11-20T20:58:11Z

src/Text/Pandoc/Writers/Markdown.hs

@@ -600,6 +600,8 @@ blockToMarkdown' opts (OrderedList (start,sty,delim) items) = do
 blockToMarkdown' opts (DefinitionList items) = do
  contents <- inList $ mapM (definitionListItemToMarkdown opts) items
  return $ mconcat contents <> blankline
+blockToMarkdown' opts (Figure (ident, classes, kvs) _ body) =
+  blockToMarkdown' opts (Div (ident, ["figure"] `union` classes, kvs) body)


Here I added the "figure" class to the div. I'd like to know how this aligns with the goal of keeping English words out of markdown.

argent0 · 2021-11-20T20:59:16Z

src/Text/Pandoc/Writers/MediaWiki.hs

@@ -210,6 +211,9 @@ blockToMediaWiki x@(DefinitionList items) = do
        contents <- local (\s -> s { listLevel = listLevel s <> ";" }) $ mapM definitionListItemToMediaWiki items
        return $ vcat contents <> if null lev then "\n" else ""

+blockToMediaWiki (Figure (ident, classes, kvs) _ body) =
+  blockToMediaWiki (Div (ident, ["figure"] `DL.union` classes, kvs) body)


src/Text/Pandoc/Writers/Muse.hs

argent0 · 2021-11-20T21:12:56Z

src/Text/Pandoc/Writers/TEI.hs

@@ -191,6 +191,7 @@ blockToTEI _ HorizontalRule = return $
  selfClosingTag "milestone" [("unit","undefined")
                             ,("type","separator")
                             ,("rendition","line")]
+blockToTEI _ (Figure {}) = return empty


Ditto

blockToTEI opts (Figure attr _ bs) = blockToTEI opts (Div attr bs)

argent0 · 2021-11-20T21:20:10Z

src/Text/Pandoc/Writers/Texinfo.hs

+  floatType [SimpleFigure {}] = " Figure"
+  floatType [Table {}] = " Table"
+  floatType _ = ""
+


The modifications in the output here are backed by this documentation.

argent0 · 2021-11-20T21:24:42Z

src/Text/Pandoc/Writers/XWiki.hs

+blockToXWiki (Figure attr _ body) = do
+  content <- blockToXWiki $ Div attr body
+  return $ intercalate content ["(((\n", ")))"]
+


This creates a group

jgm · 2021-11-20T22:30:23Z

@tarleb @argent0 I'm tempted to go for a wholesale move to using this new Figure constructor, rather than introducing it under an extension.

Yes, this would mean that some existing filters would need to be rewritten, but it would avoid pain in the future. I worry that a situation in which some filters are presupposing Figure elements and others aren't might produce a lot of confusion.

If we did this, it would mean replacing all uses of the "implicit figures" pattern (Para [Image ... "fig:"]) with Figure. It would be relatively easy to perform this switch in the readers; all we'd have to do is change simpleFigure so it produces a Figure. The other piece would be revising the writers so they all handle a Figure intelligently. That would be more work, I gather.

Thoughts?

argent0 · 2021-11-21T00:50:01Z

When I was writing this I had in mind two different concepts that became:

simpleFigure: a graph, a picture, with an optional caption. Position in the document relatively more important.
Figure: A sub-document, a document withing a document, with an optional paragraph. Position in the document relatively less important.

(theses are the concepts used in others formats BTW, like JATS and LaTeX)

I was thinking in using a constructor (Block) for the simpleFigure, and most of the work towards that goal has already been done.

But, we would need two constructors.

jgm · 2021-11-21T06:40:09Z

I would prefer to avoid having two separate figure concepts. Why can't a simple figure just be a special case of Figure (with a single image as its contents, and a single Plain as its caption)?

tarleb · 2021-11-21T09:55:52Z

@argent0 you still have full collaborator access to my fork, please don't hesitate to make changes if needed.

tarleb · 2021-11-21T10:13:13Z

Or, if you prefer, you could also update your original PR. E.g.,

git remote add -f tarleb https://github.com/tarleb/pandoc
git switch figures-gsoc
git reset --hard tarleb/figures-gsoc
git push --force-with-lease

I'd close this one then.

argent0 · 2021-11-21T15:29:01Z

I would prefer to avoid having two separate figure concepts. Why can't a simple figure just be a special case of Figure (with a single image as its contents, and a single Plain as its caption)?

Yes, this can be a better alternative. Something like:

Figure attr caption body: for full figures

Figure attr caption [Image ...] for a simpleFigure,why would there be a need to restrict the caption to single Para though?

@argent0 you still have full collaborator access to my fork, please don't hesitate to make changes if needed.

I'll make the contributions there then (rather than updating my PR).

jgm · 2021-11-22T16:52:26Z

I'm actually not sure why we need any conceptual distinction between figure types at all. There will be simpler figures and more complex ones, because sometimes the [Block] contained in the Figure element will be simple and sometimes it will be more complex. But they are both figures in the same sense.

LaTeX just has one figure environment; similarly HTML. The issue of whether figures float is a complex one; in LaTeX all figures float, though you can specify preferences for locations. In HTML they don't, because it's not a paged format so floating doesn't make so much sense. Pandoc is going to have to abstract from those differences.

mb21 · 2021-11-22T16:59:54Z

Is this idea of "two figure types" going back to this comment by despresc? I don't think this was ever discussed properly through...

argent0 · 2021-11-22T19:02:59Z

Is this idea of "two figure types" going back to this comment by despresc? I don't think this was ever discussed properly through...

Yes, it's partially based on that discussion. I was also mainly thinking about "articles" which are only a part of pandoc's scope.

mb21 · 2021-11-23T08:09:07Z

Ah, the pandoc-types commit associated with this PR doesn't have a Figure AST element yet. So far we just have the pattern synonym, correct?

@jgm is the plan to merge this without having settled on the details of the Figure AST element, or? I'm happy to provide input if needed (but unfortunately currently short on time to get my hands dirty with the code).

tarleb · 2021-11-23T09:13:00Z

The type constructor went through a good bit of evolution. I squashed all changes into a cleaned-up, single commit here: jgm/pandoc-types#94. We aimed to keep the focus of the GSoC on figures and didn't want to take tables into account at that time.

jgm · 2021-11-24T18:18:21Z

I'm not sure where we stand on my original query, so reposting:

@tarleb @argent0 I'm tempted to go for a wholesale move to using this new Figure constructor, rather than introducing it under an extension.

Yes, this would mean that some existing filters would need to be rewritten, but it would avoid pain in the future. I worry that a situation in which some filters are presupposing Figure elements and others aren't might produce a lot of confusion.

If we did this, it would mean replacing all uses of the "implicit figures" pattern (Para [Image ... "fig:"]) with Figure. It would be relatively easy to perform this switch in the readers; all we'd have to do is change simpleFigure so it produces a Figure. The other piece would be revising the writers so they all handle a Figure intelligently. That would be more work, I gather.

Thoughts?

As I understand it (I haven't looked back recently, so maybe things changed) this code does not change the simpleFigure builder to create a Figure block, and it continues to gate use of the Figure block under extensions.

tarleb · 2021-11-24T18:23:14Z

I'd be in favor of an all-in approach.

jgm · 2021-11-24T21:08:26Z

If we're doing all-in, then we'd need to make sure all writers handle Figure elements properly.

I don't think we're there yet, right?

(Eventually we'd also want to remove the old support for "single image in para with title beginning with fig:", though maybe it's better to keep this support in the writers for now...? But in any case, proper functioning of Figure in the writer should not depend on this.)

tarleb · 2021-11-28T17:32:05Z

@argent0 I think that, since this the PR contains the fruits of your work, it would be great if you could bring this to completion. However, if you think you might not have the bandwidth for this at the moment, then that's fine, too. Let me know in that case, and I can take it from there.

argent0 · 2021-12-08T22:16:08Z

If we're doing all-in, then we'd need to make sure all writers handle Figure elements properly.
I don't think we're there yet, right?

@jgm all writers handle Figure elements either in:

A format specific way.
A fallback Div-like way.

(Eventually we'd also want to remove the old support for "single image in para with title beginning with fig:", though maybe it's better to keep this support in the writers for now...? But in any case, proper functioning of Figure in the writer should not depend on this.)

Concretely. Are we talking about changing the pattern from:

pattern SimpleFigure :: Attr -> [Inline] -> Target -> Block
pattern SimpleFigure attr figureCaption tgt <-
    Para [Image attr figureCaption
        (isFigureTarget -> Just tgt)]  where
  SimpleFigure attr figureCaption tgt =
    Para [Image attr figureCaption (second ("fig:" <>) tgt)]

to

pattern SimpleFigure :: Attr -> [Inline] -> Target -> Block
pattern SimpleFigure attr figureCaption tgt <-
    Figure attr (Caption Nothing [Para figureCaption]) [Para [Image ?? ?? tgt]]
  SimpleFigure attr figureCaption tgt =
    Figure attr (Caption Nothing [Para figureCaption]) [Para [Image ?? ?? tgt]]

? It doesn't look right to me.

jgm · 2021-12-09T06:06:28Z

I wasn't thinking that SimpleFigure would need to change; rather, that we just wouldn't include clauses that match on SimpleFigure in the writers. Only real Figure blocks would render as figures. Does this make sense? (SimpleFigure would no longer have any real use.)

On the reader side, the markdown reader would produce a Figure block if implicit_figures is enabled and an image is alone in its paragraph.

tarleb · 2022-12-09T19:11:01Z

Is there a reason for this asymmetry? Maybe FB2 writer should behave like the HTML writer and not add the caption as alt text?

I seem to remember that FB2 figures can only contain a single image, and this was an attempt to create images while preserving as much info as possible. I wanted to check again, but the page with the FB2 spec is down, and I can't find it anywhere else.

tarleb · 2022-12-09T19:12:15Z

I didn't check the code here, but obviously we should only use HTML if raw_html is enabled; otherwise we could just emit the figure contents (perhaps in a div)?

I think I forgot about that; fill fix it with the next push.

tarleb · 2022-12-09T19:15:41Z

One problem is that now there is no good way to represent complex figures in Markdown. It is tempting to add a figure_divs extension that just puts them in Divs with class figure. Maybe that isn't the ideal Markdown syntax for figures, but having something might be better than having nothing.

As an additional data point: Quarto treats divs as figures if the id starts with fig-. I like this because it's reminiscent of the old convention of treating images with fig: as title prefix as figures.

jgm · 2022-12-21T05:05:39Z

HTML writer: the alt text is no longer constructed from the caption,
as was the case with implicit figures. This reduces duplication, but
comes at the risk of images that are missing alt texts. Authors should
take care to provide alt texts for all images.

This raised a question for me. I could probably answer it by reading the code, but maybe you can just tell me. How does implicit_figures work in the context of these changes? Does it create a Figure element on the reader side? Or does it get applied on the writer side, special treatment of Para [Image], so that figures created with implicit_figures are handled completely differently?

In fact, there doesn't seem to be any other way to create a figure presently, in markdown. And implicit_figures doesn't give you a way to set the alt text independently of the caption, does it? So does the above mean that if you go markdown -> HTML, your figures will suddenly lack alt text where they had it previously? That seems undesirable, especially if there is no alternative.

tarleb · 2022-12-23T22:37:57Z

I was about to say that implicit figures could we written with an explicit alt attribute, as in ![Caption](img.png){alt="description"}, but that's not true: all image attributes are currently transferred to the Figure element, so the alt text would be lost. That's not good.

A possible solution could be to set the alt attribute to the stringified caption if the image wouldn't have ~~a caption~~an alt value otherwise. Also, we should keep part of the attributes on the Image element instead of setting those attributes on the figure. Maybe only the ID should be transferred, so classes and key-value attributes (like width, height) would be left on the image.

jgm · 2022-12-24T05:08:45Z

A possible solution could be to set the alt attribute to the stringified caption if the image wouldn't have a captionan alt value otherwise.

That sounds sensible.

Also, we should keep part of the attributes on the Image element instead of setting those attributes on the figure. Maybe only the ID should be transferred, so classes and key-value attributes (like width, height) would be left on the image.

Maybe we could use current behavior of the HTML renderer as a guide? What gets transferred to the figure element currently?

jgm · 2022-12-29T05:40:09Z

Looks like the HTML reader currently ignores attributes on the figure element but passes those on the img element through to the image.

We do need to figure out some way to avoid these changes breaking current workflows with implicit_figure. That is my main hesitation, currently, about merging.

tarleb · 2023-01-10T17:35:08Z

The Markdown reader now uses the image description as figure caption, while also adding it to the image. This would lead to duplication in HTML, so we check for this case in the HTML writer and add the aria-hidden attribute if caption and image description are equal. The old behavior will be preserved this way for most people.

test/fb2/images.fb2

src/Text/Pandoc/Shared.hs

test/command/4677.md

test/writer.docbook5

test/writer.xwiki

jgm · 2023-01-13T00:49:54Z

test/command/6925.md

@@ -23,12 +23,12 @@ a
 ::: thm
 **Theorem 1**. *a*

-![image](1.png)
+![](1.png)


Here the alt text disappears -- won't that mean we no longer have a figure? An implicit_figure needs a non-empty alt text.

src/Text/Pandoc/Writers/Markdown.hs

Thanks and credit go to Aner Lucero, who laid the groundwork for this feature in the 2021 GSoC project. He contributed many changes, including modifications to the readers for HTML, JATS, and LaTeX, and to the HTML and JATS writers. Shared (Albert Krewinkel): - The new function `figureDiv`, exported from `Text.Pandoc.Shared`, offers a standardized way to convert a figure into a Div element. Readers (Aner Lucero): - HTML reader: `<figure>` elements are parsed as figures, with the caption taken from the respective `<figcaption>` elements. - JATS reader: The `<fig>` and `<caption>` elements are parsed into figure elements, even if the contents is more complex. - LaTeX reader: support for figures with non-image contents and for subfigures. - Markdown reader: paragraphs containing just an image are treated as figures if the `implicit_figures` extension is enabled. The identifier is used as the figure's identifier and the image description is also used as figure caption; all other attributes are treated as belonging to the image. Writers (Aner Lucero, Albert Krewinkel): - DokuWiki, Haddock, Jira, Man, MediaWiki, Ms, Muse, PPTX, RTF, TEI, ZimWiki writers: Figures are rendered like Div elements. - Asciidoc writer: The figure contents is unwrapped; each image in the the figure becomes a separate figure. - Classic custom writers: Figures are passed to the global function `Figure(caption, contents, attr)`, where `caption` and `contents` are strings and `attr` is a table of key-value pairs. - ConTeXt writer: Figures are wrapped in a "placefigure" environment with `\startplacefigure`/`\endplacefigure`, adding the features caption and listing title as properties. Subfigures are place in a single row with the `\startfloatcombination` environment. - DocBook writer: Uses `mediaobject` elements, unless the figure contains subfigures or tables, in which case the figure content is unwrapped. - Docx writer: figures with multiple content blocks are rendered as tables with style `FigureTable`; like before, single-image figures are still output as paragraphs with style `Figure` or `Captioned Figure`, depending on whether a caption is attached. - DokuWiki writer: Caption and "alt-text" are no longer combined. The alt text of a figure will now be lost in the conversion. - FB2 writer: The figure caption is added as alt text to the images in the figure; pre-existing alt texts are kept. - ICML writer: Only single-image figures are supported. The contents of figures with additional elements gets unwrapped. - HTML writer: the alt text is no longer constructed from the caption, as was the case with implicit figures. This reduces duplication, but comes at the risk of images that are missing alt texts. Authors should take care to provide alt texts for all images. Some readers, most notably the Markdown reader with the `implicit_figures` extension, add a caption that's identical to the image description. The writer checks for this and adds an `aria-hidden` attribute to the `<figcaption>` element in that case. - JATS writer: The `<fig>` and `<caption>` elements are used write figures. - LaTeX writer: complex figures, e.g. with non-image contents and subfigures, are supported. The `subfigure` template variable is set if the document contains subfigures, triggering the conditional loading of the *subcaption* package. Contants of figures that contain tables are become unwrapped, as longtable environments are not allowed within figures. - Markdown writer: figures are output as implicit figures if possible, via HTML if the `raw_html` extension is enabled, and as Div elements otherwise. - OpenDocument writer: A separate paragraph is generated for each block element in a figure, each with style `FigureWithCaption`. Behavior for single-image figures therefore remains unchanged. - Org writer: Only the first element in a figure is given a caption; additional block elements in the figure are appended without any caption being added. - RST writer: Single-image figures are supported as before; the contents of more complex images become nested in a container of type `float`. - Texinfo writer: Figures are rendered as float with type `figure`. - Textile writer: Figures are rendered with the help of HTML elements. - XWiki: Figures are placed in a group. Co-authored-by: Aner Lucero <4rgento@gmail.com>

jgm · 2023-01-13T17:13:16Z

Taking a deep breath...here we go!

tarleb · 2023-01-13T18:32:54Z

I got to admit, I'm properly scared. Hoping for timely feedback from the Quarto dev team.

tarleb · 2023-01-13T18:39:54Z

Congratulations @argent0, and thanks again for your work!

tarleb force-pushed the figures-gsoc branch from 892a0a9 to fada1d3 Compare November 20, 2021 11:35

jgm reviewed Nov 20, 2021

View reviewed changes

MANUAL.txt Outdated Show resolved Hide resolved

tarleb force-pushed the figures-gsoc branch 3 times, most recently from 15fdd56 to 8520010 Compare November 20, 2021 20:27

argent0 reviewed Nov 20, 2021

View reviewed changes

src/Text/Pandoc/Readers/HTML.hs Outdated Show resolved Hide resolved

argent0 reviewed Nov 20, 2021

View reviewed changes

argent0 suggested changes Nov 20, 2021

View reviewed changes

argent0 force-pushed the figures-gsoc branch from 4f43a2a to 35899b4 Compare November 21, 2021 20:20

tarleb mentioned this pull request Nov 23, 2021

[API change] Add Figure block constructor jgm/pandoc-types#94

Merged

tarleb force-pushed the figures-gsoc branch from a63b636 to ca7ebbb Compare December 12, 2022 10:39

tarleb force-pushed the figures-gsoc branch from ca7ebbb to edb2eee Compare January 10, 2023 17:29

tarleb force-pushed the figures-gsoc branch from edb2eee to b524ff2 Compare January 10, 2023 19:19