Opened 6 years ago

Last modified 3 years ago

#8612 confirmed New Feature

DocProps plugin incorrect case handling of META element names - code could be more flexible.

Reported by: NicHolt Owned by:
Priority: Normal Milestone:
Component: UI : Dialogs Version: 3.6
Keywords: Cc:

Description (last modified by Jakub Ś)

docprops.js creates a hash table of meta elements, keyed by the name converted to lower case. However, there are several cases of tests such as:

name in hash

which fail if the meta element name is not all in lower case. Fix is to replace several occurrences of the above with:

name.toLowerCase() in hash

Also in setupMeta:

result = ( name.toLowerCase() in hash ) ? hash[ name.toLowerCase() ].getAttribute( 'content' ) || '' : '';


The below description summarizes first 11 comments.

@Nickholt has extended docProps dialog with extra meta tags values. He copied the code that we use for standard values like ‘author‘ and to his surprise it didn’t work for attributes with values like meta name="THIS.Is.An.Upper.Case.Meta.Name. so he used toLowerCase() Method on name attribute.

Our code works and custom code is completely different story which makes this request rather invalid but I thought that perhaps we could make our code more flexible. Who knows if there won’t be a browser or a mobile tool that needs this change to display documents correctly.

Just a thought but perhaps fixing #8668 which to me may related will also fix this one.

Change History (12)

comment:1 Changed 6 years ago by Jakub Ś

Keywords: META docprops removed
Status: newpending

Looks important.

@NicHolt Could I ask you first for a reproducible TC (E.g. sample HTML file) so that we could see the problem on our own?

comment:3 Changed 6 years ago by NicHolt

Here are the updates:

	function commitMeta( name, isHttp, value )
	{
		return function( doc, html, head )
		{
			var hash = metaHash,
				val = typeof value != 'undefined' ? value : this.getValue();
			if ( !val && ( name.toLowerCase() in hash ) )
				hash[ name.toLowerCase() ].remove();
			else if ( val && ( name.toLowerCase() in hash ) )
				hash[ name.toLowerCase() ].setAttribute( 'content', val );
			else if ( val )
			{
				var meta = new CKEDITOR.dom.element( 'meta', editor.document );
				if ( CKEDITOR.env.ie && ( CKEDITOR.env.ie7Compat || CKEDITOR.env.ie6Compat ) ) {  
					meta.setAttribute( isHttp ? 'http-equiv' : 'Name', name )
				}
				else {
					meta.setAttribute( isHttp ? 'http-equiv' : 'name', name )
				}
				meta.setAttribute( 'content', val );
				head.append( meta );
			}
		};
	}
{{{
	function setupMeta( name, ret )
	{
		return function()
		{
			var hash = metaHash;
			var lcName = name.toLowerCase();
			
			metaList[ lcName ] = lcName;
// following changed by Nic to not setValue if no META element present, to enable default contents to work (IE problem)
			if (lcName in hash ) {
				result = hash[ lcName ].getAttribute( 'content' ) || '';
				this.setValue( result );
			}
			else {
				result = '';
			}

			return ret ? result : null;
		};
	}
}}}




{{{
	function createMetaHash( doc )
	{
		var hash = {},
			metas = doc.getElementsByTag( 'meta' ),
			count = metas.count(); 

		for ( var i = 0; i < count; i++ )
		{
			var meta = metas.getItem( i );  
			if ( CKEDITOR.env.ie && ( CKEDITOR.env.ie7Compat || CKEDITOR.env.ie6Compat ) ) {  
				hash[ meta.getAttribute( meta.hasAttribute( 'http-equiv' ) ? 'http-equiv' : 'Name' ).toLowerCase() ] = meta; 
			}
			else {
				hash[ meta.getAttribute( meta.hasAttribute( 'http-equiv' ) ? 'http-equiv' : 'name' ).toLowerCase() ] = meta; 
			}
		}
		return hash;
	}
}}}

comment:4 Changed 6 years ago by Jakub Ś

Sorry for the long time without any reposnse.

When I was talking about updates I've meant Reproducible TC - test case file or a step by step scenario of a bug (way to reproduce it and what it expected and actual outcome)

You think you could provide something like this (and also for #8613)

comment:5 Changed 6 years ago by Jakub Ś

Status: pendingconfirmed
Version: 3.6.23.6

I may have reproduced the issue.

  1. Open fullPage mode and paste in the below code in head section (notice chaRset)
    <meta content="text/html; chaRset=utf-8" http-equiv="content-type" />
    
  2. Switch to WYSIWYG area and open "Document Properties" dialog.
  3. Notice that "Character Set Encoding" is not recognized and the whole phrase is placed in "Other Character Set Encoding" filed.
  4. Click ok on the dialog and Switch again to Source. Checkout the meta tag - notice the extra charset element
    <meta content="text/html; charset=text/html; chaRset=utf-8" http-equiv="content-type" />
    
  5. If you continute to switch to WYSIWYG, open dialog, click ok and switch to source you will get more extra charsets.

This issue has been reproducible from CKEDitor 3.6.


I'm not sure however if this is what @NicHolt meant as his code does not solve this issue.
@NicHolt is there a chance that you, as the author of this ticket, could provide some extra information like:

  1. Provide a code that is causing the problem,
  2. Tell what this code should break,
  3. Write whay have you used metaList[ lcName ] = lcName; in setupMeta function? Is this some thing unecessary or the code you have provided is not complete
Last edited 6 years ago by Jakub Ś (previous) (diff)

comment:6 Changed 6 years ago by Jakub Ś

This ticket is related to #8668.

Although I'm not sure what this code provided by @NicHolt is suppose to fix as I didn't find any meta tag causing the error. Except for the above of course but the proposed code doesn’t fix this issue.

comment:7 Changed 6 years ago by NicHolt

If a document is loaded that has metatags with names that are not lower case, the hash is created with lower case keys. None of the other code that accesses the meta hash converts the name to lower case, so the test for whether the meta elements is in the hash incorrectly reports that it is not.

I should explain the I have extended docprops to handle a far wider range of metatags (specific to this organisation's metatagging requirements), and many of the meta names are of the form THIS.Is.An.Upper.Case.Meta.Name.

The changes proposed ensure that whatever the case of the metatag name, the hash is correctly constructed and accessed. This ensures that meta tags with names that are not all lower case can be round-tripped from the original document, via docprops (where they may be updated) and back to the document again.

comment:8 Changed 6 years ago by Jakub Ś

So if I understand correctly your custom meta tags like <meta content="some content" Name="THIS.Is.An.Upper.Case.Meta.Name." /> are not shown in docprops dialog?

comment:9 Changed 6 years ago by NicHolt

setupMeta attempts to lookup the meta names in the hash, but because it does not convert the meta name to lower case it does not find it in the hash, So the contents of the meta tag are not pre-loaded into the corresponding dialog field.

Then, when committing the values netered in the dialog, there is a similar problem in commitMeta, which uses the hash to determine whether a meta tag with the name already exists in the document. Again, becuse it does not convert to lower case before testing whether the name is in the hash, the lookup in the hash fails, and commitMeta creates another copy of the meta tag with the same name.

By converting meta names to lower case both before storing them in the hash AND before looking them up in the hash, the proposed code deals with ALL these cases.

comment:11 Changed 6 years ago by NicHolt

Sample HTML

<html>
	<head>
		<title>CKEditor Sample</title>
		<meta content="me" name="Author" />
	</head>
	<body>
		<p>
			sample text</p>
	</body>
</html>

In docprops.js change 'author' in setup and commit to 'Author'. Open the editor and open the docprops dialog.

Expected result: the Author field in the docprops dialog contains "me".

Actual result: The Author field in the docprops dialog is empty.

Now set the Author field in the docprops dialog to "you" and click OK. View Source.

Expected result: a single meta tag

<meta content="you" name="Author" />

Actual result: two meta tags:

<meta content="me" name="Author" />
<meta content="you" name="Author" />

comment:12 Changed 6 years ago by Jakub Ś

Description: modified (diff)
Type: BugNew Feature

comment:13 Changed 6 years ago by Jakub Ś

Description: modified (diff)
Summary: DocProps plugin incorrect case handling of META element namesDocProps plugin incorrect case handling of META element names - code could be more flexible.

@NicHolt since core code works and request concerned only custom changes I have changed your ticket into feature request - to make the code in docProps more flexible.

comment:14 Changed 3 years ago by Jakub Ś

Other bugs that concern docProps plugin: #12624, #10645, #8807, #12496.

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