Skip to content
New issue

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

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

Already on GitHub? # to your account

Add fullscreen integration with editor created in the iframe #18035

Merged
merged 3 commits into from
Mar 7, 2025

Conversation

Dumluregn
Copy link
Contributor

Suggested merge commit message (convention)

Internal (fullscreen): Add fullscreen integration with editor created in the iframe.

Internal (fullscreen): Allow to configure a custom container where fullscreen should be initialized.


Additional information

For example – encountered issues, assumptions you had to make, other affected tickets, etc.

@DawidKossowski DawidKossowski requested a review from f1ames March 4, 2025 10:49
Copy link
Contributor

@f1ames f1ames left a comment

Choose a reason for hiding this comment

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

The code itself looks good 👍


However, I have some doubts about the manual test. The editor styling is missing and the iframe is quite small so it's difficult to test it or figure out what's going on there):

Screen.Recording.2025-03-05.at.10.32.47.mov

Two suggestions:

  1. Make the iframe bigger. And maybe add some margins to its body so one can see how fs editor expands?
  2. Try to add styles to the editor in the iframe.

Ad.2
As we talked F2F it's a bit tricky (I didn't investigate too much) to add styles. The quick workaround could be using styles loaded from CDN:

<head>
	<meta http-equiv="Content-Security-Policy"
		content="default-src 'none'; connect-src 'self' https://cksource.com http://*.cke-cs.com; script-src 'self' https://cksource.com; img-src * data:; style-src * 'self' 'unsafe-inline'; frame-src * data:">
</head>

// ...

<iframe id="iframe" style="display:none;" srcdoc='
	<link rel="stylesheet" type="text/css" href="https://cdn.ckeditor.com/ckeditor5/44.2.1/ckeditor5.css">
	<div class="editor-container">
	<div id="editor">

// ...

And this works fine:
image

However, it's a bit cumbersome to maintain and update so I'm not 100% sure. But still better than no styles I guess.

Base automatically changed from ck/fullscreen-bootstrap to ck/epic/1235-fullscreen-mode March 5, 2025 09:53
@Dumluregn Dumluregn requested a review from f1ames March 6, 2025 11:58
@f1ames
Copy link
Contributor

f1ames commented Mar 7, 2025

As we talked F2F it's a bit tricky (I didn't investigate too much) to add styles. The quick workaround could be using styles loaded from CDN...

This turned out to be not a very good idea (sorry 😓) since styles from CDN are missing fullscreen styling. So the fullscreen mode in iframe was looking odd.

Screen.Recording.2025-03-06.at.15.35.31.mov

However, I found another solution, just to copy CKE5 styles from the main document to iframe document - 99ab389. And now it works as expected, all new CSS rules will be used in the iframe and so it can be tested properly:

Screen.Recording.2025-03-07.at.10.04.15.mov

Copy link
Contributor

@f1ames f1ames left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Dumluregn Dumluregn merged commit 5f35c45 into ck/epic/1235-fullscreen-mode Mar 7, 2025
5 of 8 checks passed
@Dumluregn Dumluregn deleted the ck/fullscreen-iframe branch March 7, 2025 12:24
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants