Ticket #3582 (closed Bug: fixed)

Opened 5 years ago

Last modified 3 years ago

Contents of anchors isn't visible

Reported by: alfonsoml 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

3582.patch (5.8 KB) - added by alfonsoml 4 years ago.
Proposed patch
3582_2.patch (5.8 KB) - added by alfonsoml 4 years ago.
Revised patch
3582_3.patch (13.5 KB) - added by garry.yao 4 years ago.
3582_4.patch (14.4 KB) - added by garry.yao 4 years ago.
3582_5.patch (14.0 KB) - added by garry.yao 4 years ago.
3582_6.patch (14.0 KB) - added by garry.yao 4 years ago.
3582_7.patch (21.7 KB) - added by garry.yao 4 years ago.
3582_8.patch (16.7 KB) - added by fredck 4 years ago.
3582_9.patch (16.9 KB) - added by fredck 4 years ago.
3582_10.patch (21.4 KB) - added by fredck 4 years ago.
3582_11.patch (21.4 KB) - added by fredck 4 years ago.
3582_12.patch (21.3 KB) - added by fredck 4 years ago.
3582_13.patch (21.4 KB) - added by fredck 4 years ago.
3582_14.patch (22.4 KB) - added by garry.yao 3 years ago.

Change History

comment:1 Changed 5 years ago by arczi

  • Keywords Confirmed added
  • Milestone set to CKEditor 3.x

There is an icon only.

comment:2 Changed 5 years ago by fredck

#4633 has been marked as DUP.

comment:3 Changed 5 years ago by pomu0325

  • Cc pomu@… added

comment:4 Changed 5 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 5 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 5 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 5 years ago by Webunity

  • Cc gilles@… added

comment:8 Changed 4 years ago by mattleff

  • Cc matthewlefflercomputer@… added

+1

comment:9 Changed 4 years ago by fredck

#5874 has been marked as DUP.

comment:10 Changed 4 years ago by fredck

  • Milestone CKEditor 3.x deleted

Milestone CKEditor 3.x deleted

comment:11 Changed 4 years ago by flos

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 4 years ago by alfonsoml

Proposed patch

comment:12 Changed 4 years ago by alfonsoml

  • Status changed from confirmed to review
  • Owner set to alfonsoml

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 4 years ago by fredck

  • Version changed from SVN (CKEditor) - OLD to 3.0
  • Milestone set to CKEditor 3.6

comment:14 Changed 4 years ago by garry.yao

  • Status changed from review to 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 4 years ago by kamelkev

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 4 years ago by mattleff

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 4 years ago by alfonsoml

  • Status changed from review_failed to 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

Changed 4 years ago by alfonsoml

Revised patch

comment:18 in reply to: ↑ 15 Changed 4 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 4 years ago by garry.yao

  • Status changed from review to 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:

  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 4 years ago by kamelkev

Just a reminder that many people are still waiting on this

comment:21 follow-up: ↓ 22 Changed 4 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 4 years ago by garry.yao

  • Status changed from review_failed to review
  • Owner changed from alfonsoml to garry.yao

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 4 years ago by garry.yao

comment:23 Changed 4 years ago by fredck

  • Status changed from review to 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 4 years ago by garry.yao

comment:24 Changed 4 years ago by garry.yao

  • Status changed from review_failed to review

comment:25 Changed 4 years ago by Saare

  • Status changed from review to 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 4 years ago by garry.yao

comment:26 Changed 4 years ago by garry.yao

  • Status changed from review_failed to 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 4 years ago by fredck

  • Status changed from review to 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 4 years ago by garry.yao

comment:28 in reply to: ↑ 27 Changed 4 years ago by garry.yao

  • Status changed from review_failed to 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 4 years ago by Saare

  • Status changed from review to 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 4 years ago by garry.yao

comment:30 in reply to: ↑ 29 Changed 4 years ago by garry.yao

  • Status changed from review_failed to 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 4 years ago by fredck

  • Status changed from review to 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 4 years ago by fredck

  • Owner changed from garry.yao to fredck
  • Status changed from review_failed to assigned

Changed 4 years ago by fredck

comment:33 Changed 4 years ago by fredck

  • Status changed from assigned to 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 4 years ago by garry.yao

  • Status changed from review to 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 4 years ago by fredck

comment:35 in reply to: ↑ 34 Changed 4 years ago by fredck

  • Status changed from review_failed to 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 4 years ago by Saare

  • Status changed from review to 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 4 years ago by fredck

comment:37 in reply to: ↑ 36 Changed 4 years ago by fredck

  • Status changed from review_failed to 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.

comment:38 Changed 4 years ago by Saare

  • Status changed from review to review_failed

Last patch ruins the link dialog.

Changed 4 years ago by fredck

comment:39 Changed 4 years ago by fredck

  • Status changed from review_failed to review

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

comment:40 Changed 4 years ago by Saare

  • Status changed from review to 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 4 years ago by fredck

comment:41 Changed 4 years ago by fredck

  • Status changed from review_failed to 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 4 years ago by Saare

  • Status changed from review to 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 4 years ago by fredck

comment:43 Changed 4 years ago by fredck

  • Status changed from review_failed to review

comment:44 Changed 3 years ago by garry.yao

  • Status changed from review to review_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 3 years ago by garry.yao

comment:45 Changed 3 years ago by garry.yao

  • Status changed from review_failed to review
  • Owner changed from fredck to garry.yao

_14 with changes to address above issues based on _13.

comment:46 Changed 3 years ago by fredck

  • Status changed from review to review_passed

comment:47 Changed 3 years ago by garry.yao

  • Status changed from review_passed to closed
  • Resolution set to fixed

Fixed with [6766].

comment:48 Changed 3 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 3 years ago by alfonsoml

Minor fix added with [6852]

comment:50 Changed 3 years ago by kamelkev

#7848 filed

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