viewer icon indicating copy to clipboard operation
viewer copied to clipboard

[RFC] Viewer 4.0 API

Open skjnldsv opened this issue 1 year ago โ€ข 23 comments

Let's discuss and plan the new Viewer handling. cc @juliushaertl @susnux @R0Wi @ariselseng @artonge @max-nextcloud

Challenges

With the new Files app and the new Files/Folder/Node API, we finally have a proper and official way of dealing with files It's time to re-draft the Viewer and clean the various issues we encountered over the last years

Requirements

Viewer

  • The Viewer is a component that can mount itself within a Modal to render a single or an array of Files
  • The Viewer can also be requested to be mounted within a specified div to render a single file only. In that case there would be no interaction possible (zooming, slideshow, editing, actions.... are disabled)
  • The Viewer can also be used to render two files next to each others, to compare them

Handlers

Apps can register their own views, called "Handlers" A handler provide the following

  • A unique id
  • A display name
  • A render function to process the current file
  • A open function to change the node while keeping the handler mounted
  • A toggle function to show/hide the handler (for preloading, like videos or images) :question: Is this still the right approach?
  • A condition to which the handler would work for the provided file (true/false)

skjnldsv avatar Aug 09 '24 08:08 skjnldsv

API proposal

Registering handlers

  • Like before, the Viewer app will handle registering the file actions
  • A dedicated @nextcloud/viewer library will be created
  • You will be able to register your handlers at init time, no need to wait for domcontentLoaded or other workarounds

Mime handling

  • Handlers will become mime-agnostic. Like Files actions, we'll provide an enabled prop, which you'll be able to use to define the conditions in which your component can render the file properly. You can decide to keep checking the mime only if you want, but you'll also gain more flexibility. This will also remove any fancy hacks with the mime aliases we were doing prior.

Viewer

// Init and get the viewer in Modal
export const getViewer = (viewer: any): Viewer => {}
// Create a new Viewer instance in the given element
export const createViewer = (el: HTMLElement, file: File): void => {}

type ViewerOptions = {
	loadMore: Promise<File[]>
	onPrev: () => void
	onNext: () => void
	onClose: () => void
	canLoop: boolean
}

type Viewer = {
	open: (nodes: File[], options?: ViewerOptions, handlerId?: string) => Promise<any>
	openFolder: (folder: Folder, file: File, options?: ViewerOptions) => Promise<any>
	compare: (node1: File, node2: File, handlerId?: string) => Promise<any>
}
type Handler = {
	id: string
	displayName: string
	group: string
	enabled: (nodes: Node[]): boolean
	render: (el: HTMLElement): void = {}
	open: (file: File, visible = true, static = false) => Promise<any>
	toggle: (visible: boolean): void => {}
}
  • open is used when you already have a list and want to open it. It will not fetch any additional data unless you reach then end of your list and you provided a loadMore method, in which case it will be called and the returned Files appended to the list
  • openFolder is a bit closer to the old Viewer. It allow to just provide a folder, the Viewer will fetch the content and build up the list.
    • If a file is provided, it will match the first handler enabled for that file and use it to test against all the other files in the list. The list will be filtered and will only render the elements which have an enabled handler of the same group
    • If a handler is provided, a bit like the point above, it will filter out the list, but not use the group property, only the handler enabled
  • compare is quite straightforward, most interactive features will be disabled by setting static on open. Theoritecally you would be able to open any handler. :question: I hesitated to add a canCompare boolean, but in the end, it depend on the dev. If you want to open to images side to side, you do you ! :shrug:

Removed features ?

  • enableSidebar doesn't seem to be used anywhere, are we actually still using it?

skjnldsv avatar Aug 09 '24 09:08 skjnldsv

Please feel free to ask questions, discuss things I could have missed or whatever you see.

Q&A

  • If I provided a list, what will happen if there is no handler to display one file in said list ? :a: We'll display an empty content stating we cannot render that file.
  • If a file is provided with a specified handler, but the handler doesn't work with that file ? :a: We'll also display an error in the form of an empty content. We'll move away from the Notification toast as this is not very transparent on what is happening.
  • What if I want to open a file from the FIles app, but multiple handlers declare themselves as compatible with it ? :a: If multiple handlers returned true for the given node, we can display a submenu and let the user choose which one to open with.

