Opened 15 years ago

Closed 13 years ago

Last modified 13 years ago

#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)

3582.patch (5.8 KB) - added by Alfonso Martínez de Lizarrondo 13 years ago.
Proposed patch
3582_2.patch (5.8 KB) - added by Alfonso Martínez de Lizarrondo 13 years ago.
Revised patch
3582_3.patch (13.5 KB) - added by Garry Yao 13 years ago.
3582_4.patch (14.4 KB) - added by Garry Yao 13 years ago.
3582_5.patch (14.0 KB) - added by Garry Yao 13 years ago.
3582_6.patch (14.0 KB) - added by Garry Yao 13 years ago.
3582_7.patch (21.7 KB) - added by Garry Yao 13 years ago.
3582_8.patch (16.7 KB) - added by Frederico Caldeira Knabben 13 years ago.
3582_9.patch (16.9 KB) - added by Frederico Caldeira Knabben 13 years ago.
3582_10.patch (21.4 KB) - added by Frederico Caldeira Knabben 13 years ago.
3582_11.patch (21.4 KB) - added by Frederico Caldeira Knabben 13 years ago.
3582_12.patch (21.3 KB) - added by Frederico Caldeira Knabben 13 years ago.
3582_13.patch (21.4 KB) - added by Frederico Caldeira Knabben 13 years ago.
3582_14.patch (22.4 KB) - added by Garry Yao 13 years ago.

Download all attachments as: .zip

Change History (64)

comment:1 Changed 15 years ago by Artur Formella

Keywords: Confirmed added
Milestone: CKEditor 3.x

There is an icon only.

comment:2 Changed 14 years ago by Frederico Caldeira Knabben

#4633 has been marked as DUP.

comment:3 Changed 14 years ago by pomu0325

Cc: pomu@… added

comment:4 Changed 14 years ago by Juergen

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 14 years ago by Juergen

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 14 years ago by Juergen

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 14 years ago by Gilles van den Hoven

Cc: gilles@… added

comment:8 Changed 14 years ago by Matthew Leffler

Cc: matthewlefflercomputer@… added

+1

comment:9 Changed 14 years ago by Frederico Caldeira Knabben

#5874 has been marked as DUP.

comment:10 Changed 14 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.x

Milestone CKEditor 3.x deleted

comment:11 Changed 13 years ago by Flo Schuessel

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

Changed 13 years ago by Alfonso Martínez de Lizarrondo

Attachment: 3582.patch added

Proposed patch

comment:12 Changed 13 years ago by Alfonso Martínez de Lizarrondo

Owner: set to Alfonso Martínez de Lizarrondo
Status: confirmedreview

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 13 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.6
Version: SVN (CKEditor) - OLD3.0

comment:14 Changed 13 years ago by Garry Yao

Status: reviewreview_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 Changed 13 years ago by Kevin Kamel

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 13 years ago by Matthew Leffler

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 Changed 13 years ago by Alfonso Martínez de Lizarrondo

Status: review_failedreview

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

Changed 13 years ago by Alfonso Martínez de Lizarrondo

Attachment: 3582_2.patch added

Revised patch

comment:18 in reply to:  15 Changed 13 years ago by Garry Yao

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 in reply to:  17 Changed 13 years ago by Garry Yao

Status: reviewreview_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:

  1. 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.

  1. 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:20 Changed 13 years ago by Kevin Kamel

Just a reminder that many people are still waiting on this

comment:21 Changed 13 years ago by Garry Yao

I'd wait for #1272 so we could make a change here (making cursor blinking inside empty anchor).

comment:22 in reply to:  21 Changed 13 years ago by Garry Yao

Owner: changed from Alfonso Martínez de Lizarrondo to Garry Yao
Status: review_failedreview

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 13 years ago by Garry Yao

Attachment: 3582_3.patch added

comment:23 Changed 13 years ago by Frederico Caldeira Knabben

Status: reviewreview_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 13 years ago by Garry Yao

Attachment: 3582_4.patch added

comment:24 Changed 13 years ago by Garry Yao

Status: review_failedreview

comment:25 Changed 13 years ago by Sa'ar Zac Elias

Status: reviewreview_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 13 years ago by Garry Yao

Attachment: 3582_5.patch added

comment:26 Changed 13 years ago by Garry Yao

Status: review_failedreview

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 Changed 13 years ago by Frederico Caldeira Knabben

Status: reviewreview_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 13 years ago by Garry Yao

Attachment: 3582_6.patch added

comment:28 in reply to:  27 Changed 13 years ago by Garry Yao

Status: review_failedreview

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 Changed 13 years ago by Sa'ar Zac Elias

Status: reviewreview_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 13 years ago by Garry Yao

Attachment: 3582_7.patch added

comment:30 in reply to:  29 Changed 13 years ago by Garry Yao

Status: review_failedreview
  • 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 13 years ago by Frederico Caldeira Knabben

Status: reviewreview_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 13 years ago by Frederico Caldeira Knabben

Owner: changed from Garry Yao to Frederico Caldeira Knabben
Status: review_failedassigned

Changed 13 years ago by Frederico Caldeira Knabben

Attachment: 3582_8.patch added

comment:33 Changed 13 years ago by Frederico Caldeira Knabben

Status: assignedreview

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 Changed 13 years ago by Garry Yao

Status: reviewreview_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 13 years ago by Frederico Caldeira Knabben

Attachment: 3582_9.patch added

comment:35 in reply to:  34 Changed 13 years ago by Frederico Caldeira Knabben

Status: review_failedreview

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 Changed 13 years ago by Sa'ar Zac Elias

Status: reviewreview_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 13 years ago by Frederico Caldeira Knabben

Attachment: 3582_10.patch added

comment:37 in reply to:  36 Changed 13 years ago by Frederico Caldeira Knabben

Status: review_failedreview

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.

comment:38 Changed 13 years ago by Sa'ar Zac Elias

Status: reviewreview_failed

Last patch ruins the link dialog.

Changed 13 years ago by Frederico Caldeira Knabben

Attachment: 3582_11.patch added

comment:39 Changed 13 years ago by Frederico Caldeira Knabben

Status: review_failedreview

My mistake... a half a line change fixed it.

comment:40 Changed 13 years ago by Sa'ar Zac Elias

Status: reviewreview_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 13 years ago by Frederico Caldeira Knabben

Attachment: 3582_12.patch added

comment:41 Changed 13 years ago by Frederico Caldeira Knabben

Status: review_failedreview

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 13 years ago by Sa'ar Zac Elias

Status: reviewreview_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 13 years ago by Frederico Caldeira Knabben

Attachment: 3582_13.patch added

comment:43 Changed 13 years ago by Frederico Caldeira Knabben

Status: review_failedreview

comment:44 Changed 13 years ago by Garry Yao

Status: reviewreview_failed
  1. [IE] It's now impossible to have read only links:
    <a href="http://ckeditor.com/" contenteditable="false">CKEditor</a>
    
  2. Elements path incorrect for empty anchor:
    <a href="#" name="link"></a>
    
  3. "cke_anchor" appears in non-needed browsers after using link dialog.

Changed 13 years ago by Garry Yao

Attachment: 3582_14.patch added

comment:45 Changed 13 years ago by Garry Yao

Owner: changed from Frederico Caldeira Knabben to Garry Yao
Status: review_failedreview

_14 with changes to address above issues based on _13.

comment:46 Changed 13 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

comment:47 Changed 13 years ago by Garry Yao

Resolution: fixed
Status: review_passedclosed

Fixed with [6766].

comment:48 Changed 13 years ago by Garry Yao

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.

comment:49 Changed 13 years ago by Alfonso Martínez de Lizarrondo

Minor fix added with [6852]

comment:50 Changed 13 years ago by Kevin Kamel

#7848 filed

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy