Opened 15 years ago

Closed 14 years ago

Last modified 14 years ago

#5293 closed Bug (fixed)

Unwanted linebreak on Firefox

Reported by: Artem Owned by: Frederico Caldeira Knabben
Priority: Normal Milestone: CKEditor 3.4
Component: Core : Output Data Version: 3.1.1
Keywords: Confirmed Firefox Opera Cc: tarek@…, b.basto@…, alex@…

Description

If I use empty textarea replaced with CKEditor and post form I've '<br />' in Firefox 3.6. In IE8 and Chrome 4 I have empty string.

Attachments (7)

ckedi2010-04-27_132117.jpg (20.2 KB) - added by Artem 15 years ago.
This is th result of posting empty CKeditor value
5293.patch (754 bytes) - added by Garry Yao 15 years ago.
5293_2.patch (814 bytes) - added by Frederico Caldeira Knabben 14 years ago.
5293_3.patch (781 bytes) - added by Frederico Caldeira Knabben 14 years ago.
5293_Comment34_TC_JQuery.html (1.0 KB) - added by Frederico Caldeira Knabben 14 years ago.
Test case for comment:34 using the JQuery adapter
5293_Comment34_TC_JS.html (771 bytes) - added by Frederico Caldeira Knabben 14 years ago.
Test case for comment:34 using JavaScript and DOM
ffmpeg-autumn-logo_muxson.2.txt (61 bytes) - added by Slavon 9 years ago.
http://smartmiltoys.com/kidkraft-dollhouse/

Download all attachments as: .zip

Change History (42)

comment:1 Changed 15 years ago by wester

Does this occur when Firebug is enabled or disabled?

See also http://dev.fckeditor.net/ticket/5253.

comment:2 in reply to:  1 Changed 15 years ago by Artem

Replying to wester:

Does this occur when Firebug is enabled or disabled?

See also http://dev.fckeditor.net/ticket/5253.

This problem occurs even in safe mode. Ticket 5253 is not the same bug.

comment:3 in reply to:  1 Changed 15 years ago by Artem

I checked previous version and confirm that this problem appears also in 3.1.1, but in 3.1 everything is fine.

comment:4 Changed 15 years ago by Artem

To emulate this error you can use your samples (replacebyclass.html or replacebycode.html) with empty textarea by default. Open the page in the Firefox, scroll down to button and submit the form. You will see "<br />" as value of Posted Data.

Changed 15 years ago by Artem

Attachment: ckedi2010-04-27_132117.jpg added

This is th result of posting empty CKeditor value

comment:5 Changed 15 years ago by tarek

Cc: tarek@… added

This is happening in Firefox (3.6.3) on a mac platform as well without Firebug running:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3 GTB7.0

CKeditor 3.2.1 is adding a

<br />

upon initialization. The tag is transmitted upon POST, when it should leave the input blank. It's making it especially difficult to validate database fields by checking to see if they are empty or not.

comment:6 Changed 15 years ago by tarek

I've also confirmed that this is happening on the latest nightly build (as of 5/18/2010) on Mac FF 3.6.3 and PC FF 3.6.3. It does not occur on other browsers that I've tested. A patch would be most appreciated.

comment:7 Changed 15 years ago by Frederico Caldeira Knabben

Component: GeneralCore : Output Data
Keywords: Confirmed added
Milestone: CKEditor 3.4
Version: 3.23.1.1

I confirm this has been introduced with version 3.1.1.

comment:8 in reply to:  7 Changed 15 years ago by Bruno Basto

Cc: b.basto@… added

Replying to fredck:

I confirm this has been introduced with version 3.1.1.

is there a patch already?

comment:9 Changed 15 years ago by Bruno Basto

hey guys, I guess I figured out whats the problem.

at _source\plugins\wysiwygarea\plugin.js

near line 397:

Restore the original document status by placing the cursor before a bogus br created (#5021). domDocument.createElement( 'br', { attributes: { '_moz_editor_bogus_node' : 'TRUE', '_moz_dirty' : "" } } ).replace( domDocument.getBody().getFirst() );

I'm not sure if we really need this:

domDocument.createElement( 'br', { attributes: { '_moz_editor_bogus_node' : 'TRUE', '_moz_dirty' : "" } } ).replace( domDocument.getBody().getFirst() );

does anyone know why we need this 'br'?

thanks!

comment:10 Changed 15 years ago by tarek

I tried removing the lines above and at least when using the replace by class method of including a CKeditor, the problem remains. Might a change be necessary somewhere else when using the replace by class method?

comment:11 Changed 15 years ago by Simon Bartlett

CKEditor/trunk/_source/core/dom/element.js, line 246 (version 3.1):

				this.append(
					CKEDITOR.env.opera ?
						this.getDocument().createText('') :
						this.getDocument().createElement( 'br' ) );

was changed (in version 3.1.1) to:

				var bogus = CKEDITOR.env.opera ?
						this.getDocument().createText('') :
						this.getDocument().createElement( 'br' );

				CKEDITOR.env.gecko && bogus.setAttribute( 'type', '_moz' );

				this.append( bogus );

reverting the code back fixes this issue.

comment:12 in reply to:  11 Changed 15 years ago by Simon Bartlett

Please ignore/delete my last comment. I was mistaken.

comment:13 Changed 15 years ago by Simon Bartlett

But reverting CKEditor/trunk/_source/core/htmlparser/fragment.js from 3.1.1 to 3.1 seems to fix the issue for me.

comment:14 Changed 15 years ago by Simon Bartlett

Milestone: CKEditor 3.4CKEditor 3.3

I'm sorry for the multiple comments. But to be more specific, this bug was caused by changeset: [5103]

comment:15 Changed 15 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.3CKEditor 3.4

Changed 15 years ago by Garry Yao

Attachment: 5293.patch added

comment:16 Changed 15 years ago by Garry Yao

Keywords: Review? added
Owner: set to Garry Yao
Status: newassigned

Hi gentlemen, thanks for figuring this out, while we see it not a regression related to any of the above changes, as you can safely reproduce it on raw Mozilla editor, but it's surely something to fix (especially for those who rely on SQL to empty check emptiness).

comment:17 Changed 15 years ago by Garry Yao

#4573 test could be reused here:
run OR view source.

comment:18 Changed 15 years ago by Frederico Caldeira Knabben

This one may also fix #5606.

comment:19 Changed 14 years ago by Alexander Clausen

Cc: alex@… added

It seems that sometimes, the "bogus <br />" does not actually exist, so getBody().getLast() as used in the patch is null and causes an error on submitting the form.

If I understand the code correctly, the bogus node is created in contentDomReady in plugins/wysiwygarea/plugin.js, but only *if the body has no children*. But then, it tries to access the first child with domDocument.getBody().getFirst():

if ( CKEDITOR.env.gecko && !body.childNodes.length ) // <---
{
    setTimeout( function()
    {
        restoreDirty( editor );

        // Simulating keyboard character input by dispatching a keydown of white-space text.
        var keyEventSimulate = domDocument.$.createEvent( "KeyEvents" );
        keyEventSimulate.initKeyEvent( 'keypress', true, true, domWindow.$, false,
            false, false, false, 0, 32 );
        domDocument.$.dispatchEvent( keyEventSimulate );

        // Restore the original document status by placing the cursor before a bogus br created (#5021).
        domDocument.createElement( 'br', { attributes: { '_moz_editor_bogus_node' : 'TRUE', '_moz_dirty' : "" } } )
            .replace( domDocument.getBody().getFirst() ); // <-----
        var nativeRange = new CKEDITOR.dom.range( domDocument );
        nativeRange.setStartAt( new CKEDITOR.dom.element( body ) , CKEDITOR.POSITION_AFTER_START );
        nativeRange.select();
    }, 0 );
}

what am I missing here?

comment:20 Changed 14 years ago by Frederico Caldeira Knabben

Note that a simulated keypress is sent to the browser, which creates the first node into <body>. Ad that point, it's replaced by the bogus <br>.

Those are some crazy hacks we need to live with to workaround browsers' bugs.

comment:21 Changed 14 years ago by Frederico Caldeira Knabben

For reference, this is caused by [5389].

comment:22 Changed 14 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

We should never change the DOM when retrieving the editor data.

Changed 14 years ago by Frederico Caldeira Knabben

Attachment: 5293_2.patch added

comment:23 Changed 14 years ago by Frederico Caldeira Knabben

Owner: changed from Garry Yao to Frederico Caldeira Knabben
Status: review_failedreview

The new patch is much similar to the first one, but doing that in the parsing phase, leaving the DOM untouched.

comment:24 Changed 14 years ago by Alexander Clausen

ok, so the element gets created by a fake keypress. but isn't there a race condition between dispatching the keypress event and replacing the node? because i still get the following error in the error console (the one of firefox, not the firebug error console) on load:

"node is null" in "ckeditor/_source/core/dom/node.js". opening the firebug debugger (_not_ the console...), the stack trace looks like this:

insertBefore()
called by replace()
called by (?)() <--- this is the anonymous function passed to setTimeout() I posted above (ckeditor/_source/wysiwygarea/plugin.js line 423)

btw. I'm using iceweasel 3.5.11

the original error seems to be fixed by the new patch, though.

comment:25 Changed 14 years ago by Frederico Caldeira Knabben

@sky1p, sorry, I can't understand the relation of your comment with this ticket.

comment:26 Changed 14 years ago by Sa'ar Zac Elias

Status: reviewreview_failed
  • The patch should be targeted only to FF.
  • Say the textarea content is '<br />', it should not be removed.
  • Going to source and back to WYSIWYG mode always removes the last br.

comment:27 Changed 14 years ago by Sa'ar Zac Elias

Keywords: FireFox Opera added

#6030 is a DUP and as mentioned there the bug also affects Opera.

Changed 14 years ago by Frederico Caldeira Knabben

Attachment: 5293_3.patch added

comment:28 Changed 14 years ago by Frederico Caldeira Knabben

Keywords: Firefox added; FireFox removed
Status: review_failedreview

The major problem here is that it's quite hard for us to understand whether a <br> is a real thing or a bogus. So, as a safeguard, we make the editor in a way that it'll not handle inline content directly inside <body>, expect when enterMode is BR (which is a non recommended mode).

So, right now, the most urgent thing is having the unwanted <br> disappear, which will impact on 99% of our users. Then, we can handle the "wanted" <br> case separately, if our community shows this is necessary.

The good thing about the r- is that I was able to rethink about the problem, so I'm proposing a solution that's really touching the parsing phase, so it will work with other data processors also.

comment:29 Changed 14 years ago by Sa'ar Zac Elias

Status: reviewreview_passed

comment:30 Changed 14 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: review_passedclosed

Fixed with [5750].

comment:31 Changed 14 years ago by tarek

Excellent news. Your work is very much appreciated.

comment:32 in reply to:  30 ; Changed 14 years ago by lehmanjas

Replying to fredck:

Fixed with [5750].

I added the patch described in 5750 but noticed that if the textarea never recieves focus before someone submits the form then the <br /> tag doesn't get removed and you end up having the same problem.

comment:33 in reply to:  32 ; Changed 14 years ago by Frederico Caldeira Knabben

Replying to lehmanjas:

Replying to fredck:

Fixed with [5750].

I added the patch described in 5750 but noticed that if the textarea never recieves focus before someone submits the form then the <br /> tag doesn't get removed and you end up having the same problem.

Sounds strange. The processing happens in any case, no matter the editor focus.

Can you provide us a test page for it?

comment:34 in reply to:  33 ; Changed 14 years ago by lehmanjas

Replying to fredck:

Replying to lehmanjas:

Replying to fredck:

Fixed with [5750].

I added the patch described in 5750 but noticed that if the textarea never recieves focus before someone submits the form then the <br /> tag doesn't get removed and you end up having the same problem.

Sounds strange. The processing happens in any case, no matter the editor focus.

Can you provide us a test page for it?

Here is the test link: http://coedu.rc.usf.edu/research_staff/test/testckeditor.php. It is set up to do an alert when you press the test button and it shows the contents of the box.

I am using Firefox 3.6.8 and if I just press the test link without selecting inside the box then I get an alert with the <br /> tag. Then if I select inside the box I get an empty alert. Then after that I can hit the test button and get an empty alert.

Changed 14 years ago by Frederico Caldeira Knabben

Test case for comment:34 using the JQuery adapter

Changed 14 years ago by Frederico Caldeira Knabben

Attachment: 5293_Comment34_TC_JS.html added

Test case for comment:34 using JavaScript and DOM

comment:35 in reply to:  34 Changed 14 years ago by Frederico Caldeira Knabben

Replying to lehmanjas:

I've just attached two reduced TCs for your page. It's working well for me with the current trunk, so it looks like there is something else missing in your code.

Better to wait for the 3.4 to be released, which is supposed to happen soon.

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