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

Fix for pasting lists from OneNote #863

Merged
merged 6 commits into from
Sep 12, 2017
Merged

Fix for pasting lists from OneNote #863

merged 6 commits into from
Sep 12, 2017

Conversation

msamsel
Copy link
Contributor

@msamsel msamsel commented Sep 4, 2017

What is the purpose of this pull request?

Bug fix

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

  • Unit tests
  • Manual tests

What changes did you make?

  • change way how correctLevelShift process lists. To append elements at the end instead of always at the beginning.
  • Manual tests are not added, because OneNote doesn't allow on saving document, which later might be used in manual tests.

close #796

@mlewand mlewand self-requested a review September 4, 2017 18:59
Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

Couple of minor things. On top of that:

I'd rather like to have some sort original fixture in OneNote's format. But AFAICS OneNote doesn't provide the feature to save stuff locally? Can we reproduce the exact same markup using Word? Then we could store a .docx.

( function() {
'use strict';

var config = {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to extract config here, just inline it - we're doing it very often and it's well readable.

@@ -0,0 +1,38 @@
/* bender-tags: clipboard,pastefromword */
/* jshint ignore:start */
Copy link
Contributor

Choose a reason for hiding this comment

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

I see no reason to ignore this line in JSHint.

var config = {
language: 'en',
colorButton_normalizeBackground: false,
allowedContent: 'p ul li'
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than define explicitly allowedContent, just add list plugin, and that will be fine.

'OneNote'
],
tests: {
'OneNote': true
Copy link
Contributor

Choose a reason for hiding this comment

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

Test case name need to say what it's doing. In this case we're working with lists.

@msamsel
Copy link
Contributor Author

msamsel commented Sep 5, 2017

I've analysed OneNote and Word. I wasn't able to reproduce similar situation in case of Word.
Word keeps list as <span> elements where OneNote use regular <ul> and <li> elements.

Word2016 & Word2013:

<!--StartFragment-->

<p class=MsoNormal style='margin-top:0in;margin-right:0in;margin-bottom:0in;
margin-left:47.9pt;margin-bottom:.0001pt;text-indent:-.25in;line-height:normal;
mso-list:l0 level2 lfo1;tab-stops:list 1.0in;vertical-align:middle'><![if !supportLists]><span
style='font-size:10.0pt;mso-bidi-font-size:11.0pt;font-family:Symbol;
mso-fareast-font-family:Symbol;mso-bidi-font-family:Symbol'><span
style='mso-list:Ignore'>·<span style='font:7.0pt "Times New Roman"'>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;
</span></span></span><![endif]><span style='mso-ascii-font-family:Calibri;
mso-fareast-font-family:"Times New Roman";mso-hansi-font-family:Calibri;
mso-bidi-font-family:Calibri'>1111<o:p></o:p></span></p>

<p class=MsoNormal style='margin-top:0in;margin-right:0in;margin-bottom:0in;
margin-left:47.9pt;margin-bottom:.0001pt;text-indent:-.25in;line-height:normal;
mso-list:l0 level2 lfo1;tab-stops:list 1.0in;vertical-align:middle'><![if !supportLists]><span
style='font-size:10.0pt;mso-bidi-font-size:11.0pt;font-family:Symbol;
mso-fareast-font-family:Symbol;mso-bidi-font-family:Symbol'><span
style='mso-list:Ignore'>·<span style='font:7.0pt "Times New Roman"'>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;
</span></span></span><![endif]><span style='mso-ascii-font-family:Calibri;
mso-fareast-font-family:"Times New Roman";mso-hansi-font-family:Calibri;
mso-bidi-font-family:Calibri'>2222<o:p></o:p></span></p>

<p class=MsoNormal style='margin-top:0in;margin-right:0in;margin-bottom:0in;
margin-left:47.9pt;margin-bottom:.0001pt;text-indent:-.25in;line-height:normal;
mso-list:l0 level2 lfo1;tab-stops:list 1.0in;vertical-align:middle'><![if !supportLists]><span
style='font-size:10.0pt;mso-bidi-font-size:11.0pt;font-family:Symbol;
mso-fareast-font-family:Symbol;mso-bidi-font-family:Symbol'><span
style='mso-list:Ignore'>·<span style='font:7.0pt "Times New Roman"'>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;
</span></span></span><![endif]><span style='mso-ascii-font-family:Calibri;
mso-fareast-font-family:"Times New Roman";mso-hansi-font-family:Calibri;
mso-bidi-font-family:Calibri'>3333<o:p></o:p></span></p>

<p class=MsoNormal><o:p>&nbsp;</o:p></p>

<!--EndFragment-->

OneNote:

<!--StartFragment-->

<div style='direction:ltr;border-width:100%'>

<div style='direction:ltr;margin-top:0in;margin-left:0in;width:1.1534in'>

<div style='direction:ltr;margin-top:0in;margin-left:0in;width:1.1534in'>

<ul style='margin-left:.2902in;direction:ltr;unicode-bidi:embed;margin-top:
 0in;margin-bottom:0in'>
 <ul type=disc style='margin-left:.375in;direction:ltr;unicode-bidi:embed;
  margin-top:0in;margin-bottom:0in'>
  <li style='margin-top:0;margin-bottom:0;vertical-align:middle'><span
      style='font-family:Calibri;font-size:11.0pt'>1111</span></li>
  <li style='margin-top:0;margin-bottom:0;vertical-align:middle'><span
      style='font-family:Calibri;font-size:11.0pt'>2222</span></li>
  <li style='margin-top:0;margin-bottom:0;vertical-align:middle'><span
      style='font-family:Calibri;font-size:11.0pt'>3333</span></li>
 </ul>
</ul>

</div>

</div>

</div>

<!--EndFragment-->

@mlewand mlewand self-requested a review September 12, 2017 12:13
Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

I see, ok we can do without the original file.

Subdirs in fixtures are lowercased (like word2013, word2016 etc) thus it should be onenote. I have fixed that.

However there's a missing manual test 🙀

@msamsel msamsel changed the base branch from master to major September 12, 2017 12:38
Mateusz Samsel and others added 5 commits September 12, 2017 14:39
- remove jshint ignore comment
- change config for tests
- add list plugin instead of using allowedContent
- rename test suite
@mlewand mlewand changed the title [#796] Fix for pasting lists from OneNote Fix for pasting lists from OneNote Sep 12, 2017
@mlewand mlewand self-requested a review September 12, 2017 14:57
Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

During tests I saw that IE8 adds some extra artifacts to the list. I assume it's a leftover generated by it, as I see same thing in 4.7.2 release.

image

@mlewand mlewand merged commit 503c672 into major Sep 12, 2017
@mlewand mlewand deleted the t/796 branch September 12, 2017 15:02
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.

List is pasted from OneNote in a reversed order
2 participants