Fullscreen mode feature bootstrap #17897

Dumluregn · 2025-02-10T08:46:39Z

Suggested merge commit message (convention)

Internal (fullscreen): Introduce a fullscreen mode feature.

Additional information

This PR contains a package bootstrap and a basic working feature.

scofalik · 2025-02-12T11:14:48Z

docs/getting-started/integrations/drupal-real-time-collaboration.md

@@ -41,9 +41,9 @@ You can use all the collaboration features **for {@link features/collaboration#a

 Apart from the real-time collaboration tools, the module provides various other plugins. The {@link features/format-painter format painter} feature lets users consistently style the edited text, while the {@link features/mentions mentions} feature allows you to tag other users in comments. Meanwhile, {@link features/slash-commands slash commands} let you create, insert, and format rich content on the go by typing the `/` character and choosing from many predefined actions, such as text formatting, and inserting headings, tables, or lists.

-### Full-screen mode
+### Fullscreen mode


IIRC, fullscreen for Drupal is a separate Drupal module that we created, dedicated for Drupal. After we release the official plugin, we need to make decision what do we do with the module. I don't know whether it offers anything beyond what we will offer in the official plugin, so I wouldn't be surprised if we depracte/remove it. /cc @andrzejkala

scofalik

In general I am fine with how this feature is structured and bootstrapped. One thing that I'd like to see more focus on is to try to standarize how Classic and Decoupled editors behave. I wrote more in one of comments.

scofalik · 2025-02-12T11:22:48Z

packages/ckeditor5-fullscreen/src/fullscreencommand.ts

+		} else if ( editor instanceof DecoupledEditor ) {
+			this._fullscreenHandler = new DecoupledEditorHandler( editor );
+		} else {
+			this._fullscreenHandler = new AbstractEditorHandler();


I think that both in case of supported and non-supported editor types, we should allow some customization (through a callback? or passing custom EditorHandler class?), but especially for non-supported editors.

IDK if this is planned for the first feature release or not, but this part of the code will probably change a bit.

As discussed I added a custom callbacks in #enable() and #disable() methods.

scofalik · 2025-02-12T11:28:11Z

packages/ckeditor5-fullscreen/src/fullscreenediting.ts

I am not even sure if we need this kind of plugin because fullscreen is fully UI related plugin. We can keep it this way as it is aligned to how we typically split feature's code. And we should have a separate plugin with the UI (toolbar and menu bar buttons in this case), and separate plugin holding the logic. However, all of this happens solely in the command as of now, so maybe it would be enough just to have the UI plugin or just the main plugin.

This is an interesting plugin from the perspective of how we split feature's code and how we could change it in future. We discussed it in the relation to our UI lib.

I treated the editing plugin registering the command as holding the logic so it made sense to me it's there, but if not the convention we have then probably it wouldn't make much sense. But the separation may still be useful at some point.

scofalik · 2025-02-12T11:38:19Z

packages/ckeditor5-fullscreen/src/handlers/abstracteditor.ts

+	 * Moves the given element to the fullscreen mode container, leaving a placeholder in its place.
+	 */
+	public moveToFullscreen( elementToMove: HTMLElement, placeholderName: string ): void {
+		const placeholderElement = createElement( document, 'div' );


I was a bit surprised that we call document just like that. There's no even any global declaration at the beginning of the file. Linter allows for that?

How we do it in other places of code? I remember we had something like import globalThis and then we used globalThis.document 🤔 .

There's no one rule how document is used throughout the codebase, in some places there's a global declaration used, sometimes globalThis, and sometimes nothing. Linter doesn't yell 🤔

scofalik · 2025-02-12T11:40:10Z

packages/ckeditor5-fullscreen/src/handlers/abstracteditor.ts

+	}
+
+	/**
+	 * Returns the moved elements to their original places.


Suggested change

* Returns the moved elements to their original places.

* Returns the moved elements to their original places.

This method also removes the container so I'd consider renaming it and changing this piece of API docs.

As requested in another comment I moved the logic to the #disable() method so this one was removed.

scofalik · 2025-02-12T11:47:22Z

packages/ckeditor5-fullscreen/src/handlers/abstracteditor.ts

+				</div>
+			`;
+
+			document.body.appendChild( this._container );


I am not sure of differences, but I expected this will be added to the BodyCollection not directly to the <body>. We put all our "remote" elements like e.g. contextual balloon in BodyCollection. For sure it makes less mess on the customer's website.

Putting the container into the default BodyCollection will cause issues with ck-reset_all again. We can again hack it and create a custom BodyCollection without reset, but I'm not convinced it's a better solution 🤔

packages/ckeditor5-fullscreen/src/handlers/abstracteditor.ts

scofalik · 2025-02-12T12:06:57Z

packages/ckeditor5-fullscreen/src/handlers/classiceditor.ts

+	public override enable(): void {
+		const editorUI = this._editor.ui;
+		const editorUIView = editorUI.view;
+
+		this.moveToFullscreen( editorUI.getEditableElement()!, 'editor' );
+		this.moveToFullscreen( editorUIView.toolbar.element!, 'toolbar' );
+
+		// In classic editor, the `dir` attribute is set on the top-level container and it affects the styling
+		// in both menu bar and toolbar (adding the side padding to the elements).
+		// Since we don't move the whole container but only parts, we need to reapply the attribute value manually.
+		// Decupled editor doesn't have this issue because there is no top-level container, so `dir` is set on each component separately.
+		this.getContainer().setAttribute( 'dir', editorUIView.element!.getAttribute( 'dir' )! );
+
+		if ( !this._editor.config.get( 'fullscreen.menuBar.isVisible' ) ) {
+			return;
+		}
+
+		if ( !editorUIView.menuBarView ) {
+			editorUIView.menuBarView = new MenuBarView( this._editor.locale );
+			editorUIView.menuBarView.render();
+			editorUI.initMenuBar( editorUIView.menuBarView );
+		}
+
+		this.moveToFullscreen( editorUIView.menuBarView.element!, 'menu-bar' );
+	}


Let's see how this will evolve and return to this after we finish implementing all functionalities.

I'd love if we don't actually need separate handlers for Classic, Decoupled and others. We should anyway move into direction where various editor "areas" (like sidebars, toolbar, menu bar, editable areas) are "registered" in a common place, in a standarized way. So features like fullscreen can use them without recognizing what is the editor type.

If there are just minor differences between editors, I'd strongly encourage changes for example in ClassicEditor so that at least classic and decoupled behave the same. It will simplify the code here and also have future benefits.

I agree with this feedback. I registered it as a task on board and if there's enough time after doing the most important integrations, we can get back to it.

packages/ckeditor5-fullscreen/src/handlers/classiceditor.ts

Dumluregn added 7 commits February 9, 2025 00:51

Bootstrap package.

cc6b281

Set layout, add API docs, improve manual test.

ed74c9b

CSS fixes for dialogs, panels and source editing + some docs.

8371a8c

Simplify the wrappers structure.

5b4296c

Validator fixes.

4672a2e

Rename from full-screen to fullscreen.

63f9e7c

Add unit tests and manu bar integration.

f275002

Dumluregn requested a review from scofalik February 10, 2025 08:46

Dumluregn mentioned this pull request Feb 10, 2025

Fullscreen mode feature bootstrap #17810

Closed

Dumluregn changed the title ~~Ck/fullscreen bootstrap~~ Fullscreen mode feature bootstrap Feb 10, 2025

scofalik reviewed Feb 12, 2025

View reviewed changes

scofalik approved these changes Feb 12, 2025

View reviewed changes

Dumluregn mentioned this pull request Feb 17, 2025

Add integration for fullscreen feature with revision history #17941

Open

Review fixes.

fbb0af5

Dumluregn mentioned this pull request Feb 21, 2025

Add fullscreen mode integration with comments and suggestions #17970

Open

Add customizable callback to #enable() and #disable() methods.

3a8af72

Dumluregn requested a review from scofalik February 21, 2025 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fullscreen mode feature bootstrap #17897

Fullscreen mode feature bootstrap #17897

Dumluregn commented Feb 10, 2025

scofalik Feb 12, 2025

scofalik left a comment

scofalik Feb 12, 2025

Dumluregn Feb 21, 2025

scofalik Feb 12, 2025

Dumluregn Feb 17, 2025

scofalik Feb 12, 2025

Dumluregn Feb 17, 2025

scofalik Feb 12, 2025

Dumluregn Feb 17, 2025

scofalik Feb 12, 2025

Dumluregn Feb 17, 2025

scofalik Feb 12, 2025

Dumluregn Feb 17, 2025

	* Returns the moved elements to their original places.
	* Returns the moved elements to their original places.

Fullscreen mode feature bootstrap #17897

Are you sure you want to change the base?

Fullscreen mode feature bootstrap #17897

Conversation

Dumluregn commented Feb 10, 2025

Suggested merge commit message (convention)

Additional information

Choose a reason for hiding this comment

scofalik 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