#3582 closed Bug (fixed)
Contents of anchors isn't visible
Reported by: | Alfonso Martínez de Lizarrondo | Owned by: | Garry Yao |
---|---|---|---|
Priority: | Normal | Milestone: | CKEditor 3.6 |
Component: | General | Version: | 3.0 |
Keywords: | Cc: | pomu@…, gilles@…, matthewlefflercomputer@… |
Description
Load CKEditor and switch to source mode. Simulate that you are editing any existing page from v2 with anchors that included text:
<p> <a name="test">This is</a> some <strong>sample text</strong>. You are using <a href="http://www.fckeditor.net/">CKEditor</a>.</p>
Switch to design view and the contents of anchors isn't visible:
some sample text. You are using CKEditor.
This worked correctly in V2
Attachments (14)
Change History (64)
comment:1 Changed 16 years ago by
Keywords: | Confirmed added |
---|---|
Milestone: | → CKEditor 3.x |
comment:3 Changed 15 years ago by
Cc: | pomu@… added |
---|
comment:4 Changed 15 years ago by
Ok, the reason seems to be here:
CKEDITOR.editor.prototype.createFakeParserElement = function( realElement, className, realElementType, isResizable ) { var lang = this.lang.fakeobjects, html; var writer = new CKEDITOR.htmlParser.basicWriter(); realElement.writeHtml( writer ); html = writer.getHtml(); var attributes = { 'class' : className, src : CKEDITOR.getUrl( 'images/spacer.gif' ), _cke_realelement : encodeURIComponent( html ), _cke_real_node_type : realElement.type, alt : lang[ realElementType ] || lang.unknown }; if ( realElementType ) attributes._cke_real_element_type = realElementType; if ( isResizable ) attributes._cke_resizable = isResizable; return new CKEDITOR.htmlParser.element( 'img', attributes ); };
The anchor link is simply getting replaced by an image element. Certainly thats the reason of why no text will be showing up.
I played a litle bit with it but found no way to fix it: If i change the htmlParser.element('img'...) to htmlParser.element('a'...) and also change the attributes (just for cke_anchor), the editor deadloops :( Maybe because it will try to always replace it again and again as long as theres a name tag and no href tag like defined in the datafilter afterInit...
However, i hope this information will give someone else, who is deeper in that editor source code involved than me, some useful tip in that direction.
There seemed to be some others before me trying to change the code for the correct behavior because within the link plugin theres already the correct css definition:
'a.cke_anchor' + '{' + 'background-image: url(' + CKEDITOR.getUrl( this.path + 'images/anchor.gif' ) + ');' + 'background-position: 0 center;' + 'background-repeat: no-repeat;' + 'border: 1px solid #a9a9a9;' + 'padding-left: 18px;' + '}'
But this class is never being used...
Juergen
comment:5 Changed 15 years ago by
Ok, i have some working quick and dirty fix:
afterInit : function( editor ) { // Register a filter to displaying placeholders after mode change. var dataProcessor = editor.dataProcessor, dataFilter = dataProcessor && dataProcessor.dataFilter; if ( dataFilter ) { dataFilter.addRules( { elements : { a : function( element ) { var attributes = element.attributes; if ( attributes.name && !attributes.href && !attributes.className ) { attributes.class = 'cke_anchor'; return element; } // if ( attributes.name && !attributes.href ) // return editor.createFakeParserElement( element, 'cke_anchor', 'anchor' ); } } }); } },
I have the changed lines just commented out...
This works for me but i think this could be done some more elegant or more safe (whatever you prefer) ;)
Juergen
comment:6 Changed 15 years ago by
Sorry the ckpacker couldn't work with ".class" entry, so i changed this to:
afterInit : function( editor ) { // Register a filter to displaying placeholders after mode change. var dataProcessor = editor.dataProcessor, dataFilter = dataProcessor && dataProcessor.dataFilter; if ( dataFilter ) { dataFilter.addRules( { elements : { a : function( element ) { var attributes = element.attributes; if ( attributes.name && !attributes.href && !attributes.className ) { element.attributes['class'] = 'cke_anchor'; return element; } // if ( attributes.name && !attributes.href ) // return editor.createFakeParserElement( element, 'cke_anchor', 'anchor' ); } } }); } },
Now the next step is to change the behavior when adding a new anchor. This quick & dirty fix above is to display the already existing anchors i.e. from fckeditor 2.x.
comment:7 Changed 15 years ago by
Cc: | gilles@… added |
---|
comment:11 Changed 14 years ago by
The content is not only invisible it's in fact purged (when using the anchor dialog), what increases severity/priority IMHO.
The only browsers where it works (only with the first anchor): IE7. See #6385
comment:12 Changed 14 years ago by
Owner: | set to Alfonso Martínez de Lizarrondo |
---|---|
Status: | confirmed → review |
The patch takes care of showing the icon at the left of anchors with text, allows to create and modify anchors with text, fixes issues with IE6 .. IE9.
comment:13 Changed 14 years ago by
Milestone: | → CKEditor 3.6 |
---|---|
Version: | SVN (CKEditor) - OLD → 3.0 |
comment:14 Changed 14 years ago by
Status: | review → review_failed |
---|
Why we're still keeping fake element presentation of empty anchor for none-FF, it introduce a huge inconsistency between created anchors (in the sense of elements path bar), even making empty anchor uneditable.
comment:15 follow-up: 18 Changed 14 years ago by
I filed duplicate ticket #5874 for this quite a while ago after attending a conference in the US where this was discussed as a major problem for several attendees.
Subsequently my employer tried to pay to get this resolved (contacted organization repeatedly to try and set something up), but got no where.
I suggest bumping the priority for the fix, as it really is a deal breaker for many people... the competing tools have no such constraints on a basic function.
comment:16 Changed 14 years ago by
I resonate with kamelkev's comments. We are currently using FCKEditor 2.6.x and will continue to do so until this bug is fixed. I intensely dislike using such an antiquated and inferior version but this is really a deal breaker for us.
comment:17 follow-up: 19 Changed 14 years ago by
Status: | review_failed → review |
---|
With regards to being able to remove the fake image for empty anchors as done in Firefox here are my tests:
The element isn't selectable in IE, we can trick it by adding display:inline-block in the css, and then it can be selected and the context menu works, but then it's almost impossible to put the cursor inside anchors with content, and maybe due to a bug with how selection is saved while opening the dialog, the empty anchor doesn't show the current name when it's being edited. It would need a review of the selection logic to understand better the problem.
In Opera the element isn't selectable, it's not possible to edit empty anchors.
In Chrome 8 it's possible to edit the anchor if we add display:inline-block and min-height:18px, but that doesn't work in Safari 5 (Mac), so I don't think that it's worth trying to single case Chrome 8 that way
I think that it's better to leave the way that empty anchors work as is, fix the current problem and we can always try again in another ticket to improve that support.
I've adjusted the patch so the cke_anchor class is added only for IE in compatibility mode with IE6
comment:18 Changed 14 years ago by
Replying to kamelkev:
the competing tools have no such constraints on a basic function.
I understand your pain, but depending on what you mean by "competing tools", I didn't see such a competition at this point among browser based WYSIWYGs.
comment:19 Changed 14 years ago by
Status: | review → review_failed |
---|
Replying to alfonsoml:
I think that it's better to leave the way that empty anchors work as is, fix the current problem and we can always try again in another ticket to improve that support.
Let's have this filing another ticket.
Back to ticket:
- the way how anchor are applied to the selection is wrong, check the following case:
<p>par[agraph1</p> <p>para]graph2</p>
It's enough to just follow the link dialog's implementation at this point.
- We don't need to make FF a specialty for empty anchor, as it's also impossible to make anchor edited in that way.
comment:21 follow-up: 22 Changed 14 years ago by
I'd wait for #1272 so we could make a change here (making cursor blinking inside empty anchor).
comment:22 Changed 14 years ago by
Owner: | changed from Alfonso Martínez de Lizarrondo to Garry Yao |
---|---|
Status: | review_failed → review |
Replying to garry.yao:
I'd wait for #1272 so we could make a change here (making cursor blinking inside empty anchor).
Investigation shows that the fix from #1272 cannot resolve the editing problem in empty anchor, while we'd still go without fake element for the benefits of clipboard friendly and code reduction.
Changed 14 years ago by
Attachment: | 3582_3.patch added |
---|
comment:23 Changed 14 years ago by
Status: | review → review_failed |
---|
Other than finding console.log stuff in the code, the patch unfortunately stops making the Anchor dialog creating any kind of anchors. I'm applying it on the 3.6.x branch.
Changed 14 years ago by
Attachment: | 3582_4.patch added |
---|
comment:24 Changed 14 years ago by
Status: | review_failed → review |
---|
comment:25 Changed 14 years ago by
Status: | review → review_failed |
---|
- Icon is not presented correctly using RTL language.
- It's impossible to move before an empty anchor using the arrow key.
- IE, Opera: Impossible to select an empty anchor and right click on it.
- Opera: Using the following, hitting BACKSPACE removes the anchor.
<p> <a name="f"></a>g^</p>
- We should avoid adding the cke_* classes in CKEDITOR.dom. It should be added by the dialog/datafilter.
Changed 14 years ago by
Attachment: | 3582_5.patch added |
---|
comment:26 Changed 14 years ago by
Status: | review_failed → review |
---|
New patch addressed some of the above mentioned issues except:
IE, Opera: Impossible to select an empty anchor and right click on it.
We've made it easy to edit anchor by double clicking to open dialog, let's leave the context menu fix later.
Opera: Using the following, hitting BACKSPACE removes the anchor.
It's an strange Opera bug, but should be become a blocker.
We should avoid adding the cke_* classes in CKEDITOR.dom. It should be added by the dialog/datafilter.
It's enough to place those additions in plugin and limit to the current editor document, with this approach we have much cleaner code in dialogs, I consider it a good move (e.g. There's no more confusing "cke_anchor" class name displayed inside link dialog adv tab).
comment:27 follow-up: 28 Changed 14 years ago by
Status: | review → review_failed |
---|
We don't need to reinvent the wheel here. It's enough to look at FCKeditor 2 and reproduce the same behavior.
I'm telling that because the last patch presents the following issues:
- Empty anchors are badly aligned to the text (because of display:inline-block).
- It's not possible to drag anchors to change their position (idem).
- Anchor with text shows the "text" cursor when moving over the icon. The arrow should show up instead. (cursor:auto should fix it).
- When double-clicking to fully select an anchor with text, the anchor dialog is showing. The dialog must open only if the anchor is empty or if its full contents are already selected (double-click twice).
Changed 14 years ago by
Attachment: | 3582_6.patch added |
---|
comment:28 Changed 14 years ago by
Status: | review_failed → review |
---|
Replying to fredck:
We don't need to reinvent the wheel here. It's enough to look at FCKeditor 2 and reproduce the same behavior.
No invention, just an improvement, as fake element (for empty anchor is not good for a11y and clipboard).
I'm telling that because the last patch presents the following issues:
- Empty anchors are badly aligned to the text (because of display:inline-block).
Ok, it's fixable, but depends on what "badly" means, there's no standard behavior.
- It's not possible to drag anchors to change their position (idem).
For empty anchor, WFM in both FF and IE (thanks to display:inline-block), not available for Opera and Webkit same as v2.
- Anchor with text shows the "text" cursor when moving over the icon. The arrow should show up instead. (cursor:auto should fix it).
This's also highly browser dependent, e.g. FF doesn't show "move" type on image also, but as you pointed out, it's enough to avoid displaying as "text".
- When double-clicking to fully select an anchor with text, the anchor dialog is showing. The dialog must open only if the anchor is empty or if its full contents are already selected (double-click twice).
True, while this's an existing defect so out of the scope here.
I've just added some style adjustment pointed by Fred to the previous patch.
comment:29 follow-up: 30 Changed 14 years ago by
Status: | review → review_failed |
---|
- Moving with the arrow key before the anchor is still not working properly, cursor goes one character before.
- IE: Use the following, hit BACKSPACE, anchor is removed (didn't happen on 3582_4).
<p> abc<a name="hgf"></a>d^ef</p>
- IE and Opera still have the wrong cursor, not sure it's fixable.
- IE6: Create an empty anchor, click it, move it somewhere else inside the document, double click it, note that it's gone.
Changed 14 years ago by
Attachment: | 3582_7.patch added |
---|
comment:30 Changed 14 years ago by
Status: | review_failed → review |
---|
- Moving with the arrow key before the anchor is still not working properly, cursor goes one character before.
Ok now I understand the point but really not a big issue, as the left-hand-side position is still available by using one arrow key combine with one with reverse direction.
- IE: Use the following, hit BACKSPACE, anchor is removed (didn't happen on 3582_4).
- IE6: Create an empty anchor, click it, move it somewhere else inside the document, double click it, note that it's gone.
Ok...considering that (old) IE has more troubles than benefits, new patch allow this fall back to fake only in IE version 6,7,8.
- IE and Opera still have the wrong cursor, not sure it's fixable.
Again, it's not a "right" or "wrong" situation, the cursor shape depends on browser, and some browsers like Opera and Webkit just prefer the default cursor, even on images, so it's more of nit-pick.
comment:31 Changed 14 years ago by
Status: | review → review_failed |
---|
All issues from my previous review are still on the last patch. This is not acceptable. I'm taking over the ticket.
comment:32 Changed 14 years ago by
Owner: | changed from Garry Yao to Frederico Caldeira Knabben |
---|---|
Status: | review_failed → assigned |
Changed 14 years ago by
Attachment: | 3582_8.patch added |
---|
comment:33 Changed 14 years ago by
Status: | assigned → review |
---|
This new patch has most of the dialog code proposed on the previous patch. I was most focused on proper styling, compatibility and minor details.
I've also removed several of the selection related changes from the previous patch. I was not sure what they were doing there and I would prefer to avoid them. Please let me know if I've misobserved something.
comment:34 follow-up: 35 Changed 14 years ago by
Status: | review → review_failed |
---|
I'm surprised as you're proposing non-fake for all IEs (perhaps thanks to "contenteditable=false"), but leaving Webkit and Opera outdoor looks bad:
// Opera and WebKit don't make it possible to select empty anchors. Fake // elements must be used for them. fakeAnchor : CKEDITOR.env.opera || CKEDITOR.env.webkit,
I can confirm at least for Webkit this's able to workaround with the following styles:
-webkit-user-drag:element;-webkit-user-modify:read-only
Also there's no reason for using synthetic class name for styling anchor in IE9, it leads to leaking those classes to all dialog's advanced tab, css3 is simply there
a[name]:empty {...}
I've also removed several of the selection related changes from the previous patch. I was not sure what they were doing there and I would prefer to avoid them. Please let me know if I've misobserved something.
I can confirm it's not needed for now, it was used when "display:inline" are set on anchor before.
Changed 14 years ago by
Attachment: | 3582_9.patch added |
---|
comment:35 Changed 14 years ago by
Status: | review_failed → review |
---|
Replying to garry.yao:
leaving Webkit and Opera outdoor looks bad
I was not able to find any combination that would make it work properly with these browsers. Not a big issue though.
I can confirm at least for Webkit this's able to workaround with the following styles:
-webkit-user-drag:element;-webkit-user-modify:read-only
I've tried this one with no success. Empty anchors where not selectable, draggable nor had the context menu.
Also there's no reason for using synthetic class name for styling anchor in IE9
Ok, the new patch addresses this issue. I've confirmed it to work well with IE9.
comment:36 follow-up: 37 Changed 14 years ago by
Status: | review → review_failed |
---|
[IE] The cke_anchor_empty
class is present in the link dialog if one uses <a href="#" name="a"></a>
. The cke_anchor
class is present there if one uses <a href="#" name="a">a</a>
.
The fake elements title reads "unknown object".
[Opera/Webkit] Elements such as <a href="b" name="a"></a>
are treated as anchors instead of links (regarding dialog type).
[IE<8] Create an anchor using the dialog and right click on it. Note that the anchor option does not appear. If you switch to source and back, it works.
[IE<8] RTL non-empty anchors are incorrectly displayed.
Changed 14 years ago by
Attachment: | 3582_10.patch added |
---|
comment:37 Changed 14 years ago by
Status: | review_failed → review |
---|
Replying to Saare:
[IE<8] RTL non-empty anchors are incorrectly displayed.
This looks like an IE issue with padding. I'm afraid we'll have to live with this issue on a dedicated ticket.
The new patch addresses all the other issues.
Changed 14 years ago by
Attachment: | 3582_11.patch added |
---|
comment:39 Changed 14 years ago by
Status: | review_failed → review |
---|
My mistake... a half a line change fixed it.
comment:40 Changed 14 years ago by
Status: | review → review_failed |
---|
- The fake elements title reads "unknown object".
- [Opera/WebKit] Using
<a href="b" name="a"></a>
, right click on the object, choose link properties. Note that the fields in the dialog remain empty.
Changed 14 years ago by
Attachment: | 3582_12.patch added |
---|
comment:41 Changed 14 years ago by
Status: | review_failed → review |
---|
This new patch treats empty anchor as anchor only, regardless of having href. So the link context menu will not appear for it. In any case, having such kind of empty elements is an edge case. Any further issue related to it can be treated on separated tickets, to avoid blocking this ticket.
comment:42 Changed 14 years ago by
Status: | review → review_failed |
---|
I'm not sure if it's the way to go, but let's stick to it for now, one problem though with IE and FF: double clicking on such element opens the link dialog, it should open the anchor dialog for consistency.
[IE<8] Opening the link dialog for regular links opens the link dialog with an error.
Looks good to me besides.
Changed 14 years ago by
Attachment: | 3582_13.patch added |
---|
comment:43 Changed 14 years ago by
Status: | review_failed → review |
---|
comment:44 Changed 14 years ago by
Status: | review → review_failed |
---|
- [IE] It's now impossible to have read only links:
<a href="http://ckeditor.com/" contenteditable="false">CKEditor</a>
- Elements path incorrect for empty anchor:
<a href="#" name="link"></a>
- "cke_anchor" appears in non-needed browsers after using link dialog.
Changed 14 years ago by
Attachment: | 3582_14.patch added |
---|
comment:45 Changed 14 years ago by
Owner: | changed from Frederico Caldeira Knabben to Garry Yao |
---|---|
Status: | review_failed → review |
_14 with changes to address above issues based on _13.
comment:46 Changed 14 years ago by
Status: | review → review_passed |
---|
comment:47 Changed 14 years ago by
Resolution: | → fixed |
---|---|
Status: | review_passed → closed |
Fixed with [6766].
comment:48 Changed 14 years ago by
When double-clicking to fully select an anchor with text, the anchor dialog is showing. The dialog must open only if the anchor is empty or if its full contents are already selected (double-click twice).
#7728 filed.
There is an icon only.