Non-image media attachments

[julien-nc]

refs #2339

Allow non-image media upload. Some details needs to be discussed before moving forward.

[julien-nc]

@mejo- @max-nextcloud @vinicius73 @juliushaertl

This is now based on latest master which includes the material icon PR :grin:. :warning: compiled code is not included.

Before moving forward, do we agree on the following aspects?

1. We could just rename image related entries in the menubar s/image/media/ as we want to let users insert any kind of file via: the menubar, the paste event, the drop event
2. Images and Medias (any other mimetype) can be handled by the same controller/service on the server side?
   • We could rename ImageController => MediaController
   • We could rename ImageService => MediaService
3. We have to choose how to write media files in the markdown content (and then make the magic happen in Tiptap's parsing mechanism)
4. We will handle the media display in a dedicated node (pretty much like ImageView). It could look like the file list items in Files.

What's been implemented for now:

• It's possible to upload any kind of file
  • If it's an image, it's done like before
  • If not, then it's almost inserted like an image except the link target ends with #media
• ImageView calls different endpoints when the link target ends with #media
• We now have 3 endpoints to serve image/media stuff:
  • one to get an image (it existed before), it provides a preview if possible, the real image content if not
  • one to get a media file, it provides the file content
  • one to get a media file preview, it provides a file preview if possible, the mimetype icon URL if not

What's weird but not definitive :stuck_out_tongue_winking_eye: :

• There's no styling when ImageView display a media file as it will be done somewhere else later
• The attachment cleanup does not work anymore because the trailing #media breaks it. It will be adjusted when we decide how to write media files in the markdown content

TODO

• [x] Implement server side logic
• [ ] Implement the parsing magic for media attachments
• [ ] Implement a MediaFileView node to display media attachments
• [ ] Adjust the attachment cleanup methods
• [ ] Implement some PhpUnit tests
• [ ] Implement some Cypress tests

Let's go!

[vinicius73]

I like this approach, looks future-proof. What do you think about change MediaHandler/ImageHandler to AttachmentHandler @eneiluj?

[julien-nc]

Some news

• Media attachments are inserted just like images in the Markdown content
• There are 3 new API endpoints to load a media attachment file content, a preview and metadata
• The image loading fallback mechanism can now handle any number of fallback targets
• We attempt to load a media file if loading as an image has failed
• Media attachments are rendered by <ImageView>. It is still possible to delete with the trash icon. It looks like that:

Some problems

• For now we can't point to a media file with a relative path using the dav API. It just works for files in the attachment folder.
• The current implementation produces multiple requests for each media attachment. In the worst case:
  1. one checking the dav API (failing)
  2. one checking the image attachment API (failing)
  3. one checking the media attachment API (yay success)
  4. one last to get the metadata (yay again)

TODO

• What do we do when clicking on a media attachment?
  • Download?
  • Show in Files?
  • Open in the Viewer if possible?
  • All that and more?
• Rename ImageService, ImageResolver, MediaHandler etc...
• Think about having a single attachment endpoint returning the mimetype, a preview and some metadata

[jancborchardt]

@eneiluj the delete icon looks a bit off, it seems to be a community one from materialdesignicons.com (please try not to use that, only when the official site has no icon) – use this instead :) https://fonts.google.com/icons?selected=Material%20Icons%3Adelete%3A

Regarding the layout of the element itself, it would be a bit nicer if the element doesn’t go full width. The size info could go into the subline of the element, using color: var(--color-text-maxcontrast);, and the delete icon on the right of it immediately. Since the delete icon is only shown on hover/focus it is fine to have it at different positions.

Example mockup like it is in Mail:

[julien-nc]

/compile

[julien-nc]

/compile

[julien-nc]

/compile

[julien-nc]

/compile

[julien-nc]

/compile

[julien-nc]

@jancborchardt Thanks for the feedback.

• Delete icon is now the official one
• We can move it in another PR, there were no changes made to the icon position
• It now looks like that (2 files and one image):

What about alignment? It's currently centered so the icons are not vertically aligned. Would it make sense to left-align the media attachments? But then do we leave the images centered?

@vinicius73 I think I addressed all your remarks, thanks! Rebased on master, tests are finally passing.

[jancborchardt]

Would it make sense to left-align the media attachments? But then do we leave the images centered?

@eneiluj yes to both. :) Would look better to left-align non-images, and better to keep images centered.

(Btw, image filenames can be styled as color-text-maxcontrast since the image is more relevant than the text.)

[julien-nc]

@jancborchardt Here is how it looks now:

https://user-images.githubusercontent.com/11291457/186871688-e2f8866c-3206-49e9-8e6b-9fcef8af737b.mp4

• Medias are left aligned
• The delete icon is now in NcButton
• The image name color is now color-text-maxcontrast (but not in the video).

[julien-nc]

/compile

[julien-nc]

/compile

[julien-nc]

Failin Cypress test is not related (sharing).

[jancborchardt]

cc @juliushaertl we could slightly adjust the styles here and in the OpenGraph embedding PR.

@eneiluj I think the only things I would still change are:

• On files, a border: 2px solid var(--color-border) on the files for a nice framing (not extending the whole width, just nicely framing them)
• On the image, the delete button could be directly next to the image filename, currently it’s a bit floating in the air
• Said before, image filenames can be styled as color-text-maxcontrast since the image is more relevant than the text. :)

[juliusknorr]

Images are special as the filename field is editable and is used as the alt text of the image on markdown.

[julien-nc]

/compile

[julien-nc]

@jancborchardt Thanks.

• Border is set for medias, is it fine to leave the delete button outside the border? I think it looks better than if it's inside because we can keep the border fixed when the button appears.
• Image delete button: This was harder than I thought. I had to add a parent element with position: relative to be then able to use position: absolute for the button so it does not "push the caption away" when it appears.
• Caption color (it was already done but not effective in the previous video). I don't have any opinion on that one but I agree with the remark from @juliushaertl, this is a special case... As you wish.

https://user-images.githubusercontent.com/11291457/187412429-17f1d399-e4e4-4133-993f-e2a44d232dc7.mp4

[julien-nc]

/compile

[jancborchardt]

Looks great @eneiluj! The image (filename) doesn't need the border. :) Otherwise good to go design-wise!

[jancborchardt]

Images are special as the filename field is editable and is used as the alt text of the image on markdown.

Ah it's an input field? Can we only show the border on hover & focus @eneiluj?

[julien-nc]

@jancborchardt hover of the input or of the whole image wrapper (so the input border appears even when hovering the image itself)?

I think hover on the wrapper is more natural.

[julien-nc]

@vinicius73 @max-nextcloud Some news, there's a new Cypress attachment test. I did a bit of renaming there.

[jancborchardt]

@eneiluj it's the input field border, so I'd say should just appear on hover/focus of the input. :)

[julien-nc]

@jancborchardt It's kind of nice that it appears at the same time the button does. What do you think?

https://user-images.githubusercontent.com/11291457/187433368-e09f6f3a-04f3-4a46-9a45-6f680e797c47.mp4

[jancborchardt]

@eneiluj you're absolutely right! Let's do it like that :)

[julien-nc]

/compile

[julien-nc]

/compile

[julien-nc]

Cypress failure seems unrelated (share.spec). @vinicius73 @max-nextcloud Ready IMO.