Relevant issues:

  • https://github.com/nextcloud/viewer/issues/2393
  • https://github.com/nextcloud/viewer/issues/240
  • https://github.com/nextcloud/viewer/issues/1450
  • https://github.com/nextcloud/viewer/issues/2099
  • https://github.com/nextcloud/viewer/issues/7
  • https://github.com/nextcloud/viewer/issues/926
  • https://github.com/nextcloud/viewer/issues/1440
  • https://github.com/nextcloud/files_pdfviewer/issues/649

skjnldsv avatar Aug 09 '24 09:08 skjnldsv

An alternative for the API that I discussed briefly with @ShGKme when talking about Vue3ยน, instead of just rendering to a HTML element passed we could make use of custom elements (web components). This way you have a framework agnostic way of registering components, it works with vanilla JS and also pretty easy with Vue (defineCustomElement).

You could have the element constructor as a getter on the handler like:

interface IHandler {
	id: string
	displayName: string
	group: string // what is this for?
	enabled: (nodes: Node[]): boolean
	// could also call it "element" or other...
	readonly component: HTMLElement // this would be a getter for the custom component constructor 
}

Benefits are: We can pass props to custom elements and we can have events from the element, this allows communication between the rendered element and the viewer, so e.g. programmatically close the viewer, communicate loading state etc. This also means no need to have render, open, toggle methods on the handler, this are just props on the element like is-visible, file, etc. And one could have events like loaded, close etc.

ยน because currently passing a Vue component is required, but this does not work with different Vues.


So this might also be an interesting approach to discuss.

susnux avatar Aug 09 '24 10:08 susnux

This also means no need to have render, open, toggle methods on the handler, this are just props on the element like is-visible, file, etc. And one could have events like loaded, close etc.

:star_struck: such a good idea! Is there any pitfall we should be aware of? Reactivity? Css with shadow-root?... something else ? Also: has anyone here already experimented with it at Nextcloud? I'd like Viewer to not be yet another experimentation place :see_no_evil:

skjnldsv avatar Aug 09 '24 10:08 skjnldsv

In addition to the render method, should we also have a destroy-like method on the handler to be able to cleanup once component is no longer active?

The web components suggestion also sounds like a great idea but I don't have any knowledge around that to properly comment on it.

juliusknorr avatar Aug 09 '24 10:08 juliusknorr

In addition to the render method, should we also have a destroy-like method on the handler to be able to cleanup once component is no longer active?

If we go for web components, you can already define your own handlers when yur component is rendered and when it's destroyed with connectedCallback and disconnectedCallback :) https://developer.mozilla.org/en-US/docs/Web/API/Web_components/Using_custom_elements#custom_element_lifecycle_callbacks

skjnldsv avatar Aug 09 '24 10:08 skjnldsv

I played with Web Components in a similar problem just yesterday (always wanted to apply this approach in Nextcloud).

We have classic settings dialog in Talk.

I needed a public API in Talk to register new settings sections. But not like a classic child component, so there is no need to share Vue library instance.

So, I tried to register custom settings section like Viewers but via Web Components.

Benefits:

  1. ๐Ÿ’… A native straight forward solution
  2. ๐Ÿš€ Vue 3 continues improving defineCustomElement in upcoming Vue 3.5
    • It makes creating Web Comonent on Vue 3 quite easy

Problems:

  1. ๐Ÿค” In Vue 2 there is no built-in helper to define custom elements
    • But in a simple case it is not complicated
    • For complex cases there are examples and 3rd part tools
  2. 2๏ธโƒฃ In Vue 2 apps vue-devtools doesn't see both Vue 2 and Vue 3 based custom elements
    • In Vue 3 apps vue-devtools can see
      • Vue 3 based custom elements without any problems
      • Vue 2 based custom elements but with glitches
  3. โ™Š A small problem - global scope and name conflict.
    • When registering a custom element, you need to provide a unique name
    • Solution - generate name

Notes:

  1. ๐Ÿซฅ Shadow DOM (typically used in web components) has no global styles (by design)
    • No Nextcloud Server global styles
    • Vue3.defineCustomElement only supports disabling shadow DOM since last week in 3.5-beta.1

Some examples of different Vue 2 and Vue 3 based custom elements:

ShGKme avatar Aug 09 '24 14:08 ShGKme

Amazing feedback @ShGKme , thanks!

