Full screen mode #1588

starypatyk · 2023-03-26T12:57:01Z

This PR is an attempt to implement full screen mode in the Viewer app.

Lack of full screen mode is inconvenient - esp. when viewing landscape photos on a mobile device. On Android this looks like this - only a small fraction of the screen is used to display the photo.

I have implemented a possibility to enter full screen mode - see screenshots below.

Additional item in the menu - "Full screen"

Viewer in full screen mode

Another item in the menu - "Exit full screen"

Implementation currently has the following limitations:

AFAIK does not work on iPhone - apparently the API in Safari works on Macs and iPads, but not on iPhones. I have not been able to verify this or find any workaround.
The photos are still not full screen - some area above and below the photo is taken by the viewer itself. This might be improved in this PR or separately - please advise.
There is a separate mechanism for making videos full screen. I am not sure yet how to combine these two mechanisms.

Any other questions/suggestions?

starypatyk · 2023-03-26T20:09:03Z

I have made an experimental change in commit a58f46d to show what I am thinking about.

With this change content might use 100% of available vertical space:

The implementation is a hack. Most likely it needs a design review and some changes should probably go into NcModal.

artonge

Code looks fine.
@nextcloud/designers what do you think ?

skjnldsv · 2023-03-28T06:17:57Z

src/views/Viewer.vue

+			<NcActionButton v-if="!isFullscreenMode"
+				:close-after-click="true"
+				@click="requestFullscreen">
+				<template #icon>
+					<Fullscreen :size="20" />
+				</template>
+				{{ t('viewer', 'Full screen') }}
+			</NcActionButton>
+			<NcActionButton v-else
+				:close-after-click="true"
+				@click="exitFullscreen">
+				<template #icon>
+					<FullscreenExit :size="20" />
+				</template>
+				{{ t('viewer', 'Exit full screen') }}
+			</NcActionButton>


We can simplify this with a '"toggle full screen" kind of button? :)
Instead of two v-if/v-else?

We can. 😉 However, a few libraries that I have seen (to manage photo galleries) seem to use different icons to enter/exit full screen mode. More explicit text (Full screen/Exit full screen) also seems more appropriate to my taste. If we would like to keep these, I doubt that a single button with changing icon/text would be simpler/easier to understand.

You can use two icons with v-if="isFullscreenMode" and v-else 👍

Suggested change

<NcActionButton v-if="!isFullscreenMode"

:close-after-click="true"

@click="requestFullscreen">

<template #icon>

<Fullscreen :size="20" />

</template>

{{ t('viewer', 'Full screen') }}

</NcActionButton>

<NcActionButton v-else

:close-after-click="true"

@click="exitFullscreen">

<template #icon>

<FullscreenExit :size="20" />

</template>

{{ t('viewer', 'Exit full screen') }}

</NcActionButton>

<NcActionButton :close-after-click="true"

@click="toggleFullScreen">

<template #icon>

<Fullscreen v-if="!isFullscreenMode" :size="20" />

<FullscreenExit v-else :size="20" />

</template>

{{ t('viewer', 'Toggle full screen') }}

</NcActionButton>

Thanks for your suggestion! I have made a small adjustment and committed: 9cfff2a

src/views/Viewer.vue

skjnldsv · 2023-03-28T06:21:06Z

src/views/Viewer.vue

@@ -993,6 +1071,10 @@ export default {
 		cursor: pointer;
 	}

+	:deep(.modal-header) {
+		background-color: rgba(0, 0, 0, 0.2);


why is that? :)

This gives the semi-transparent background for the header. Without this the file name and action icons would be barely visible on pictures with very light background.

I have tried to make it similar in appearance to an equivalent bar in the Android app - see screenshot below:

Unfortunately that would not work out with bright theme viewer mode (not related to the global nextcloud theming), can you try this with he Text app for example? They use Viewer with a bright theme

I have tried with the Text app and it is readable. One could even argue that having different background for the header is a good thing. 😉 Obviously this should be verified by the design team.

But - there is another problem that is much more serious.

In full screen mode content "slides" underneath the header - to use as much screen height as possible. This works good when browsing photos, but does not work at all for the text editor. Here is how it looks on Android.

The header is not visible at all. 😞

I guess we could have two modes of entering full screen:

aggressive - used probably only for images when we "slide" content under the header

normal - used for everything else, that would keep content below the header

Then the text editor would look like this in full screen:

I can implement these two modes in viewer, but I am not sure how to control which mode is used when. Could you please suggest something?

cc @nextcloud/office

I'd say full-screen is only needed for images (and possibly videos).

Text files don't need fullscreen, and also should not have their header colored differently. :) So it's fine if they stay as they are.

@skjnldsv - In 0ec3543 I have implemented the two modes of entering full screen that I had mentioned earlier. These are controlled by the MIME type of the displayed file. I have reused heuristic from canEdit.

Please have a look.

@jancborchardt - text files in full screen mode now look like this:

skjnldsv · 2023-03-28T06:21:43Z

src/views/Viewer.vue

+		.modal-container-fullscreen {
+			top: 0;
+			bottom: 0;
+			height: 100%;
+		}


I guess we would not need that if we set the modal element as full screen instead of the modal ?

Even with the modal element made full screen this will be required - the CSS rule above sets top and bottom properties to --header-height (50px AFAIK), to leave space for the header and have symmetric space at the bottom.

viewer/src/views/Viewer.vue

Line 1081 in 660d1fc

top: var(--header-height);

As we want to use 100% of available screen height, we must set top/bottom to 0.

jancborchardt

Nice, looks quite good design-wise from my side.

@starypatyk also nice with the adjustment of the header in fullscreen.
I'd even say we could omit the filename and header bar background in fullscreen, instead making sure the icons on the top right have a half-transparent background (like in Talk)?

starypatyk · 2023-04-16T21:00:00Z

@jancborchardt - thanks for your comments.

I looked at Talk for comparison and noticed that the icon in Talk is different than in my proposal.

Talk uses the same icon to enter/exit full screen mode:

In this PR there are separate icons for enter/exit and each of them is a different than the one used in Talk.

There is also a small question regarding spelling - should it be "fullscreen" vs. "full screen"?

Could you please suggest? Should I align Viewer to what is currently in Talk? Or vice versa?

On the other hand I cannot see semi-transparent background for buttons in Talk. This is how Talk looks like in full screen mode in my environment.

jancborchardt · 2023-04-17T06:55:41Z

@starypatyk to answer your questions:

"Full screen" is the correct noun writing, which also has "Exit full screen" as opposite, like you used it.
The icons you use are correct. Talk uses an older one since we are moving to Material Design icons step by step

Would be awesome if you can adjust the wording and icon in Talk! :)

Regarding the buttons, the viewer buttons are already semi-transparent, no? :)

max-nextcloud · 2023-04-17T06:57:53Z

On the other hand I cannot see semi-transparent background for buttons in Talk. This is how Talk looks like in full screen mode in my environment.

I think this might only be during a call. I'll pay attention to it during my next call and try to take a screenshot.

starypatyk · 2023-04-17T20:36:40Z

@jancborchardt

Would be awesome if you can adjust the wording and icon in Talk! :)

Sure. I should be able to do it in the next few days.

Regarding the buttons, the viewer buttons are already semi-transparent, no? :)

Currently the buttons are fully transparent (unless focused). So without some semi-transparent background will be hard to spot on very light pictures.

I have implemented your earlier suggestion - ef81778:

we could omit the filename and header bar background in fullscreen, instead making sure the icons on the top right have a half-transparent background

It looks like this now:

starypatyk · 2023-04-21T20:51:43Z

Hi @skjnldsv,

I have managed to fix most of the cypress tests to match proposed changes. The only one remaining is the visual-regression.cy.js. I can update the base images but I have a few questions.

Why is the whole cypress/snapshots folder excluded in .gitignore? There are additional rules for cypress/snapshots/actual and cypress/snapshots/diff below in .gitignore. I would expect cypress/snapshots/base not to be ignored.

There are important differences in two images.

non-dav - different icon (base vs actual):

video - focus circle (actual below):

But I also see small font differences in all images (base vs actual):

In the last test run only the non-dav image was reported, but earlier I have seen that other images failed as well.

I am running my tests on Ubuntu (apparently similar to what is done in CI) - should I update all the base images or just the ones with significant differences?

Or maybe it would make sense to increase threshold in compareSnapshot? Some tests have default threshold (zero?)

PS. This is my first experience with cypress - I was surprised how quickly one can set it up and update the tests. 😮

Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>

Co-authored-by: John Molakvoæ <skjnldsv@users.noreply.github.com> Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>

Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>

starypatyk · 2023-04-27T20:10:47Z

@skjnldsv @jancborchardt

I think that I have implemented your suggestions. I have also managed to fix failing cypress tests.

The PR is ready to merge. I do not plan more changes, unless you have additional comments.

skjnldsv · 2023-04-28T11:51:18Z

Why is the whole cypress/snapshots folder excluded in .gitignore?

To avoid mistakenly adding an update of those snapshots.
Since this is an important step, having to force add is a good safety measure :)

PS. This is my first experience with cypress - I was surprised how quickly one can set it up and update the tests.

Sooo glad to read this!! This is a very powerful tool indeed and luckily this is fairly easy to get around :)

I am running my tests on Ubuntu (apparently similar to what is done in CI) - should I update all the base images or just the ones with significant differences?

To update the snapshots, you need to build with TESTING=true, it will load the roboto font and ensure consistency with the github check.
Maybe we should add an npm pre-step for this. You can do it on this PR if you want

diff --git a/package.json b/package.json
index 174f0658..4fa6c0b9 100644
--- a/package.json
+++ b/package.json
@@ -35,8 +35,9 @@
     "cypress": "npm run cypress:e2e",
     "cypress:e2e": "cypress run --e2e",
     "cypress:gui": "cypress open --e2e",
