Errors when styles applied to paragraphs as classname

[jonhg]

I have made a few testcases based on sample14.html. The first one load custom styles from xml and the second loads custom styles from javascript array in config file.

Firstly, the behavior is different between these two approaches. When loading custom styles from javascript, the style is applied when chosen, but this is not reflected in the combo box. When loading styles from the xml, the combo box seems to be updated correctly when applying styles to paragraphs.

Secondly, there are a few issues when loading the styles from the xml file as well.
1. The normal style (style without class) is always selected. Expected behaviour is that this style should be selected only when current paragraph has no class (or when it is not overridden by other styles).
2. When chosing the currently selected style, for a given paragraph, a second time, the paragraph is removed! The paragraphs innerHTML is placed directly under body.
3. When a paragraph has no class attribute, all styles are marked as selected. Expected only style "normal" to be selected.

Tested in IE6/IE7

[alfonsoml]

For me it seems that the behavior is the same, no matter the source of the definitions, but I might have looked carefuly at that.

Point 1 is problematic, because the way that it works the system is that the element is selected if the style definition matches the element. I mean: the style definition for a clean p doesn't specify anything with regards to classes, so any p will match it. It's not possible to say that all the attributes of the style definition and the element must match, because as soon as you define an ID on the element it won't be recognized.

For points 2 and 3 this patch seems to work, but it should be reviewed by Fred because he knows why he did it that way.

fckstyle.js

@@ -231 +231 @@
                                                 // Remove overrides defined to the same element name.
                                                 this._RemoveOverrides( pathElement, styleOverrides[ pathElementName ] ) ;
 
-                                                // Remove the element if no more attributes are available.
-                                                this._RemoveNoAttribElement( pathElement ) ;
+                                                // Remove the element if no more attributes are available and it's an inline style element
+                                                if ( this.GetType() == FCK_STYLE_INLINE)
+                                                        this._RemoveNoAttribElement( pathElement ) ;
                                         }
                                 }
                                 else if ( isBoundary )
@@ -486 +487 @@
                 switch ( this.GetType() )
                 {
                         case FCK_STYLE_BLOCK :
-                                return this.CheckElementRemovable( elementPath.Block || elementPath.BlockLimit ) ;
+                                return this.CheckElementRemovable( elementPath.Block || elementPath.BlockLimit, true ) ;
 
                         case FCK_STYLE_INLINE :
 

[alfonsoml]

I think that the behavior for 1 could be done setting an empty string as the value for the class attribute. That way it's clear that the user wants an empty class (or any other attribute) and doesn't care about the rest.

This idea works partially now, the changes would be to adjust the test to see if the attributes of the element match with the ones of the style so an empty string in the style matches a non present attribute in the element (and adjust it could be adjusted the setting so it doesn't set an empty string as it does now, and just removes the attribute instead)

So if the style has defined both class="" and style="" then it's clear that this is "Normal" with no modifications, and any other situation is a different state.

[alfonsoml]

I've tested the idea from comment 2 and it seems to work, these are the changes to the fckstyle.js file:

fckstyle.js

@@ -231 +231 @@
                                                 // Remove overrides defined to the same element name.
                                                 this._RemoveOverrides( pathElement, styleOverrides[ pathElementName ] ) ;
 
-                                                // Remove the element if no more attributes are available.
-                                                this._RemoveNoAttribElement( pathElement ) ;
+                                                // Remove the element if no more attributes are available and it's an inline style element
+                                                if ( this.GetType() == FCK_STYLE_INLINE)
+                                                        this._RemoveNoAttribElement( pathElement ) ;
                                         }
                                 }
                                 else if ( isBoundary )
@@ -486 +487 @@
                 switch ( this.GetType() )
                 {
                         case FCK_STYLE_BLOCK :
-                                return this.CheckElementRemovable( elementPath.Block || elementPath.BlockLimit ) ;
+                                return this.CheckElementRemovable( elementPath.Block || elementPath.BlockLimit, true ) ;
 
                         case FCK_STYLE_INLINE :
 
@@ -688 +689 @@
                         valueB = valueB.replace( /;$/, '' ).toLowerCase() ;
                 }
 
-                return ( valueA == valueB )
+                // Return true if they match or if valueA is null and valueB is an empty string
+                return ( valueA == valueB || ( valueA === null && valueB === '') )
         },
 
         GetFinalAttributeValue : function( attName )

Again, this hasn't been tested thoroughly so it can't land on the SVN because it might break lots of things.

The changes fixes the 1-3 issues, and I don't see any difference between loading the styles from XML or JS.

[alfonsoml]

Proposed SVN patch

[alfonsoml]

I've turned the changes into a patch so it can be reviewed.

[fredck]

This fix looks good Alfonso. My only concern is about the change at line 691. Looking at that function isolated, it sounds weird checking it if is not also true when valueB is null and valueA is ''. Something like this could fix it (not sure "!valueA && !valueB" would always work):

return ( valueA == valueB || ( ( valueA === null || valueA === '' ) && ( valueB === null || valueB === '' ) ) )

If you feel this is correct, go ahead committing it with that change. I would also ask you to please add the whatsnew entry for it.

[alfonsoml]

Ok, I've applied that change as it looks fine.

Fixed with [1378]

[w.olchawa]

#1887 has been marked as DUP

[w.olchawa]

#1904 has been marked as DUP