For me the blockers would be:

  1. Vue3.defineCustomElement only supports disabling shadow DOM since https://github.com/vuejs/core/issues/4314
    • :a: That mean we'll have to wait for 3.5 final. The server style are too important for theming? I don't want to hack our way around this. It should work out of the box without the need to work around this for out dark/light theme to apply.
  2. Vue2 not being properly supported.
    • :a: I'm ok with this, but we need to ensure the main apps we're using can be converted to vue3 easily.
    • :b: Else maybe vue2 can hack its way around by creating a vanilla js custom element that render the vue2 component?

The vue-devtools issue is annoying, but not the worst issue I've sen. We can work without it for vue2. Viewer 3.0 would be vue3 anyway

skjnldsv avatar Aug 09 '24 15:08 skjnldsv

  • That mean we'll have to wait for 3.5 final.

We can still define custom element manually (see example). defineCustomElement just makes it much simpler.

  • Else maybe vue2 can hack its way around by creating a vanilla js custom element that render the vue2 component?

Creating a vanilla JS custom element that render the vue2 component is the only way to create Vue 2 based custom element.

Anyway, the problem persists only if the host app is Vue 2. If Viewer 3.0 is on Vue 3 - there is no problem.

ShGKme avatar Aug 09 '24 15:08 ShGKme

We can pass props to custom elements

Just wanted to note. Technically, Web Component's props are strings (like HTML attributes). But when it is used in Vue, Vue sets them as properties, not attributes, so we can path any objects to custom element props like with Vue components.

ShGKme avatar Aug 09 '24 16:08 ShGKme

disabling shadow DOM

I think even with shadow DOM you could include the server styles, like described here :) I personally really prefer the shadow DOM, the global styles are super fragile as apps often mess with them, especially as long as we do not properly use 2-way scoping (e.g. CSS modules).

Vue2 not being properly supported.

There is e.g.: https://github.com/vuejs/vue-web-component-wrapper (Also I really hope we quickly move away from Vue2, there are the first RCE :pensive: )

Just wanted to note. Technically, Web Component's props are strings (like HTML attributes). But when it is used in Vue, Vue sets them as properties, not attributes, so we can path any objects to custom element props like with Vue components.

Thats why I said "prop" not "attribute ;)

susnux avatar Aug 09 '24 18:08 susnux

Just wanted to note. Technically, Web Component's props are strings (like HTML attributes). But when it is used in Vue, Vue sets them as properties, not attributes, so we can path any objects to custom element props like with Vue components.

That is an issue. If we wanna keep a compatibility with vanilla or even react, we need to make sure the behaviour is consistent. If only Vue web components are able to handle non-string props, but the other not, that is a big issue. Or did I misunderstood?

skjnldsv avatar Aug 09 '24 20:08 skjnldsv

I think even with shadow DOM you could include the server styles, like described here :) I personally really prefer the shadow DOM, the global styles are super fragile as apps often mess with them, especially as long as we do not properly use 2-way scoping (e.g. CSS modules).

Yes, styling isolation is amazing. That's why we scope our style almost everywhere here. But the theming engine is what I'm most worried about. We have things automatically set on root and body. We CANNOT rely on Devs implementing their sharowroot adoptedStyleSheets properly to work with viewer. They should only care about the props that are given and their implementation should seamlessly integrate into it.

skjnldsv avatar Aug 09 '24 20:08 skjnldsv

That is an issue. If we wanna keep a compatibility with vanilla or even react, we need to make sure the behaviour is consistent. If only Vue web components are able to handle non-string props, but the other not, that is a big issue. Or did I misunderstood?

No it is only about the "rendering" part not about the components. Meaning if you have a element you can do:

<my-element foo="3" /> this is passing foo as an attribute, the type will always be of type "string" (or boolean when done like <my-component foo />).

But exactly as with every HTMLElement you can also pass things as properties, this is the same as doing:

myElement.bar = 3

So if you use web components in vue, Vue automatically passes props as properties (using the component instance). Which would be the same as doing this with vanilla JS:

const myElement = new MyElement()
myElement.foo = 3
document.body.appendChild(myElement)

For me this was quite helpful: https://open-wc.org/guides/knowledge/attributes-and-properties/

susnux avatar Aug 09 '24 22:08 susnux

Yes, styling isolation is amazing. That's why we scope our style almost everywhere here.

(We do one way scoping but this also is causing issues regularly)

But the theming engine is what I'm most worried about. We have things automatically set on root and body. We CANNOT rely on Devs implementing their sharowroot adoptedStyleSheets properly to work with viewer. They should only care about the props that are given and their implementation should seamlessly integrate into it.

Yes I agree, but as this would be new (while I really see a lot of use cases) we could of course provide a base class that is handling all this styling. This would make creating components very easy :)

susnux avatar Aug 09 '24 22:08 susnux

we could of course provide a base class that is handling all this styling. This would make creating components very easy

I'll have to think about this. From experience with designing APIs around here for a while now, I tend to want to avoid writing as much custom shortcuts for devs as possible. This always came back biting us after a bit.

skjnldsv avatar Aug 10 '24 13:08 skjnldsv

Looking at the onPrev and onNext callbacks in the API I wonder if it would be possible to take browser navigation into account.

Right now in files browser navigation will change the background while the viewer remains open and unchanged. For me it would be more intuitive if each viewed file had an entry in the browser history. When navigating back from the first file opened the viewer would close and display what was shown before.

If we push history entries during navigation that could maybe replace the onPrev and onNext callbacks - with listeners for the PopStateEvent. But frankly ... i don't really know what they are used for right now.

max-nextcloud avatar Sep 02 '24 12:09 max-nextcloud

I think we should be more elegant with this, you're right.

RFC addition:

  • On open, we save the current history init and inject our own history handler
  • On prev/next, we adjust the history, allowing to use history next/prev
  • On ESC we restore back to the init history state and restore the previous history handler

skjnldsv avatar Sep 03 '24 10:09 skjnldsv

I have same issue with it. Could someone let me know how to resolve it or when a new version addressing it will be released?

jthanh8144 avatar Jan 11 '25 13:01 jthanh8144

@pulsejet I saw you cloned the Viewer code into memories. I'd like us to work more closer in the future and unify the way it works for maintainability and unifying the user experience. Could you have a look at this RFC ? ๐Ÿ˜Š I'd be happy to chat about this if you got time ๐Ÿ™‡

skjnldsv avatar Apr 23 '25 09:04 skjnldsv

@skjnldsv I'll try to briefly describe the features that were missing in Viewer back then, hopefully this helps.

  1. Menu customization - this was the big one. Memories does a bunch of things like archive/stacking etc. and needs custom menu items in the viewer for this. Having this customizable would help a lot.
  2. Gestures - this one is important for media apps. E.g. zoom and swipe between files on mobile, especially.
  3. (Less important) transitions while opening / closing. But it is very nice to have - if you have a lot of duplicate files it is very hard to figure out which file you were on when you close it (without a transition).
  4. (Also critical) - The ability to lazy load in both directions. The API would also need some feedback on which is the currently open file while the user is changing files. For this one, things get a bit complicated.

I do think there are some trade offs here. A multimedia app like Memories/Photos likely requires much more functionality from the Viewer than any other app, so I'm not sure if the added complexity is worth it to support these use cases. IMO it would be more helpful to allow apps like Memories to register their own handlers easily for other apps to use, while Viewer itself only provides a simple interface for opening files (which is likely sufficient for most apps). Maybe this is already possible right now?

pulsejet avatar Apr 23 '25 19:04 pulsejet

To answer your four points:

  1. The new API is planning to use the same Files actions API, so actions visible on the main Files app (or wherever you want actually), will show on Viewer too. You should be able to register your own actions ๐Ÿ‘
  2. We already have gestures, since like, forever ๐Ÿ™ˆ Swipe to change and pinch to zoom
  3. Would be nice sure!
  4. With the new API here, you would only pass an array or Nodes, you can pass it as a reference and use the Viewer events to lazy load yourself. That should give devs all the flexibility to do whatever they want :)

A multimedia app like Memories/Photos likely requires much more functionality from the Viewer than any other app IMO it would be more helpful to allow apps like Memories to register their own handlers easily for other apps to use

Can you elaborate on those two points please? :)

skjnldsv avatar Apr 25 '25 06:04 skjnldsv

A multimedia app like Memories/Photos likely requires much more functionality from the Viewer than any other app IMO it would be more helpful to allow apps like Memories to register their own handlers easily for other apps to use

Can you elaborate on those two points please? :)

@pulsejet any news ? :)

skjnldsv avatar Sep 27 '25 13:09 skjnldsv