Opened 6 years ago

Closed 6 years ago

#10438 closed Bug (fixed)

[FF&IE] No selection put in editable on setData

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

Description (last modified by Piotrek Koszuliński)

Attached two samples.

TC:

  1. Open framed_focus.html sample.
  2. Open console.
  3. Execute setData();.
  4. Quickly focus editor and wait.
  5. After data will be set verify:
    • Whether caret blinks.
    • Whether EDITOR#BLUR is logged when you click outside editor.
    • Whether you can type and whether after typing additional paragraph is not created (what means that selection was placed outside editable place.

Note: selection issues (3rd point) in inline editor are not a subject of this ticket. The most important part is fixing blur problem (2nd point), selection in framed editor is fixed because it was convenient.

Attachments (2)

framed_focus.html (6.8 KB) - added by Piotrek Koszuliński 6 years ago.
inline_focus.html (6.0 KB) - added by Piotrek Koszuliński 6 years ago.

Download all attachments as: .zip

Change History (17)

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

Status: newconfirmed

comment:2 Changed 6 years ago by Piotrek Koszuliński

Owner: set to Piotrek Koszuliński
Status: confirmedassigned

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

Description: modified (diff)
Summary: [FF] No selection put in editable on setData[FF&IE] No selection put in editable on setData

Changed 6 years ago by Piotrek Koszuliński

Attachment: framed_focus.html added

Changed 6 years ago by Piotrek Koszuliński

Attachment: inline_focus.html added

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

Pushed t/10438 on tests and t/10438 on dev with prototype of a patch.

Tests are fixed on FF and IE9 and they pass on Webkit on master. I haven't yet tested whether IE8, IE10 and Opera also require this fix.

Manual tests on framed_focus.html also pass.

Manual tests on inline_focus.html does not fully pass - selection is not placed in editable place on FF and IE (Chrome is ok). On IE9 additionally, selection is placed at the end of editable, not at the beginning.

This makes me thing that perhaps we should promote these fixes to some editor's method called on every setData. Perhaps the original fix for Webkit also does not have to be in selection construtor. I've found that it was proposed for these issues:

<li><a href="http://dev.ckeditor.com/ticket/6192">#6192</a> : [WebKit] Inserting smileys was not working because of editor focus issues.</li>
<li><a href="http://dev.ckeditor.com/ticket/6178">#6178</a> : [WebKit] Inserting elements by code was failing if the editor didn't receive the focus first.</li>
<li><a href="http://dev.ckeditor.com/ticket/6179">#6179</a> : [WebKit] The Image dialog was not working if the editor didn't receive the focus first.</li>

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

There will be another advantage of promoting this fix higher. We will be able to make the initial caret location configurable and I have seen such requests.

Although, since we are short on time, we may work on extracting this patch later, because that would require some research and tests.

comment:6 Changed 6 years ago by Piotrek Koszuliński

[FF] Tickets to test against this: #4472, #5021, #3864, #5781.

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

I pushed few more commits to tests and dev making all browsers pass them.

  • IE7/8 - very similar fix to that for IE9.
  • Opera - it says: 'everything is fine, really! selection and active element are fine!'... yeah - right. I haven't found a way to discover that selection is broken, so I just added tests to ignored.

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

It turns out that we have to improve this patch and use for all setData calls (also initial one) earlier than I thought. As #10439 proves, we need to have consistent initial selection (before first focus, after focus&setData, after switching from source mode and in all other cases) in all browsers.

In cases when we do not need selection locking (FF and Chrome with framed editor) it will be as simple as it is now - on editor load we need to set selection in the right place (range.moveToElementEditStart( this.root, true/false )). In other cases I think that we can mock initial locked selection, as the editor was focused before. Although... brace yourself - critical core changes are coming.

Last edited 6 years ago by Piotrek Koszuliński (previous) (diff)

comment:9 Changed 6 years ago by Piotrek Koszuliński

Status: assignedreview

I pushed simplified version of t/10438.

Tests:

Note: Tests created for this ticket contains some events logging - I left this intentionally - they help while testing.

comment:10 Changed 6 years ago by Piotrek Koszuliński

Description: modified (diff)

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

I tested FF latest for #4472, #5021, #3864, #5781 which could become regressions after git:cc72d842 (Commit: "Removed unncessary hack."), but everything is fine.

comment:12 Changed 6 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

comment:13 Changed 6 years ago by Piotrek Koszuliński

I tried to debug that IE8's stack overflow alert. I reduced the TC to the absolute minimum:

<!DOCTYPE html>
<html>
<head>
	<title>CKEDITOR.htmlDataProcessor</title>
	<meta name="cktester-tags" content="editor,unit">
	<script src="../../cktester/cktester.js"></script>
	<script>

	CKTESTER.editor = {
		config: {
			enterMode: CKEDITOR.ENTER_BR
		}
	};

	CKTESTER.test( {
		'ie, we will never forget what you did to us': function() {
			this.editor.focus();
			this.editorBot.setData( '', function() {
				assert.isTrue( true );
			} );
		}
	} );

	</script>
</head>
<body></body>
</html>

Still, alert is opened.

Updates:

  • IE8 doesn't allow me to place break point in TC file or it misses it.
  • I overwrote alert function, but apparently IE calls it internally.
  • It's not possible to catch that error (cause there's no error) in dev or test code.
  • Surprisingly, IE9 also notifies about some problems:
    SCRIPT28: Stack overflow 
    document.js, line 220 char 3
    SCRIPT2343: Stack overflow in line: 220 
    
  • There really is some kind of inf recursion. It's getting hotter here!
Last edited 6 years ago by Piotrek Koszuliński (previous) (diff)

comment:14 Changed 6 years ago by Piotrek Koszuliński

OK. I pushed additional fix for IEs preventing from infinitive recursion between this new selection fix and fix for #6966. It would be cool to find a better solution, but for now I'm closing this issue, because it's blocking us.

comment:15 Changed 6 years ago by Piotrek Koszuliński

Resolution: fixed
Status: review_passedclosed

Fixed on major with git:b83e32f on dev and c84d41f on tests.

This is the commit which would be cool to enhance: git:260015a1.

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