Make images block nodes

[mejo-]

Use markdown-it-image-figures to render standalone images inside
<figure> elements instead of <p> when markdown source gets parsed
into HTML. This allows to treat images as block elements by TipTap.

Also keep support for inline images to not break markdown files that
have inline images.

• Resolves: Attachments are formatted together with the next paragraph #2873
• Target version: master

Summary

[mejo-]

Mh, seems like inserting an image as first element in a new document is broken now 😢

[mejo-]

So @julien-nc and me spent some three more hours debugging the main remaining issue with this PR: inserting a (block node) image as first element into an empty editor breaks due to us doing this.$editor.chain().setImage().insertContent('<br />').focus().run() here.

It turns out to be a Tiptap (or maybe even ProseMirror) bug. I reported ueberdosis/tiptap#3355 upstream.

[cypress]

---

Test summary

106 • 0 • 0 • 0 • 2

---

Run details

Project	Text
Status	Passed
Commit	35f1e2e ℹ️
Started	Nov 15, 2022 7:30 AM
Ended	Nov 15, 2022 7:36 AM
Duration	06:01 💡
OS	Linux Ubuntu -
Browser	Electron 106

View run in Cypress Dashboard ➡️

---

Flakiness

	workspace.spec.js		1
1	Workspace > formats text
	sections.spec.js		1
1	Content Sections > Heading anchors > Anchor scrolls into view

---

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

[mejo-]

After some further debugging, we came up with a mostly working solution that

• Always adds a newline when inserting an image and places the cursor after that newline (like before)
• Works well when inserting several images at once
• Works well with dragging & dropping several images
• Has a small issue when inserting an image in between text

@max-nextcloud and me agreed that it is better to get this merged now and follow-up by looking into the underlying Tiptap issue later on.

@max-nextcloud are you ok with merging this as is, or do you want to further look into it?

[juliusknorr]

Can you quickly summarize the remaining issue, so we can file this as a follow up?

[mejo-]

Can you quickly summarize the remaining issue, so we can file this as a follow up?

Sure, sorry for not doing so in the first place 🙈

So the fix involves moving the cursor two steps further after inserting the image and before inserting the hard break (if the image is not inserted as first element, which needs other special treatment). This is necessary because per default the cursor is in front of the image after inserting it, but we want to add the hard break afterwards.

If you insert an image in between text, this results in the linebreak being inserted two characters further for the moment. I think that's an acceptable tradeoff given that images are block nodes now and most likely it will not happen that often that people insert them in the middle of text anyway.

Here's the relevant code:

text/src/components/Editor/MediaHandler.vue

Lines 199 to 209 in 02dc2ba

	const selection = this.$editor.view.state.selection
	if (!selection.empty) {
	// If inserted image is first element, it is selected and would get overwritten by
	// subsequent editor inserts (see tiptap#3355). So unselect the image by placing
	// the cursor at the end of the selection.
	this.$editor.commands.focus(selection.to)
	} else {
	// Place the cursor after the inserted image node
	this.$editor.commands.focus(selection.to + 2)
	}

If you have a quick idea how to detect that the cursor (i.e. the focus, this.$editor.view.state.selection) is in the middle of some text and not at the end of a paragraph, we could add this as another special handling. But instead of waiting longer and spending another bunch of hours on this minor issue, my suggestion would be to merge this PR as is now 😉

[max-nextcloud]

/compile

[max-nextcloud]

@mejo- There's still a test failing.

I'm not sure what to make of this. I think we are adding two new lines on purpose in the toMarkdown function of the block image node.

Is this really required. I think it's unfortunate if we change the markdown syntax without any changes being made to the document. Why are those newlines needed? Can't we say every line that only contains an image is considered a block image even if no empty lines follow?

Ahh... i guess markdown parsers will then think the line is part of a larger paragraph... Hmm... difficult.

[max-nextcloud]

I think i will exclude 2f7ff70 for now so images still render the way they used to even if inline.

[max-nextcloud]

I think this can be merged now. I created issues for the remaining things and even if they do not get fixed this is an improvement over the status quo.

[max-nextcloud]

/compile

[max-nextcloud]

@juliushaertl @mejo- I'm not sure if we should backport this.

Right now in 25 images and attachments are often in the same paragraph as the following content. Turning that content into say a heading then also toggles the image as it's part of that paragraph. I think that's pretty bad. At the same time this change is pretty profound and we have not really tested it so much. It will also require a bunch of follow up changes. I don't think they would need to be backported - but still.

I wonder if there would be a less intrusive fix we could apply to stable25.

[mejo-]

/backport 0c496fa,4b63cb684897f5512d86ed00cf290b3fffa1df43,5ee1d6b7c6e9edc03222ae470c456209e90b1a7b,59b2b68337bda059a71fbf81e2748f6d7c4d647b,47a203748157f922c17eb297d495d0b247998df0,f4b660d893ca3471310ac65a0a0ea9a475913ea9,8f9a6c4d42814d20916c8bec0a4f916b282a982b to stable25