-
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
Improved pastefromword to retain cell borders #2219
Conversation
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's hard to say anything about this fix as there is no manual test. Please at least add some manual tests for it.
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.
Just 2 cents: make sure to add unit test for this enhancement. Currently it is not covered.
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.
Seems like borders are not correctly parsed (tested on Chrome).
It doesn't work in Safari at all:
Raw HTML data: https://gist.github.com/Comandeer/ed20b6dd576b7350f972355f799cb29a
The unit tests are missing.
Style.createStyleStack( element, filter, editor, | ||
/margin|text\-align|padding|list\-style\-type|width|height|border|white\-space|vertical\-align|background/i ); | ||
|
||
function mergeBorderStyles( borderType, borderValue ) { |
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.
I'm also thinking about moving this function into CKEDITOR.tools.style
. What a pity that CKEDITOR.tools.style.border
is a function, because it would be a great namespace for such functions.
@bender-ui: collapsed | ||
@bender-ckeditor-plugins: wysiwygarea, toolbar, pastefromword | ||
|
||
1. Using Microsoft Excel copy table with custom cell borders. Use different colors and border types. |
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 create ready to use asset.
@@ -0,0 +1,14 @@ | |||
@bender-tags: bug, 4.11.0, 1490, 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 update version tags.
@@ -0,0 +1,14 @@ | |||
@bender-tags: bug, 4.11.0, 1490, pastefromword | |||
@bender-ui: collapsed | |||
@bender-ckeditor-plugins: wysiwygarea, toolbar, 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.
Table from Excel is pastes as plain text. There are several missing plugins (e.g. table
).
delete styles[ style ]; | ||
styles[ style.toLowerCase() ] = temp; | ||
} | ||
for ( var i = 0; i < allowedBorderTypes.length; i++ ) { |
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.
You use CKEDITOR.tools.array.forEach
just right after this loop. Won't be better to use also here?
return borderObj.width + ' ' + | ||
borderObj.style + ' ' + | ||
// Replace old CSS2 `windowtext` color with `black`. | ||
borderObj.color.replace( 'windowtext', 'black' ); |
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.
windowtext
isn't actually equal to black
, it's platform dependent and could be changed by changing OS's theme or other settings. As for now every browser is supporting it, so I'm not sure if it's necessary to replace it.
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.
I'm wondering here, it's tricky to work with these values.
95% (ofc my gut feeling) people use the default values, so it would kinda make sense to convert it anyway. And it is not rare to see MS Word using this:
We could expose a dedicated method in CKEDITOR.tools.style
to replace these magic values. Then again I thought about possibility to adjust these values or even disable this replacement. So I kind of intuitively thought of CKEDITOR.config
but on the second thought I'm not a fan of bloating (already enormous) config with very specific options like that.
So eventually it could hold some sort of public mapping in CKEDITOR.tools.style
. That will allow only for global customization, but we don't need more than that.
The feature doesn't work on Safari and IE8 thus the issue: #2609 |
ec937a6
to
62c942d
Compare
Yes, I know that this PR is missing unit tests, I already wrote it in the PR description. No worries, I won't leave this code without test coverage. Although this implementation kills every test case which uses table (currently failing 132 unit tests only for chrome, some tests are written per browser so it may multiply fast) so I would really appropriate solid review before I start fixing unit tests. Small implementation change may require correcting all tests again and thus they are generated, it's a highly time-consuming task. Although I added a unit test for Chrome, so it will be easier to validate output HTML. I'm wondering if https://github.com/ckeditor/ckeditor-dev/blob/015e1cc00b114d6a503421db9c0a35b71afe525e/plugins/pastefromword/plugin.js#L26-L27 ACF filter shouldn't be moved to the I'm not very fond of |
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.
There are some differences between original table and pasted one:
Original:
Pasted one:
It's possible to fully mimic the behaviour of Excel by switching to border-collapse: separate
and moving border-bottom
from the first table cell (with [colspan=9]
) to border-top
of cells below:
Without moving borders we'd end with double borders:
Please also include changes described in #2219 (comment)
@@ -533,7 +519,7 @@ | |||
return; | |||
} | |||
|
|||
if ( value === '' ) { | |||
if ( value === '' || value == null ) { |
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 will also remove styles with 0
value, which can lead to bugs (e.g. showing borders that shouldn't be shown).
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.
I will remove this change because it's not important for this PR, although I'm curious how could it remove styles with 0
value? In JS 0 == null
and '0' == null
gives false logical value. I'm using double equal operator here to perform null == undefined
coercion.
plugins/pastefromword/plugin.js
Outdated
@@ -23,6 +23,8 @@ | |||
// Snapshots are done manually by editable.insertXXX methods. | |||
canUndo: false, | |||
async: true, | |||
allowedContent: 'table{border-collapse};' + |
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.
PFW shouldn't allow any content on its own. Content should be allowed by certain plugins. Such styles should be allowed by table
, colorbutton
etc.
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.
Yep, I already mentioned it in #2219 (comment), so we agree on this one. However, not sure how far can we go with changing ACF of the table
plugin for this feature.
core/tools.js
Outdated
* @returns {Number} return.left Left value. | ||
* @member CKEDITOR.tools.style.parse | ||
*/ | ||
shorthand: function( value, split ) { |
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.
This name is a little bit misleading, because there are several types of shorthands in CSS (e.g. one in border-radius
property).
Just to add my 2 cents here, I like the proposal added by @Comandeer. Given that we're able to make it work 100% of the time and that it will not cause any side effects it would really improve the pasting experience. However with this, this PR starts to do too many things™. We should extract this feature proposed by @Comandeer to a separate issue (as it is feature request) and fix the bug here. |
core/tools.js
Outdated
* @returns {String} return.border-left Border left style. | ||
* @member CKEDITOR.tools.style.parse | ||
*/ | ||
splitBorderStyles: function( styles, fallback ) { |
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.
Naming: it's already in style namespace, so there's no reason to call it splitBorderStyles
because it would be CKEDITOR.tools.style.parse.splitBorderStyles
(note style twice).
CKEDITOR.tools.style.parse.splitBorder
is definitely simpler.
I have pushed But we decided not to add it in order to simplify the code (we didn't need this at this point). Now as there is more than 1 method working on borders it make sense to unify it. Note that on this branch I didn't expose the type (for sure the types should be exposed, as they're useful). Also I'd work on simplifying the shorthand interaction. It didn't work well inside I think that Finally this part is garbage, and should be simplfieid: https://github.com/ckeditor/ckeditor-dev/blob/t/1490-oop/core/tools.js#L1939-L1941 This acutally is related to the API mentioned before. Anyway this is just a proposal for possible direction. As a result function returns border object that expose border information in a convenient way (just Note that with this approach there would be a little bit of API trickery - we should use BorderStyle for values returned by style.parse.border which we could change only in major release (but that's fine we'll extract it to a separate task). |
I updated the code with the new @mlewand The issue for this PR is labeled TODO Update changelog with:
I will update changelog once we are sure about the API / release version. |
At the moment |
Rebased onto latest |
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?
It's early PR which requires some changes. Among others:
pastefromword
. Not sure about IE10 and IE9.Closes #1490, closes #2924