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

Notification plugin collides and overwrites Web Notifications API #1057

Closed
CExWHamdan opened this issue Oct 16, 2017 · 6 comments · Fixed by #1077
Closed

Notification plugin collides and overwrites Web Notifications API #1057

CExWHamdan opened this issue Oct 16, 2017 · 6 comments · Fixed by #1077
Assignees
Labels
changelog:api A changelog entry should be put in the API section of the changelog. good first issue Relatively easy to fix. This is a perfect issue if you are willing to create a Pull Request. plugin:notification The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. target:major Any docs related issue that should be merged into a major branch. type:bug A bug.
Milestone

Comments

@CExWHamdan
Copy link

Are you reporting a feature request or a bug?

Bug

Provide detailed reproduction steps (if any)

  1. Open Developers Tool in your browser
  2. In the Console tab type Notification

Expected result

Web Notifications API should be returned.

Actual result

CKEditor Notification plugin is returned.

Other details

  • Browser: Chrome Version 61.0.3163.100
  • OS: macOs Sierra
  • CKEditor version: 4.6.1
  • Installed CKEditor plugins: wordcount, notification
@msamsel
Copy link
Contributor

msamsel commented Oct 17, 2017

I cannot reproduce problem on our sample: https://cdn.ckeditor.com/4.7.3/full-all/samples/.

CKEditor should not leak to global scope except CKEDITOR variable. From your description it seems that you have wrong configuration or usage of CKEditor. Alternative there might be situation that third party plugin's author made leak to global scope.

If you are able to reproduce issue without 3rd party plugins, please provide more detailed steps how to repeat such situation.

@CExWHamdan
Copy link
Author

CExWHamdan commented Oct 17, 2017

@msamsel, the issue can only be reproduced with the Notification plugin enabled which seems to be leaking to global scope. Since the Notification plugin is shipped with CKEditor, is it still considered a third party plugin that you don't support?

@msamsel
Copy link
Contributor

msamsel commented Oct 17, 2017

@CExWHamdan I gave you such information, because I'm not able to reproduce the problem.
I provide you link to our sample, where I repeat your steps and problem with notification does not occur. That's why I suggest possibility of problem with configuration or 3rd party plugins.

Please provide an example (it can be on page like codepen, jsfiddle, etc.) or reproduction steps which will allow on repeating this problem.

@CExWHamdan
Copy link
Author

Thanks @msamsel for looking into the issue.

Here's a demo where the issue can be reproduced: https://jsfiddle.net/n0m8pf3a/1/

The demo demonstrates the state of global Notification before and after instantiating a simple CKEditor with the notification plugin enabled.

@msamsel
Copy link
Contributor

msamsel commented Oct 18, 2017

Issue does not occur in actual (4.7.3) version of CKEditor.
https://codepen.io/msamsel/pen/RLEQyp?editors=1010

@msamsel msamsel closed this as completed Oct 18, 2017
@msamsel msamsel added resolution:invalid Not a valid issue (wrong request type, support requests, etc). and removed status:pending labels Oct 18, 2017
@mlewand
Copy link
Contributor

mlewand commented Oct 18, 2017

@msamsel This is a still valid issue. If you check the dev version, you're still able to reproduce it there.

Explaination

The reason why you're unable to reproduce it is that you use 4.7.x version - where notification was added as a dependency required by the clipboard plugin, which came in standard/full preset. That meant that it was inlined into a minified ckeditor.js, which is wrapped with a closure.

The jsfiddle example provided by @CExWHamdan uses 4.6.x where notification plugin was not inlined into ckeditor.js thus it's loaded at runtime from plugins/notification/plugin.js, which is not wrapped with a closure.

We must never leak any global vars other than CKEDITOR you have mentioned.

@mlewand mlewand reopened this Oct 18, 2017
@mlewand mlewand added plugin:notification The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. type:bug A bug. good first issue Relatively easy to fix. This is a perfect issue if you are willing to create a Pull Request. and removed resolution:invalid Not a valid issue (wrong request type, support requests, etc). labels Oct 18, 2017
@mlewand mlewand added this to the Backlog milestone Oct 18, 2017
@beatadelura beatadelura self-assigned this Oct 19, 2017
@mlewand mlewand added target:minor Any docs related issue that can be merged into a master or major branch. target:major Any docs related issue that should be merged into a major branch. and removed target:minor Any docs related issue that can be merged into a master or major branch. labels Oct 25, 2017
@mlewand mlewand modified the milestones: Backlog, 4.8.0 Oct 30, 2017
@mlewand mlewand added the changelog:api A changelog entry should be put in the API section of the changelog. label Oct 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:api A changelog entry should be put in the API section of the changelog. good first issue Relatively easy to fix. This is a perfect issue if you are willing to create a Pull Request. plugin:notification The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. target:major Any docs related issue that should be merged into a major branch. type:bug A bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants