-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Page break support for paste from word #2602
Conversation
35b0fc0
to
0301831
Compare
'br': function( element ) { | ||
var styles = tools.parseCssText( element.attributes.style ); | ||
|
||
if ( editor.plugins.pagebreak && styles[ 'page-break-before' ] === 'always' ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
editor.plugins.pagebreak
should be moved to the beginning of the function, as an early return. If the plugin is no loaded, there is no need to even parse element's styles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unit test is failing in IE 8.
Additionally it seems that the feature does not work in IE8.
@@ -0,0 +1,27 @@ | |||
@bender-tags: feature, 4.12.0, pastefromword |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add issue reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not working, this time in Safari.
39b6e76
to
7e9bc1a
Compare
It looks like Safari PFW contains a different tag to recognize page breaks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice to have additional fixtures, at least for IE8 and Edge.
7e9bc1a
to
43bdf1b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
What is the purpose of this pull request?
New feature
Does your PR contain necessary tests?
All patches which 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
What changes did you make?
I think that it makes the most sense to introduce this feature as optional if
pagebreak
plugin is available. Requiringpagebreak
plugin bypastefromword
will force users to usepagebreak
plugin which may not be wanted behavior (especially that it changes toolbar).Closes #2598