Opened 10 years ago

Closed 4 years ago

#3509 closed Bug (worksforme)

Styles aren't maintained at end of line

Reported by: Josh Nisly Owned by:
Priority: Normal Milestone:
Component: Core : Styles Version: SVN (CKEditor) - OLD
Keywords: Firefox HasPatch Cc: satya_minnekanti@…, irinauru@…

Description

Under certain conditions, basic styles (bold, italic, etc) aren't maintained at the end of lines.

Steps to reproduce:

  • Open the replace by code example.
  • Press the Bold button, then type text.
  • Press the end button (or click after the end of the line.)
  • Type some more text.

The newly typed text should be bold, but is not.

This also happens under the following conditions:

  • Open the replace by code example.
  • Type some text.
  • Press Home, then Shift+End to select the whole line.
  • Press the Bold button.
  • Press the end button (or click after the end of the line.)
  • Type some more text.

The problem is that Firefox is appending a <br/> at the end, but it is not being included in the <strong> element. The solution is, when applying a style, to include that BR element.

A patched is attached which fixes this problem. The problem can be reproduced in the latest nightly samples.

Attachments (6)

3509.patch (1.6 KB) - added by Josh Nisly 10 years ago.
3509_2.patch (2.6 KB) - added by Josh Nisly 10 years ago.
3509_3.patch (4.2 KB) - added by Josh Nisly 10 years ago.
3509_4.patch_ (4.2 KB) - added by Josh Nisly 10 years ago.
3509_4.patch
3509_4.patch (4.2 KB) - added by Josh Nisly 10 years ago.
3509_5.patch (2.0 KB) - added by Josh Nisly 10 years ago.

Download all attachments as: .zip

Change History (29)

Changed 10 years ago by Josh Nisly

Attachment: 3509.patch added

comment:1 Changed 10 years ago by Josh Nisly

It turns out that there are multiple issues involved. This is a more comprehensive patch to address those issues:

  • The enterkey plugin needs to be changed to appendBogus() to any newly-created inline style elements, instead of to the block element.
  • fixBlock() in range.js needs to handle situations where an inline element is the only element on the page. Without this, an <p> elements created when pressing enter are created within the inline style element, which makes it impossible to remove the style.

Just to clarify, the aim of this patch is to make sure that the ending, bogus BR is always contained in any relevant inline style elements, so that when pressing End and typing text, the newly created text maintains the previous text's style.

Changed 10 years ago by Josh Nisly

Attachment: 3509_2.patch added

comment:2 Changed 10 years ago by Josh Nisly

Oops, the last patch didn't include the changes in the first one.

Changed 10 years ago by Josh Nisly

Attachment: 3509_3.patch added

comment:3 Changed 10 years ago by Garry Yao

The thinking is reasonable and the fixing is mostly valid, but I don't understand why the changes arround L1299 of _source/core/dom/range.js, could you explain what doesn 'not necessary' in the comments mean here?

comment:4 Changed 10 years ago by Garry Yao

Keywords: Confirmed added
Milestone: CKEditor 3.1

comment:5 Changed 10 years ago by Josh Nisly

The issue is that if the document consists of a single style element (e.g. "<strong>Bold</strong>", the range will be entirely inside the strong, so when the range is split into a <p> element, it will be contained inside the <strong> ("<strong><p>Bold</p></strong>"), which makes it impossible to unbold. With the bogus appended, the range extends past the strong, which makes the strong be moved inside of the p element.

If the strong element is not the only element, the range would extend past the strong even without the bogus. That is why it is sometimes unnecessary.

comment:6 Changed 10 years ago by Josh Nisly

Actually, it turns out that the range.js change was the right idea, but not quite the right implementation. Here is a better patch.

Changed 10 years ago by Josh Nisly

Attachment: 3509_4.patch_ added

3509_4.patch

Changed 10 years ago by Josh Nisly

Attachment: 3509_4.patch added

comment:7 in reply to:  5 ; Changed 10 years ago by Garry Yao

Replying to highjinx_53:

The issue is that if the document consists of a single style element (e.g. "<strong>Bold</strong>", the range will be entirely inside the strong, so when the range is split into a <p> element, it will be contained inside the <strong> ("<strong><p>Bold</p></strong>"), which makes it impossible to unbold. With the bogus appended, the range extends past the strong, which makes the strong be moved inside of the p element.

This is not true, I assume you're question the result of range::fixBlock function here, and I'll show you how it actually works well with this TC:

  1. Open the 'replacebyclass' example with 'enterMode = br' with the following content:
    <b>bold</b>
    
  2. Put the cursor at the beginning of document, then use several 'Shift + Arrow Right' to select the whole word, the selection range after this step is as following:
    ^<b>bold</b>^
    <br />
    
  3. Execute the following lines of code globally:
      CKEDITOR.instances.editor3.getSelection().getRanges()[0].fixBlock(false, 'p')
    
  • Expected Result :
    <p>
    	<strong>bold</strong><br />
    </p>
    
  • Actual Result : Same as expected.

comment:8 in reply to:  3 ; Changed 10 years ago by Garry Yao

Keywords: Pending added

Replying to garry.yao:

The thinking is reasonable and the fixing is mostly valid...

Now I have some other concern above your fixing highjinx, actually, on output data, we would like to strip those line-height maintaining br ( aka. padding br ) on #2886, and the problem is that your fixing would have impact on the [principle http://dev.fckeditor.net/ticket/2886#comment:3] of judging padding br.
And actually, here the bug is caused by End Key, which might be more properly been fixed by keystroke handler, as you could imagine, we can intercept the key press, 'redirect' it to the correct position, say before end of <b> element here.
So, as stated above and actually we've identified several other places where keystroke redirecting is needed (e.g. #3375), I would now ask you to please hold the current fixing on this ticket and wait to see if we can have a better fix later maybe also from you. I also see your forum post regard this, so again sorry for slow responding, and thanks for your contribution on this and several other tickets, it will be greate to see more of your activities on v3.

comment:9 in reply to:  8 Changed 10 years ago by Josh Nisly

Replying to garry.yao:

Replying to garry.yao:

The thinking is reasonable and the fixing is mostly valid...

And actually, here the bug is caused by End Key, which might be more properly been fixed by keystroke handler, as you could imagine, we can intercept the key press, 'redirect' it to the correct position, say before end of <b> element here.

Actually, this isn't just an End key issue. If you click after the end of the line, the same problem happens, so it's a general selection bug. Without manipulating the DOM, I don't know of any way to avoid the click-after-line problem. I'm curious if you have any ideas on fixing that?

So, as stated above and actually we've identified several other places where keystroke redirecting is needed (e.g. #3375), I would now ask you to please hold the current fixing on this ticket and wait to see if we can have a better fix later maybe also from you.

This continues to be an issue for our users, so we need to have a solution. We'll continue to use this patch (which works well for us); perhaps once another solution is provided we can switch to it.

comment:10 in reply to:  7 Changed 10 years ago by Josh Nisly

Replying to garry.yao:

Replying to highjinx_53:

The issue is that if the document consists of a single style element (e.g. "<strong>Bold</strong>", the range will be entirely inside the strong, so when the range is split into a <p> element, it will be contained inside the <strong> ("<strong><p>Bold</p></strong>"), which makes it impossible to unbold. With the bogus appended, the range extends past the strong, which makes the strong be moved inside of the p element.

This is not true, I assume you're question the result of range::fixBlock function here, and I'll show you how it actually works well with this TC:

Ah, but you missed the point of this patch. :-) This patch moves the <br/> inside the strong, which breaks the TC.

comment:11 in reply to:  8 Changed 10 years ago by Josh Nisly

Replying to garry.yao:

Replying to garry.yao:

The thinking is reasonable and the fixing is mostly valid...

And actually, here the bug is caused by End Key, which might be more properly been fixed by keystroke handler, as you could imagine, we can intercept the key press, 'redirect' it to the correct position, say before end of <b> element here.

I've attached another patch that works as you suggest. There's one big problem, though: there's no key event that we can tie into to do what we need to do. If we use keydown, the selection hasn't moved to the end of line yet, and if we use keyup, other keystrokes can sneak in. For example, if a user types the end key and then a letter key in rapid succession, the letter will "beat" our handler, and therefore will use the default font. I don't see a way around this.

Changed 10 years ago by Josh Nisly

Attachment: 3509_5.patch added

comment:12 Changed 10 years ago by Garry Yao

Keywords: Pending removed
Milestone: CKEditor 3.1CKEditor 3.2

We'll have to leave these keystroke handler feature to 3.2.

comment:13 Changed 10 years ago by Garry Yao

Milestone: CKEditor 3.2CKEditor 3.x

Defer this bug since at least users could still use arrow key to correct the result.

comment:14 Changed 9 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.x

Milestone CKEditor 3.x deleted

comment:15 Changed 7 years ago by Jakub Ś

Can be reproduced in CKEditor 4.x (v4)

comment:16 Changed 5 years ago by Jakub Ś

Cc: satya_minnekanti@… added

#8195 was marked as duplicate.

comment:17 Changed 5 years ago by Jakub Ś

It seems we have whole buch of issues basic styles and selection problem across all browsers:

comment:18 Changed 5 years ago by Jakub Ś

#12329 was marked as duplicate.

comment:19 Changed 5 years ago by Irina

Cc: irinauru@… added

comment:20 Changed 5 years ago by Piotrek Koszuliński

Component: GeneralCore : Styles

comment:21 Changed 5 years ago by Jakub Ś

Possible duplicate was reported here: #12649

comment:22 Changed 4 years ago by Jakub Ś

#13137 was marked as duplicate.

comment:23 Changed 4 years ago by kkrzton

Resolution: worksforme
Status: confirmedclosed

Tried to reproduce on FF 43.0 and CKEditor 4.5.6 without success.

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