Opened 11 years ago

Closed 11 years ago

#10291 closed Bug (fixed)

[Webkit] Space after filling char should be secured

Reported by: Piotrek Koszuliński Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.1.1
Component: Core : Selection Version:
Keywords: Cc:


Based on:

  1. Open any sample.
  2. Write text.
  3. Press shift+enter.
  4. Press space and type text.
  5. Switch to source mode and back - space after <br> is lost.

Note: filling char is also removed in selection.js:490 and perhaps this place should be fixed too.

Change History (8)

comment:1 Changed 11 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.1.1
Status: newconfirmed

I'm adding this ticket to 4.1.1 milestone conditionally - if the patch is good (or doesn't need a lot of additional work), let's use it. If not, then we won't be able work on it for 4.1.1.

comment:2 Changed 11 years ago by Jakub Ś

Please note that Webkit browser inserts blank space at the beginning of text and not &bnsp; like at the end. Are you sure this is good idea to "fight" with browser default behaviour?

NOTE: Perform this TC once and you get desired result. Now press "New Page" command (don't refresh page) and do it one more time. Notice that this time space isn't lost. Safari shows blank space, Chrome however shows $#8203; which makes me think that working it in second case is caused by #10031 in both (only Safari doesn't show this character)

comment:3 Changed 11 years ago by Piotrek Koszuliński

Owner: set to Piotrek Koszuliński
Status: confirmedassigned

comment:4 Changed 11 years ago by Piotrek Koszuliński

Status: assignedreview

Pushed t/10291 on dev.

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

Coincidentally I was working on two tickets that touches filling char subject at the same time and it'd be safer to test t/10291 on t/10315b (#10315) which fixes filling char removing after new page / switching to source and back.

comment:6 Changed 11 years ago by Olek Nowodziński

Status: reviewreview_failed

This patch totally fixes the case and, if there's no other solution, it can be safely accepted. However there's another case that one that makes some trouble:

  1. Set data:
    <p>foo^ bar</p>
  2. Shift+Enter
  3. editor.getData()
    1. Expected:
    2. Actual:

The space before bar is lost and I can't find any solution for this. If there's no easy solution, then we can accept t/10291 as at the moment this is enough.

comment:7 Changed 11 years ago by Piotrek Koszuliński

Status: review_failedreview_passed

The reason why this patch does not cover getData() (despite the fact that replace function is used for getData() as well) is that the following space is a separate text node.

We could of course cover also this case, but that would make this code even more complex. Or even extremely complex, because we should replace this nbsp with normal space after someone typed something before it. Unfortunately I checked that Chrome will not do that for us.

	var next = fillingChar.getNext();
	// Next text node is empty. Find first which is not.
	while ( next.type == CKEDITOR.NODE_TEXT && !next.getText() )
		next = next.getNext();

	if ( next.type == CKEDITOR.NODE_TEXT && next.getText().indexOf( ' ' ) == 0 )
		next.setText( next.getText().replace( /^ /, '\xa0' ) );

Then after pressing shift+enter start typing and nbsp will remain between new text and "bar".

Therefore, we're not able to fix this right now. In fact, I think that we should take different approach - instead of playing with DOM (what's very complicated and unsafe) we should replace filling char after getting HTML (when doing snapshot or getting data). I'm pretty sure that we can safely remove all \u200b characters fixing also preceding and following spaces. Although, I'm not confident about snapshots and corresponding bookmarks. This needs to be investigated.

So for now, let's have this simple fix and we'll work on the rest later.

comment:8 Changed 11 years ago by Piotrek Koszuliński

Resolution: fixed
Status: review_passedclosed

Fixed on master with git:12de140.

I extracted idea described by me in comment 7 to #10332.

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy