Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[editor] Disable editing while PresentationMode is active #15123

Merged

Conversation

Snuffleupagus
Copy link
Collaborator

No description provided.

@Snuffleupagus
Copy link
Collaborator Author

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented Jul 1, 2022

From: Bot.io (Linux m4)


Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/0d4496ca949fa51/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jul 1, 2022

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/0d4496ca949fa51/output.txt

Total script time: 2.60 mins

Published

@Snuffleupagus Snuffleupagus force-pushed the editor-presentation-disable branch from 4be19b2 to 37b61f2 Compare July 1, 2022 14:52
@Snuffleupagus
Copy link
Collaborator Author

/botio integrationtest

@pdfjsbot
Copy link

pdfjsbot commented Jul 1, 2022

From: Bot.io (Linux m4)


Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/e97bf197642268d/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jul 1, 2022

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/ef370e3f41c23a4/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jul 1, 2022

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/e97bf197642268d/output.txt

Total script time: 4.65 mins

  • Integration Tests: Passed

@pdfjsbot
Copy link

pdfjsbot commented Jul 1, 2022

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/ef370e3f41c23a4/output.txt

Total script time: 7.80 mins

  • Integration Tests: FAILED

@marco-c
Copy link
Contributor

marco-c commented Jul 1, 2022

Might not be too bad to support it (e.g. if you're doing a presentation and you want to highlight some area of the presentation), but definitely requires a better UX.

@Snuffleupagus
Copy link
Collaborator Author

Might not be too bad to support it (e.g. if you're doing a presentation and you want to highlight some area of the presentation), but definitely requires a better UX.

Given that we purposely don't have any toolbars (or other UI) visible in PresentationMode, there'd be no way to e.g. change any of the editing-parameters without leaving PresentationMode first.
Furthermore, considering how PresentationMode is implemented there's bound to be a number of assumptions (in both the JS and CSS code) that basically depends on it being a very simple mode without e.g. any toolbars. Changing that would probably not be entirely simple, since there's going to be lots of edge-cases, which is why I think this patch makes sense here.

@marco-c
Copy link
Contributor

marco-c commented Jul 1, 2022

Oh yes, it makes perfect sense to disable it for now. Mine was just an idea for a future feature :)

@Snuffleupagus
Copy link
Collaborator Author

Note that there's also the "browser fullscreen"-mode, which can be toggled with F11 (at least on Windows), that allows the regular PDF.js viewer (or any web-page really) to occupy the entire screen.

Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@Snuffleupagus Snuffleupagus merged commit 845b7f0 into mozilla:master Jul 2, 2022
@Snuffleupagus Snuffleupagus deleted the editor-presentation-disable branch July 2, 2022 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants