Opened 11 years ago
Closed 11 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 )
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"><div id="header" style="background-color: purple; width: 100%; height:100px;"><img alt="email header" height="63" src="http://www.example.net/content/images/header.gif" style="border: 0px;" width="600"></div></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"><div id="header" style="text-align: center;background-color: purple; width: 100%; height:100px;"><img alt="email header" height="63" src="http://www.example.net/content/images/header.gif" style="border: 0px;" width="600"></div></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
- Download 11283.html to
samples/. - Run it.
Attachments (3)
Change History (17)
Changed 11 years ago by
| Attachment: | build-config.js added |
|---|
comment:1 Changed 11 years ago by
| Description: | modified (diff) |
|---|---|
| Version: | 4.3 |
Changed 11 years ago by
| Attachment: | 11283.html added |
|---|
comment:2 Changed 11 years ago by
| Description: | modified (diff) |
|---|---|
| Milestone: | → CKEditor 4.3.2 |
| Status: | new → confirmed |
| Version: | → 4.3 |
comment:3 Changed 11 years ago by
| Summary: | Error on particular content. → [Image2] Div with text-align:center and image inside is not recognised correctly |
|---|
comment:4 Changed 11 years ago by
| Owner: | set to Olek Nowodziński |
|---|---|
| Status: | confirmed → assigned |
comment:5 Changed 11 years ago by
| Status: | assigned → review |
|---|
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.
comment:6 Changed 11 years ago by
#11300 was postponed, but this issue is important, so we need a patch based on master.
comment:7 Changed 11 years ago by
| Status: | review → assigned |
|---|
comment:8 Changed 11 years ago by
| Status: | assigned → review |
|---|
comment:9 follow-up: 10 Changed 11 years ago by
Can we avoid adding full integration tests just for upcasting tests?
comment:10 Changed 11 years ago by
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 follow-up: 12 Changed 11 years ago by
| Status: | review → review_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 Changed 11 years ago by
| Status: | review_failed → review |
|---|
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:initto 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 11 years ago by
| Status: | review → review_passed |
|---|
comment:14 Changed 11 years ago by
| Resolution: | → fixed |
|---|---|
| Status: | review_passed → closed |
git:be11a1b landed in master (e2c75b4 tests).
Confirmed with 11283.html. Image widget incorrectly recognises or handles centered image.