Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#5293 closed Bug (fixed)

Unwanted linebreak on Firefox

Reported by: aaburlaka Owned by: fredck
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 aaburlaka 7 years ago.
This is th result of posting empty CKeditor value
5293.patch (754 bytes) - added by garry.yao 7 years ago.
5293_2.patch (814 bytes) - added by fredck 6 years ago.
5293_3.patch (781 bytes) - added by fredck 6 years ago.
5293_Comment34_TC_JQuery.html (1.0 KB) - added by fredck 6 years ago.
Test case for comment:34 using the JQuery adapter
5293_Comment34_TC_JS.html (771 bytes) - added by fredck 6 years ago.
Test case for comment:34 using JavaScript and DOM
ffmpeg-autumn-logo_muxson.2.txt (61 bytes) - added by Slavon 18 months ago.
http://smartmiltoys.com/kidkraft-dollhouse/

Download all attachments as: .zip

Change History (42)

comment:1 follow-ups: Changed 7 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 7 years ago by aaburlaka

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 7 years ago by aaburlaka

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 7 years ago by aaburlaka

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 7 years ago by aaburlaka

This is th result of posting empty CKeditor value

comment:5 Changed 7 years ago by technotarek

  • 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 7 years ago by technotarek

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 follow-up: Changed 7 years ago by fredck

  • Component changed from General to Core : Output Data
  • Keywords Confirmed added
  • Milestone set to CKEditor 3.4
  • Version changed from 3.2 to 3.1.1

I confirm this has been introduced with version 3.1.1.

comment:8 in reply to: ↑ 7 Changed 7 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 7 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 7 years ago by technotarek

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 follow-up: Changed 7 years ago by SimonB

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 7 years ago by SimonB

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

comment:13 Changed 7 years ago by SimonB

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 7 years ago by SimonB

  • Milestone changed from CKEditor 3.4 to CKEditor 3.3

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

comment:15 Changed 7 years ago by fredck

  • Milestone changed from CKEditor 3.3 to CKEditor 3.4

Changed 7 years ago by garry.yao

comment:16 Changed 7 years ago by garry.yao

  • Keywords Review? added
  • Owner set to garry.yao
  • Status changed from new to assigned

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 7 years ago by garry.yao

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

comment:18 Changed 7 years ago by fredck

This one may also fix #5606.

comment:19 Changed 6 years ago by sk1p

  • 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 6 years ago by fredck

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 6 years ago by fredck

For reference, this is caused by [5389].

comment:22 Changed 6 years ago by fredck

  • Status changed from review to review_failed

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

Changed 6 years ago by fredck

comment:23 Changed 6 years ago by fredck

  • Owner changed from garry.yao to fredck
  • Status changed from review_failed to review

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

comment:24 Changed 6 years ago by sk1p

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 6 years ago by fredck

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

comment:26 Changed 6 years ago by Saare

  • Status changed from review to review_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 6 years ago by Saare

  • Keywords FireFox Opera added

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

Changed 6 years ago by fredck

comment:28 Changed 6 years ago by fredck

  • Keywords Firefox added; FireFox removed
  • Status changed from review_failed to review

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 6 years ago by Saare

  • Status changed from review to review_passed

comment:30 follow-up: Changed 6 years ago by fredck

  • Resolution set to fixed
  • Status changed from review_passed to closed

Fixed with [5750].

comment:31 Changed 6 years ago by technotarek

Excellent news. Your work is very much appreciated.

comment:32 in reply to: ↑ 30 ; follow-up: Changed 6 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 ; follow-up: Changed 6 years ago by 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?

comment:34 in reply to: ↑ 33 ; follow-up: Changed 6 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 6 years ago by fredck

Test case for comment:34 using the JQuery adapter

Changed 6 years ago by fredck

Test case for comment:34 using JavaScript and DOM

comment:35 in reply to: ↑ 34 Changed 6 years ago by fredck

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