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

Full Screen Mode #76

Closed
quicksketch opened this issue Oct 27, 2023 · 24 comments · Fixed by #80
Closed

Full Screen Mode #76

quicksketch opened this issue Oct 27, 2023 · 24 comments · Fixed by #80

Comments

@quicksketch
Copy link
Member

So far, CKEditor 5 doesn't support Full-screen editing.

More info:

Originally posted by @olafgrabienski in #36 (comment)

@indigoxela
Copy link
Member

Oops, another evidence, that Open Source rocks...

https://github.com/pikulinpw/ckeditor5-fullscreen

@indigoxela
Copy link
Member

indigoxela commented Oct 29, 2023

The third party plugin's for sure helpful, but we can't use it with our build, it seems (at least, not without complications).

However, looking at the code - we can accomplish something similar in Backdrop. A PR is available for testing and review.
@olafgrabienski are you willing to test?

@olafgrabienski
Copy link
Member

Thanks for the PR, I'm happy to test it, hopefully soon.

Is it okay to apply the PR to the last release, or should I use the latest 'dev' as basis for the PR?

@indigoxela
Copy link
Member

indigoxela commented Oct 30, 2023

Is it okay to apply the PR to the last release, or should I use the latest 'dev'...

@olafgrabienski As the latest alpha release is already quite behind dev and there are several PRs in the queue, I'd currently actually recommend to clone the repo (if you have git available) and use git apply for patches. With a cloned repo it's probably easier to keep up with changes. But use whatever works best for you. 👍

@pikulinpw
Copy link

pikulinpw commented Oct 30, 2023

The third party plugin's for sure helpful, but we can't use it with our build, it seems (at least, not without complications).

However, looking at the code - we can accomplish something similar in Backdrop. A PR is available for testing and review. @olafgrabienski are you willing to test?

Hello, if there are any problems/questions on the plugin let me know. I will finalize it if necessary. At the moment a few add-ons should be released in the next few days. One update has already been done and published.

Also I will be glad to develop a plugin that will solve any other problem of yours

@indigoxela
Copy link
Member

indigoxela commented Oct 30, 2023

Hey @pikulinpw, many thanks for chiming in! 👋

The complications on our side are pretty Backdrop-specific (based on dll-build, currently not even fully implemented...). Considering these could lead to complications on your side, that might not be necessary.

However, it turned out, the JS/PHP part is rather trivial, the difficult part is CSS 😜, especially figuring out the right z-index. That's something that might also be interesting for you. Currently you're setting z-index: 99999;, which means, the cke modal components (like the image toolbar) would slip below the fullscreen display, hence are inaccessible. Additionally (at least with our CMS), there might be other modal components, that get covered, too. Eventually making the z-index a configurable option could solve that for you. For the cke components this var is helpful: --ck-z-modal (as in my PR)

Again, many thanks for your useful implementation example. 🙏 It helped me a lot.

@pikulinpw
Copy link

Hey @pikulinpw, many thanks for chiming in! 👋

The complications on our side are pretty Backdrop-specific (based on dll-build, currently not even fully implemented...). Considering these could lead to complications on your side, that might not be necessary.

However, it turned out, the JS/PHP part is rather trivial, the difficult part is CSS 😜, especially figuring out the right z-index. That's something that might also be interesting for you. Currently you're setting z-index: 99999;, which means, the cke modal components (like the image toolbar) would slip below the fullscreen display, hence are inaccessible. Additionally (at least with our CMS), there might be other modal components, that get covered, too. Eventually making the z-index a configurable option could solve that for you. For the cke components this var is helpful: --ck-z-modal (as in my PR)

Again, many thanks for your useful implementation example. 🙏 It helped me a lot.

My plugin is organized differently from yours. Your plugin opens full screen with just css, my plugin first of all tries to open full screen via FullScreen API of the browser and only if it is not possible it does it via css.
Regarding z-index, I will finalize and post an update of the plugin today so that you can customize it. I will also add a setting to allow you to open to full screen only with css.

I am also releasing two more plugins the other day, one of them may solve another issue you have, namely "Apply front-end theme CSS to CKEditor instances", if you don't mind when I release the plugin I can announce it in your issue. Also if you need any other plugin for CKEditor5 let me know, maybe I can help by developing it.

@indigoxela
Copy link
Member

my plugin first of all tries to open full screen via FullScreen API of the browser

Yes, saw that in the code, but as I wasn't able to look at it with our editor implementation, I couldn't check, how that behaves re modals. 👍

I am also releasing two more plugins the other day, one of them may solve another issue you have, namely "Apply front-end theme CSS to CKEditor instances"

That sounds great! Sure, ping us any time, you're welcome. 😄

if you need any other plugin for CKEditor5 let me know...

Wow, many thanks! (Be careful with what you offer, we might come back to you 😁)

@pikulinpw
Copy link

pikulinpw commented Oct 30, 2023

my plugin first of all tries to open full screen via FullScreen API of the browser

Yes, saw that in the code, but as I wasn't able to look at it with our editor implementation, I couldn't check, how that behaves re modals. 👍

I am also releasing two more plugins the other day, one of them may solve another issue you have, namely "Apply front-end theme CSS to CKEditor instances"

That sounds great! Sure, ping us any time, you're welcome. 😄

if you need any other plugin for CKEditor5 let me know...

Wow, many thanks! (Be careful with what you offer, we might come back to you 😁)

I'm waiting for you to come back to me 😁
Happy to help your project, please contact and tag me if you need any more help with CKEditor5

Subscribe so you don't miss plugin updates)

@quicksketch
Copy link
Member Author

I am a little hesitant to add a feature with custom code that seems likely to be added to CKEditor core in the future (per ckeditor/ckeditor5#1235), but overall it seems like this is a relatively simple plugin. @indigoxela and @olafgrabienski what are your thoughts on the possible options?

@pikulinpw Your plugin looks great. Would it be possible for you to compile a DLL version of the plugin either in the release or as part of the npm package? CKEditor core plugins all include DLL builds which make it easier for us to integrate plugins without recompiling the entire editor.

@olafgrabienski
Copy link
Member

what are your thoughts on the possible options?

(custom plugin / 3rd party plugin / wait for native support)

No strong opinion, but native support seems to be preferable. Hard to guess how likely it will be added to CKEditor, and when. Maybe start with a plugin, it it's simple enough, and replace it later?

@olafgrabienski
Copy link
Member

clone the repo (if you have git available) and use git apply for patches

@indigoxela Thanks for the hint. I've applied the pull request to my test site and added the Maximize button to the Editor toolbar. Works like a charm!

One note: When CKEditor is maximized, I see the following warning In the Firefox Dev tools console: "This site appears to use a scroll-linked positioning effect. This may not work well with asynchronous panning"

@quicksketch
Copy link
Member Author

Maybe start with a plugin, it it's simple enough, and replace it later?

I'm leaning this direction too.

@indigoxela
Copy link
Member

I am a little hesitant to add a feature with custom code that seems likely to be added to CKEditor core in the future

Yes, maybe... but it doesn't appear to happen any time soon.

Another option is to make this a contrib module. But as the maximize feature shipped with CKE4, keeping it in core would provide better backwards compatibility, I guess.
In Drupal there are two options: use the paid premium features module, or the Open Source fullscreen module.

@indigoxela
Copy link
Member

indigoxela commented Oct 31, 2023

@olafgrabienski many thanks for testing!

Re "This site appears to use a scroll-linked positioning effect..." – I think, this is caused by the Backdrop dialogs, not by the maximize plugin. This needs verification, though.

Update: it seems to be caused by the cke sticky toolbar, so there's nothing we could do about it. 🤷

indigoxela added a commit to indigoxela/ckeditor5 that referenced this issue Oct 31, 2023
@olafgrabienski
Copy link
Member

it seems to be caused by the cke sticky toolbar

Good to know! (Now I see, the warning is also present in 'normal' mode, when the sticky toolbar is active.)

@klonos
Copy link
Member

klonos commented Nov 24, 2023

I don't feel that this issue should be a priority to get the module into Backdrop core. Could be worked on once that's done, in the core issue queue.

@indigoxela the PR branch now has conflicts by the way.

@indigoxela
Copy link
Member

@klonos many thanks for having a look. 🙏

This issue here for sure isn't a blocker for getting CKE5 into core (it's a feature request). Very likely it won't even be a part of the main CKE module, but will rather stay in contrib as its own module, or we wait until the library ships with an alternative solution. Yes, the PRs conflicting.

@olafgrabienski
Copy link
Member

Very likely it won't even be a part of the main CKE module, but will rather stay in contrib (...)

@indigoxela Why that? Some time ago you mentioned backwards compatibility:

But as the maximize feature shipped with CKE4, keeping it in core would provide better backwards compatibility, I guess.

In my opinion, it's still a valid argument to provide the feature in core.

@indigoxela
Copy link
Member

indigoxela commented Nov 24, 2023

In my opinion, it's still a valid argument to provide the feature in core.

As far as I can tell, nothing's decided, yet. 😉 If there are enough requests to ship this with core, then we can put it into core. If not - contrib. The proof-of-concept exists and works, so it will be available.

@indigoxela
Copy link
Member

FTR: rebased and conflict resolved.

@quicksketch
Copy link
Member Author

The PR looks good. I was going to suggest that the CSS (and maybe the icon) should be moved into the plugins directory, but on further thought it makes sense as-is, where the icon is just collected with all other icons, and the CSS file has a unique name so that it can be overridden by a theme.

@indigoxela
Copy link
Member

indigoxela commented Nov 25, 2023

Actually, the icon under /icons is only used in the toolbar builder (on /admin/config/content/formats/FORMAT).
The (same) svg for the editor's toolbar button is defined inline in class Maximize (didn't find another way to register custom button icons in CKE5).
The css file can get aggregated. It will always be attached (as implemented in #5), but shouldn't cause any problems, as all selectors are specific to maximize.

@quicksketch
Copy link
Member Author

From the looks of ckeditor/ckeditor5#1235, I don't think it's likely that CKEditor is going to provide a built-in solution in the near future:

  1. There are now helpful alternatives like https://github.com/pikulinpw/ckeditor5-fullscreen (thanks again @pikulinpw).
  2. CKSource is pushing their Full Screen plugin as part of their premium bundle.

Considering other plugins require recompilation to bundle them, I think moving forward with our own plugin is a good solution that should have a useful lifetime, and provide one more button that can be mapped 1-to-1 from CKEditor 4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants