Opened 7 years ago

Closed 7 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:

Description

Based on: https://github.com/ckeditor/ckeditor-dev/pull/24

  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 7 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 7 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 7 years ago by Piotrek Koszuliński

Owner: set to Piotrek Koszuliński
Status: confirmedassigned

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

Status: assignedreview

Pushed t/10291 on dev.

comment:5 Changed 7 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 7 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:
      <p>foo
      &nbsp;bar</p>
      
    2. Actual:
      <p>foo
      bar</p>
      

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 7 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 7 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 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy