Opened 2 years ago

Closed 20 months ago

Last modified 20 months ago

#14856 closed Bug (fixed)

Font size and font family reset each other when modified at certain positions

Reported by: Satya Minnekanti Owned by: kkrzton
Priority: Nice to have (we want to work on it) Milestone: CKEditor 4.6.2
Component: General Version: 4.4.6
Keywords: IBM Cc: Irina

Description

Steps to reproduce

  1. Open nightly build http://nightly.ckeditor.com/16-09-12-06-07/full/samples/
  1. Clear all content
  1. Select Font Name ( Georgia) Font Size (24) & type some content.
  1. Select a new Font Name (ex: Tahoma)& type content

Expected result: Newly typed text should have Font Name (Tahoma) & Font Size 24

Actual result: Newly typed text has Font Name (Tahoma) but Font Size going back to default

Change History (19)

comment:1 Changed 2 years ago by Satya Minnekanti

It's not just Font Size that's been removed, If you apply Bold, Italic, Underline,Srikethrough, Text colour, Background colour, font size, Type some text & then change Font Name it removed all previous Styles( Bold, Italic etc ) & all also previous SPAN's for Text Colour, Background Colour & Font Size only SPAN for new Font Name remains. This is Critical issue.

Same issue happens if you apply Font Size first then all styles Font Name text colour, bold etc and change Font Size all previous styles (Bod, Italic etc, font name text colour etc) gone, only new font size remains.

Last edited 2 years ago by Satya Minnekanti (previous) (diff)

comment:2 Changed 2 years ago by Satya Minnekanti

Summary: Font Size going back to default when Font Name changedAll Formatting removed when user tries to change value of first styling (Font Size/Font Name) that was applied

comment:3 Changed 2 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.0

This does look like a regression, we need to check when it was introduced.

comment:4 Changed 2 years ago by Jakub Ś

Status: newconfirmed
Version: 4.4.6

Problem can be reproduced from CKEditor 4.4.6

comment:5 Changed 2 years ago by Jakub Ś

#14875 was marked as duplicate.

Last edited 20 months ago by kkrzton (previous) (diff)

comment:6 Changed 22 months ago by Marek Lewandowski

Milestone: CKEditor 4.6.0CKEditor 4.6.1

comment:7 Changed 21 months ago by Marek Lewandowski

Milestone: CKEditor 4.6.1CKEditor 4.6.2

comment:8 Changed 20 months ago by Marek Lewandowski

Priority: NormalNice to have (we want to work on it)

Bumping priority.

comment:9 Changed 20 months ago by kkrzton

Owner: set to kkrzton
Status: confirmedassigned

Looks like the breaking commit was f98505b.

comment:10 Changed 20 months ago by kkrzton

This issue only occurs when caret is at the end or beginning of the typed text and then the new style is applied.

The cause of this issue is line https://github.com/ckeditor/ckeditor-dev/blob/master/plugins/font/plugin.js#L142 and https://github.com/ckeditor/ckeditor-dev/blob/master/plugins/font/plugin.js#L144 (beginning/end). Basically, when re-applying font-family or font-size, the span element with matching style type (font-family/font-size) is found and caret moved outside so new span is placed after/before. If previous span contained other styling (font-size, bold, etc), those styles are not copied and lost when new span element is created.

comment:11 Changed 20 months ago by kkrzton

Status: assignedreview

Fix pushed to t/14856 branch.

As described in a previous comment the new style span outside the previous one is created so all styling elements inside the previous one are not propagated properly. I adjusted the code which now also copies nested styling elements while creating new style span.

comment:12 Changed 20 months ago by Tade0

Status: reviewreview_failed

Overall the fix looks good, but there's one thing that bothers me. Do the following:

  1. Set font to Tahoma and size to 24.
  2. Type some text.
  3. Select the text make a link from it.
  4. place the caret at the end.
  5. Set the font to Courier New.
  6. Type something.

Behaviour:

  • Pre-fix: font: Courier New, font size reset.
  • Post-fix: font reset, font size: 24.

I think this fix should cover this case as well.

comment:13 Changed 20 months ago by kkrzton

Status: review_failedreview

The case with link elements (comment:12) is quite complex and requires more general approach IMHO. I extracted the whole thing to #16768. This two cases are connected in some way but can be also treated as separate issues. At this point we should decide if #14856 could be processed separately (if there are no other issues with it) or wait for #16768 to get fixed .

comment:14 Changed 20 months ago by Marek Lewandowski

Correct, comment:12 is related to link-specific selection handling in certain browsers, explained #16768. Let's keep these things separate, so we're not worried about this TC for now.

comment:15 Changed 20 months ago by Tade0

Status: reviewreview_passed

Ok, so aside of the need to rebase with master(took care of that) and a few mistakes in the test description(also taken care of) LGTM.

comment:16 Changed 20 months ago by Marek Lewandowski

Summary: All Formatting removed when user tries to change value of first styling (Font Size/Font Name) that was appliedFont size and font family would reset each other when modified at certain positions

Checked the code LGTM, good job with test coverage!

Updated the title, as it was only font size and family affected by this issue.

comment:17 Changed 20 months ago by Marek Lewandowski

Summary: Font size and font family would reset each other when modified at certain positionsFont size and font family reset each other when modified at certain positions

comment:18 Changed 20 months ago by Marek Lewandowski

Resolution: fixed
Status: review_passedclosed

Fixed with git:eab8e4b.

comment:19 Changed 20 months ago by kkrzton

@m.lewandowski other inline styling/elements are also affected by this issue (like strong, em, etc -> comment:10) so the title should mention it too IMHO.

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy