Opened 5 years ago

Closed 5 years ago

#11283 closed Bug (fixed)

[Image2] Div with text-align:center and image inside is not recognised correctly

Reported by: Marcus Bointon Owned by: Olek Nowodziński
Priority: Normal Milestone: CKEditor 4.3.2
Component: General Version: 4.3
Keywords: Cc:

Description (last modified by Piotrek Koszuliński)

This bug was originally reported here.

Here's a really simple, minimal example of a CKEditor page, using a fresh download of 4.3, and it works fine:

<!DOCTYPE html>
<html>
<head>
    <title></title>
</head>
<body>
<script type="text/javascript" src="/ckeditor/ckeditor.js"></script>
<textarea id="html_header" name="html_header" rows="8" cols="60">&lt;div id="header" style="background-color: purple; width: 100%; height:100px;"&gt;&lt;img alt="email header" height="63" src="http://www.example.net/content/images/header.gif" style="border: 0px;" width="600"&gt;&lt;/div&gt;</textarea>
<script type="text/javascript">
    CKEDITOR.replace('html_header', {
        "baseHref":"http://example.com/",
        "toolbarStartupExpanded":true,
        "customConfig":false
    });
</script>
</body>
</html>

Now add 'text-align: center;' to the inline style in the textarea content so it becomes this:

<!DOCTYPE html>
<html>
<head>
    <title></title>
</head>
<body>
<script type="text/javascript" src="/ckeditor/ckeditor.js"></script>
<textarea id="html_header" name="html_header" rows="8" cols="60">&lt;div id="header" style="text-align: center;background-color: purple; width: 100%; height:100px;"&gt;&lt;img alt="email header" height="63" src="http://www.example.net/content/images/header.gif" style="border: 0px;" width="600"&gt;&lt;/div&gt;</textarea>
<script type="text/javascript">
    CKEDITOR.replace('html_header', {
        "baseHref":"http://example.com/",
        "toolbarStartupExpanded":true,
        "customConfig":false
    });
</script>
</body>
</html>

CKEditor now fails to run on load, giving this error, and leaving the editor unresponsive:

[Error] TypeError: 'null' is not an object (evaluating 'a.previous=this.previous')
	replaceWith (ckeditor.js, line 249)
	upcast (ckeditor.js, line 1016)
	(anonymous function) (ckeditor.js, line 979)
	forEach (ckeditor.js, line 264)
	forEach (ckeditor.js, line 264)
	(anonymous function) (ckeditor.js, line 978)
	j (ckeditor.js, line 10)
	(anonymous function) (ckeditor.js, line 12)
	fire (ckeditor.js, line 13)
	toHtml (ckeditor.js, line 285)
	setData (ckeditor.js, line 763)
	(anonymous function) (ckeditor.js, line 327)
	j (ckeditor.js, line 10)
	(anonymous function) (ckeditor.js, line 12)
	fire (ckeditor.js, line 13)
	setData (ckeditor.js, line 239)
	b (ckeditor.js, line 759)
	(anonymous function) (ckeditor.js, line 761)
	setMode (ckeditor.js, line 313)
	(anonymous function) (ckeditor.js, line 308)
	j (ckeditor.js, line 10)
	(anonymous function) (ckeditor.js, line 12)
	fire (ckeditor.js, line 13)
	fireOnce (ckeditor.js, line 12)
	fireOnce (ckeditor.js, line 13)
	(anonymous function) (ckeditor.js, line 234)
	l (ckeditor.js, line 214)
	u (ckeditor.js, line 214)
	s (ckeditor.js, line 214)
	(anonymous function) (ckeditor.js, line 215)

If I take out the text-align style, it works again. I've no idea why it should break like that, I'm assuming it's a bug.

I attach my build-config.js file.

To reproduce

  1. Download 11283.html to samples/.
  2. Run it.

Attachments (3)

build-config.js (2.7 KB) - added by Marcus Bointon 5 years ago.
11283.html (700 bytes) - added by Piotrek Koszuliński 5 years ago.
11283.2.html (901 bytes) - added by Olek Nowodziński 5 years ago.
Yet another sample

Download all attachments as: .zip

Change History (17)

Changed 5 years ago by Marcus Bointon

Attachment: build-config.js added

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

Description: modified (diff)
Version: 4.3

Changed 5 years ago by Piotrek Koszuliński

Attachment: 11283.html added

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

Description: modified (diff)
Milestone: CKEditor 4.3.2
Status: newconfirmed
Version: 4.3

Confirmed with 11283.html. Image widget incorrectly recognises or handles centered image.

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

Summary: Error on particular content.[Image2] Div with text-align:center and image inside is not recognised correctly

comment:4 Changed 5 years ago by Olek Nowodziński

Owner: set to Olek Nowodziński
Status: confirmedassigned

comment:5 Changed 5 years ago by Olek Nowodziński

Status: assignedreview

I'm able to reproduce the problem. An error is thrown because centering <div> is wrongly detected (false positive because of text-align:center) while upcasting. Once detected, the following lines consider DOM structure as a centered, captioned widget while, in a matter of fact, it is an unaligned, non-captioned one.

The standard way of centering is <p style="text-align:center"><img/></p>. Using <div style="text-align:center">...</div> should only be possible in enter modes different than ENTER_P.

I pushed a fix to t/11283 that improves biased centering wrapper detection and upcasting (+corresponding tests).

This ticket requires #11300 to be R+d.

Last edited 5 years ago by Olek Nowodziński (previous) (diff)

Changed 5 years ago by Olek Nowodziński

Attachment: 11283.2.html added

Yet another sample

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

#11300 was postponed, but this issue is important, so we need a patch based on master.

comment:7 Changed 5 years ago by Olek Nowodziński

Status: reviewassigned

comment:8 Changed 5 years ago by Olek Nowodziński

Status: assignedreview

I extracted crucial parts of code from t/11283 to t/11283b, which is based on master (+tests in corresponding branch).

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

Can we avoid adding full integration tests just for upcasting tests?

comment:10 in reply to:  9 Changed 5 years ago by Olek Nowodziński

Replying to Reinmar:

Can we avoid adding full integration tests just for upcasting tests?

I created a separate file for upcasting, moved tests cases there and extended them in t/11283c.

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

Status: reviewreview_failed

Unnecessary object creation, unnecessarily ugly and long ;)

if ( !{ figure: 1, img: 1 }[ childName ] )

This unfortunately does not make sense:

if ( childName == 'img' && editor.activeEnterMode == CKEDITOR.ENTER_P )

Match rule and upcast methods know nothing about enter mode in which content is currently processed. So it neither can be editor.enterMode nor activeEnterMode. This information is simply missing, so we have to live without it - e.g. make the check less precise (upcast both - centering div and p). However, if that's not doable or sounds bad, then we can use editor.enterMode for now, because we don't have nested widgets.


You cannot override tc.editorBot in the middle of tests. I could not understand why the last tc is in enter mode BR... It's completely confusing. Use async:init to initialize necessary editors using tools.setupEditors.

comment:12 in reply to:  11 Changed 5 years ago by Olek Nowodziński

Status: review_failedreview

Replying to Reinmar:

Unnecessary object creation, unnecessarily ugly and long ;)

if ( !{ figure: 1, img: 1 }[ childName ] )

It's shorter when compressed. I decided to follow your way mostly because of performance concerns (it's DP...), anyway.

This unfortunately does not make sense:

if ( childName == 'img' && editor.activeEnterMode == CKEDITOR.ENTER_P )

Match rule and upcast methods know nothing about enter mode in which content is currently processed. So it neither can be editor.enterMode nor activeEnterMode. This information is simply missing, so we have to live without it - e.g. make the check less precise (upcast both - centering div and p). However, if that's not doable or sounds bad, then we can use editor.enterMode for now, because we don't have nested widgets.

Changed to enterMode since upcasting <div text-align="center">...</div> would be possibly messy in ENTER_P. Moved discussion to #11394.

You cannot override tc.editorBot in the middle of tests. I could not understand why the last tc is in enter mode BR... It's completely confusing. Use async:init to initialize necessary editors using tools.setupEditors.

I re-factorized tests. Additionaly, I extended CKTESTER.tools.setUpEditors so it passes an "object of bots" to the callback, making it more useful.

The code remains in t/11283b and t/11283c.

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

Status: reviewreview_passed

comment:14 Changed 5 years ago by Olek Nowodziński

Resolution: fixed
Status: review_passedclosed

git:be11a1b landed in master (e2c75b4 tests).

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