Opened 16 years ago
Closed 16 years ago
#3240 closed Bug (fixed)
Font style not correct with existed style
Reported by: | Garry Yao | Owned by: | Garry Yao |
---|---|---|---|
Priority: | Normal | Milestone: | CKEditor 3.0 |
Component: | General | Version: | |
Keywords: | Confirmed IBM Review+ | Cc: |
Description
When inline style is applied, it should always create at the possible outmost, with selection enlarged.
Reproducing Procedures
- Open the replace by class example page;
- Make the content along with selection as below by double click 'text':
<b><i>^text^</i></b>
- Click on 'Font' to open combo and select 'Arial' option;
- Expected Result:
<p> <span style="font-family: Arial,Helvetica,sans-serif;"><b><i>text</i></b></span></p>
- Actual Result: An JavaScript Error thrown;
<p> <b><i><span style="font-family: Arial,Helvetica,sans-serif;">text</span></i></b></p>
- Expected Result:
Attachments (5)
Change History (22)
comment:1 Changed 16 years ago by
comment:2 Changed 16 years ago by
Owner: | set to Garry Yao |
---|---|
Status: | new → assigned |
#3091 is going to fixed by another approach which doesn't resolve the issue of this bug, so a specific fix should be found.
Changed 16 years ago by
Attachment: | 3240.patch added |
---|
comment:3 Changed 16 years ago by
Keywords: | Review? added |
---|
comment:4 Changed 16 years ago by
Keywords: | Review- added; Review? removed |
---|
Confirmed with IE. Ok with Firefox.
The patch has no effect for me though. These is the way I've reproduced it:
- Pasted the following into Source view:
<b><i>text</i></b>
- Switched to WYSIWYG.
- Double-click on the word "text".
- Selected a Font style.
The <span> has been created inside <b><i>, instead of outside of it. I've reproduced it in the same way before and after the patch with IE7.
comment:5 Changed 16 years ago by
I've found the real cause for this problem is a hideous bug from CKEDITOR.dom.node::getPosition within IE, see the attached TC for detail.
Changed 16 years ago by
Attachment: | 3240_2.patch added |
---|
comment:6 Changed 16 years ago by
Keywords: | Review? added; Review- removed |
---|
comment:7 Changed 16 years ago by
Keywords: | Review- added; Review? removed |
---|
It looks like recent changes have already fixed this one. I'm not able to reproduce it anymore. Can you confirm it?
comment:8 Changed 16 years ago by
Keywords: | Review? added; Review- removed |
---|
I've confirmed that the problem still exist for IE6 at least, and here's the execution stack which cause the problem:
getPosition()
applyInlineStyle
applyToRange()
applyStyle
apply()
comment:9 Changed 16 years ago by
Keywords: | IBM added |
---|
comment:10 Changed 16 years ago by
Keywords: | Review- added; Review? removed |
---|
The patch works.
But it seems to me much of the code in getPosition() can be eliminated if getAddress() is used to compare the node positions. Once we've got the index path of two nodes comparing their positions becomes very easy.
Changed 16 years ago by
Attachment: | 3240_3.patch added |
---|
comment:11 Changed 16 years ago by
Keywords: | Review? added; Review- removed |
---|
Simplify the CKEDITOR.dom.node::getPosition function.
comment:12 Changed 16 years ago by
Keywords: | Review- added; Review? removed |
---|
- The performance optimization of using native compareDocumentPosition() is a good idea - CKEDITOR.dom.node::getPosition() is used in a DFS-like while loop in the style plugin, so performance is important there.
- I can see you copied John Resig's compareDocumentPosition() adaptor for IE here: http://ejohn.org/blog/comparing-document-position/. The values returned are correct.. But please format the code better:
// e.g. If we followed John Resig's indenting... which doesn't necessarily make sense. function standard( a, b ) { return a.compareDocumentPosition ? a.compareDocumentPosition( b ) : a.contains ? ( a != b && a.contains( b ) && CKEDITOR.POSITION_CONTAINS ) + ( a != b && b.contains( a ) && CKEDITOR.POSITION_IS_CONTAINED ) + ( a.sourceIndex >= 0 && b.sourceIndex >= 0 ? ( a.sourceIndex < b.sourceIndex && CKEDITOR.POSITION_PRECEDING ) + ( a.sourceIndex > b.sourceIndex && CKEDITOR.POSITION_FOLLOWING ) : CKEDITOR.POSITION_DISCONNECTED ) + CKEDITOR.POSITION_IDENTICAL : CKEDITOR.POSITION_IDENTICAL; }
So, Review- based on minor code readability issues. The patch is otherwise very good.
comment:13 Changed 16 years ago by
- The CKEDITOR.env object should not be extended for such very specific cases. It also impacts in the size of ckeditor_basic.js.
- There is not need of an complete function replacement here. We just need to have Martin's suggestion applied for those cases where compareDocumentPosition, contains and sourceIndex are not supported.
Changed 16 years ago by
Attachment: | 3240_4.patch added |
---|
comment:14 Changed 16 years ago by
Keywords: | Review? added; Review- removed |
---|---|
Owner: | changed from Garry Yao to Frederico Caldeira Knabben |
Status: | assigned → new |
This new patch simply incorporates Garry's proposal with a few enhancements to it.
comment:15 Changed 16 years ago by
Keywords: | Review+ added; Review? removed |
---|
comment:16 Changed 16 years ago by
Owner: | changed from Frederico Caldeira Knabben to Garry Yao |
---|---|
Status: | new → assigned |
Fixed with [3532].
comment:17 Changed 16 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
This one should be resolved after fixing #3091.