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

Prevent editor creation on detached element #4463

Merged
merged 103 commits into from Apr 23, 2021
Merged

Prevent editor creation on detached element #4463

merged 103 commits into from Apr 23, 2021

Conversation

sculpt0r
Copy link
Contributor

@sculpt0r sculpt0r commented Dec 31, 2020

What is the purpose of this pull request?

New feature

Does your PR contain necessary tests?

All patches that change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

  • Unit tests
  • Manual tests

Did you follow the CKEditor 4 code style guide?

Your code should follow the guidelines from the CKEditor 4 code style guide which helps keep the entire codebase consistent.

  • PR is consistent with the code style guide

What is the proposed changelog entry for this pull request?

* [#4461](https://github.com/ckeditor/ckeditor4/issues/4461): Delay editor creation if target element is detached.

What changes did you make?

Give an overview…

  • If config.delayDetached is true and target element isDetached: immediately return null and start checking in 1000ms periods.
  • If target element is attached -> create an editor instance and stop checking.
  • Editor will be available e.g. inside onInstanceReady via this
CKEDITOR.replace( textArea, {
	delayDetached: true,
	on: {
		instanceReady: function(evt) {
			this.setData("Editor created");
		}
	}
} )

Which issues does your PR resolve?

Closes #4461 .

@sculpt0r sculpt0r marked this pull request as ready for review January 3, 2021 21:59
@f1ames f1ames self-requested a review January 4, 2021 15:10
f1ames
f1ames previously requested changes Jan 4, 2021
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.

CI is red for this PR could you check what's going on?

Could you also provide unit tests which covers this case?

core/creators/themedui.js Outdated Show resolved Hide resolved
core/creators/themedui.js Outdated Show resolved Hide resolved
core/creators/themedui.js Outdated Show resolved Hide resolved
tests/core/creators/manual/detached.md Outdated Show resolved Hide resolved
tests/core/creators/manual/detached.html Outdated Show resolved Hide resolved
@sculpt0r sculpt0r self-assigned this Jan 5, 2021
@sculpt0r
Copy link
Contributor Author

sculpt0r commented Jan 5, 2021

CI is red for this PR could you check what's going on?

Could you also provide unit tests which covers this case?

Initially, it was red because I didn't check if the config exists before use it. I fixed this.

Now it's only red for `BROWSER=Firefox MOZ_HEADLESS=1`. One of them shows fail with pdf export and second tests/plugins/pastefromlibreoffice/generated/imagesextraction

I restart one of them to check if there is any randomness.

@f1ames
Copy link
Contributor

f1ames commented Jan 5, 2021

fail with pdf export

This is a known issue we have already reported so it's not caused by this PR.

and tests/plugins/pastefromlibreoffice/generated/imagesextraction

And this is not so if it still fails after restart it means this PR may be the cause here.

@sculpt0r
Copy link
Contributor Author

sculpt0r commented Jan 5, 2021

fail with pdf export

This is a known issue we have already reported so it's not caused by this PR.

and tests/plugins/pastefromlibreoffice/generated/imagesextraction

And this is not so if it still fails after restart it means this PR may be the cause here.

The second run: shows only fail with pdf on single BROWSER=Firefox MOZ_HEADLESS=1.

I will make all correct and I'll back to verify CI

@sculpt0r
Copy link
Contributor Author

sculpt0r commented Jan 7, 2021

Also, I add new CKEditor error code, and made a ckeditor4-docs PR: ckeditor/ckeditor4-docs#344

@sculpt0r
Copy link
Contributor Author

sculpt0r commented Jan 7, 2021

CI is green (except pdf exporter). Ready for review.

@github-actions
Copy link

It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days.

@github-actions github-actions bot added the stale The issue / PR will be closed within the next 7 days of inactivity. label Jan 15, 2021
@github-actions
Copy link

There was no activity regarding this pull request in the last 14 days. We're closing it for now. Still, feel free to provide us with requested feedback so that we can reopen it.

@github-actions github-actions bot added the pr:frozen ❄ The pull request on which work was suspended due to various reasons. label Jan 22, 2021
@github-actions github-actions bot closed this Jan 22, 2021
@sculpt0r
Copy link
Contributor Author

Waiting for next planing, after release

@sculpt0r sculpt0r reopened this Jan 22, 2021
@github-actions github-actions bot removed the stale The issue / PR will be closed within the next 7 days of inactivity. label Jan 23, 2021
@sculpt0r sculpt0r removed the pr:frozen ❄ The pull request on which work was suspended due to various reasons. label Jan 27, 2021
@sculpt0r
Copy link
Contributor Author

Rebase onto newest major.

@github-actions
Copy link

github-actions bot commented Feb 4, 2021

It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days.

@github-actions github-actions bot added the stale The issue / PR will be closed within the next 7 days of inactivity. label Feb 4, 2021
@github-actions
Copy link

There was no activity regarding this pull request in the last 14 days. We're closing it for now. Still, feel free to provide us with requested feedback so that we can reopen it.

@github-actions github-actions bot added the pr:frozen ❄ The pull request on which work was suspended due to various reasons. label Feb 11, 2021
@github-actions github-actions bot closed this Feb 11, 2021
@f1ames
Copy link
Contributor

f1ames commented Apr 23, 2021

Rebased onto latest major.

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 🎉 Good job @sculpt0r and @Dumluregn 👏 👏 👏


  • added two manual tests: detachedcustomcallbackmobile and detachedcustominterval. Both are ignored in the non-mobile environment - since the test cases are the same as detachedcustomcallback and detachedcustominterval. However, I added message field in those tests. Because on mobile (at least mine) some operations are so slow, that in the first place you can think that nothing happened. So this field informs about the current stage.
  • For detachedcustomcallback and detachedcustominterval I added mobile ignore.

👍

Please take a look at #4463 (comment). Should I create follow up?

Yes, please 👍


Rechecked tests on IE9 and IE10 - everything's green ✔️

@f1ames
Copy link
Contributor

f1ames commented Apr 23, 2021

Just waiting for the CI to finish after rebase and I'll be merging this PR 👍

@sculpt0r
Copy link
Contributor Author

Yes, please +1

Created: #4641

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

Successfully merging this pull request may close these issues.

Prevent editor creation on detached element
3 participants