-    "cypress:visual-regression": "TESTING=true cypress run --spec cypress/e2e/visual-regression.cy.js",
-    "cypress:update-snapshots": "TESTING=true cypress run --env type=base --spec cypress/e2e/visual-regression.cy.js --config screenshotsFolder=cypress/snapshots/base"
+    "cypress:pre-snapshots": "TESTING=true npm run dev",
+    "cypress:visual-regression": "npm run cypress:pre-snapshots && cypress run --spec cypress/e2e/visual-regression.cy.js",
+    "cypress:update-snapshots": "npm run cypress:pre-snapshots && cypress run --env type=base --spec cypress/e2e/visual-regression.cy.js --config screenshotsFolder=cypress/snapshots/base"
   },
   "dependencies": {
     "@fontsource/roboto": "^4.5.8",

src/views/Viewer.vue

Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>

starypatyk · 2023-04-28T22:34:56Z

Hi @skjnldsv,

To update the snapshots, you need to build with TESTING=true, it will load the roboto font and ensure consistency with the github check.

Thanks for clarifying - I have updated one more image to get pixel perfect verification. 😉

Maybe we should add an npm pre-step for this. You can do it on this PR if you want

I would rather not mix this with the functional changes for full screen.

skjnldsv · 2023-05-03T08:06:09Z

I would rather not mix this with the functional changes for full screen.

Would you be up to open another Pr for it afterwards?

starypatyk added enhancement New feature or request 2. developing Work in progress labels Mar 26, 2023

szaimen added this to the Nextcloud 27 milestone Mar 26, 2023

szaimen requested review from skjnldsv and artonge March 26, 2023 15:06

starypatyk marked this pull request as draft March 26, 2023 20:10

artonge approved these changes Mar 27, 2023

View reviewed changes

skjnldsv reviewed Mar 28, 2023

View reviewed changes

src/views/Viewer.vue Outdated Show resolved Hide resolved

skjnldsv reviewed Mar 28, 2023

View reviewed changes

src/views/Viewer.vue Show resolved Hide resolved

skjnldsv reviewed Mar 28, 2023

View reviewed changes

src/views/Viewer.vue Show resolved Hide resolved

skjnldsv reviewed Mar 28, 2023

View reviewed changes

jancborchardt reviewed Mar 30, 2023

View reviewed changes

starypatyk force-pushed the enh/full-screen-mode branch from ef81778 to 3fd7881 Compare April 19, 2023 19:49

starypatyk and others added 9 commits April 27, 2023 21:14

Full screen mode - initial implementation

c57e1b7

Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>

lint

c5e14e2

Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>

Experiment - 100% height for content

cddde00

Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>

Minor corrections/simplification

73d3cba

Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>

Code review - BEM syntax

f09a4cd

Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>

Code review - single button to toggle full screen

3a2f496

Co-authored-by: John Molakvoæ <skjnldsv@users.noreply.github.com> Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>

Use entire screen height only for images

3934a4a

Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>

Simplify header bar for images in full screen

0b2e652

Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>

Full screen mode - lint again

25b0a28

Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>

starypatyk added 2 commits April 27, 2023 21:14

Fix failing test - download-share.cy.js

2fa6887

Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>

Additional fixes for failing cypress tests

ab692ca

Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>

starypatyk force-pushed the enh/full-screen-mode branch from 85abbad to ab692ca Compare April 27, 2023 19:16

starypatyk added 3 commits April 27, 2023 21:27

Update base image for failing cypress test

55eb30a

Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>

Update base image for failing cypress test (again)

59e2223

Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>

Full screen mode - compile after rebase

a50631a

Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>

starypatyk marked this pull request as ready for review April 27, 2023 20:10

starypatyk added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 27, 2023

skjnldsv requested changes Apr 28, 2023

View reviewed changes

src/views/Viewer.vue Outdated Show resolved Hide resolved

starypatyk added 2 commits April 28, 2023 23:32

Cypress - update another base image (focus ring)

cddba04

Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>

Code review - simplify special handling of images

1d6a18e

Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>

skjnldsv approved these changes May 3, 2023

View reviewed changes

skjnldsv merged commit 59aa8e3 into master May 3, 2023

skjnldsv deleted the enh/full-screen-mode branch May 3, 2023 08:06

skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels May 3, 2023

starypatyk mentioned this pull request May 6, 2023

Fullscreen - use Material Design icons nextcloud/spreed#9484

Merged

6 tasks

juliusknorr mentioned this pull request Dec 28, 2023

feat: Allow comparing images, videos, audio #1862

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Full screen mode #1588

Full screen mode #1588

starypatyk commented Mar 26, 2023

starypatyk commented Mar 26, 2023 •

edited

Loading

artonge left a comment

skjnldsv Mar 28, 2023

starypatyk Mar 28, 2023

skjnldsv Mar 29, 2023

skjnldsv Mar 29, 2023

starypatyk Mar 29, 2023

skjnldsv Mar 28, 2023

starypatyk Mar 28, 2023

skjnldsv Mar 29, 2023

starypatyk Mar 29, 2023

skjnldsv Mar 31, 2023

jancborchardt Apr 3, 2023

starypatyk Apr 16, 2023

starypatyk Apr 16, 2023

skjnldsv Mar 28, 2023

starypatyk Mar 28, 2023

jancborchardt left a comment

starypatyk commented Apr 16, 2023

jancborchardt commented Apr 17, 2023

max-nextcloud commented Apr 17, 2023

starypatyk commented Apr 17, 2023

starypatyk commented Apr 21, 2023

starypatyk commented Apr 27, 2023

skjnldsv commented Apr 28, 2023

starypatyk commented Apr 28, 2023

skjnldsv commented May 3, 2023

Full screen mode #1588

Full screen mode #1588

Conversation

starypatyk commented Mar 26, 2023

starypatyk commented Mar 26, 2023 • edited Loading

artonge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jancborchardt left a comment

Choose a reason for hiding this comment

starypatyk commented Apr 16, 2023

jancborchardt commented Apr 17, 2023

max-nextcloud commented Apr 17, 2023

starypatyk commented Apr 17, 2023

starypatyk commented Apr 21, 2023

starypatyk commented Apr 27, 2023

skjnldsv commented Apr 28, 2023

starypatyk commented Apr 28, 2023

skjnldsv commented May 3, 2023

starypatyk commented Mar 26, 2023 •

edited

Loading