Ticket #1503 (closed Bug: fixed)

Opened 6 months ago

Last modified 3 months ago

Errors when styles applied to paragraphs as classname

Reported by: jonhg Assigned to: alfonsoml
Priority: Normal Milestone: FCKeditor 2.6
Component: Core : Styles Version: FCKeditor 2.5 Beta
Keywords: Review? Cc:

Description

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

Attachments

styletest.fromarray.html (2.4 kB) - added by jonhg on 11/05/07 12:53:32.
styletest.fromxml.html (2.4 kB) - added by jonhg on 11/05/07 12:54:25.
styletest.fromarray.config.js (435 bytes) - added by jonhg on 11/05/07 12:54:47.
styletest.fromxml.config.js (252 bytes) - added by jonhg on 11/05/07 12:54:56.
styletest.styles.css (1.1 kB) - added by jonhg on 11/05/07 12:55:10.
styletest.styles.xml (372 bytes) - added by jonhg on 11/05/07 12:56:14.
1503.patch (1.3 kB) - added by alfonsoml on 12/03/07 13:38:05.
Proposed SVN patch

Change History

11/05/07 12:53:32 changed by jonhg

  • attachment styletest.fromarray.html added.

11/05/07 12:54:25 changed by jonhg

  • attachment styletest.fromxml.html added.

11/05/07 12:54:47 changed by jonhg

  • attachment styletest.fromarray.config.js added.

11/05/07 12:54:56 changed by jonhg

  • attachment styletest.fromxml.config.js added.

11/05/07 12:55:10 changed by jonhg

  • attachment styletest.styles.css added.

11/05/07 12:56:14 changed by jonhg

  • attachment styletest.styles.xml added.

11/05/07 22:50:26 changed by alfonsoml

  • owner changed.
  • component changed from General to Core : Styles.

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

    old new  
    231231                                                // Remove overrides defined to the same element name. 
    232232                                                this._RemoveOverrides( pathElement, styleOverrides[ pathElementName ] ) ; 
    233233 
    234                                                 // Remove the element if no more attributes are available. 
    235                                                 this._RemoveNoAttribElement( pathElement ) ; 
     234                                                // Remove the element if no more attributes are available and it's an inline style element 
     235                                                if ( this.GetType() == FCK_STYLE_INLINE) 
     236                                                        this._RemoveNoAttribElement( pathElement ) ; 
    236237                                        } 
    237238                                } 
    238239                                else if ( isBoundary ) 
    …… 
    486487                switch ( this.GetType() ) 
    487488                { 
    488489                        case FCK_STYLE_BLOCK : 
    489                                 return this.CheckElementRemovable( elementPath.Block || elementPath.BlockLimit ) ; 
     490                                return this.CheckElementRemovable( elementPath.Block || elementPath.BlockLimit, true ) ; 
    490491 
    491492                        case FCK_STYLE_INLINE : 
    492493 

11/06/07 15:43:44 changed by 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.

11/09/07 22:45:49 changed by alfonsoml

  • keywords set to HasPatch.

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

  • fckstyle.js

    old new  
    231231                                                // Remove overrides defined to the same element name. 
    232232                                                this._RemoveOverrides( pathElement, styleOverrides[ pathElementName ] ) ; 
    233233 
    234                                                 // Remove the element if no more attributes are available. 
    235                                                 this._RemoveNoAttribElement( pathElement ) ; 
     234                                                // Remove the element if no more attributes are available and it's an inline style element 
     235                                                if ( this.GetType() == FCK_STYLE_INLINE) 
     236                                                        this._RemoveNoAttribElement( pathElement ) ; 
    236237                                        } 
    237238                                } 
    238239                                else if ( isBoundary ) 
    …… 
    486487                switch ( this.GetType() ) 
    487488                { 
    488489                        case FCK_STYLE_BLOCK : 
    489                                 return this.CheckElementRemovable( elementPath.Block || elementPath.BlockLimit ) ; 
     490                                return this.CheckElementRemovable( elementPath.Block || elementPath.BlockLimit, true ) ; 
    490491 
    491492                        case FCK_STYLE_INLINE : 
    492493 
    …… 
    688689                        valueB = valueB.replace( /;$/, '' ).toLowerCase() ; 
    689690                } 
    690691 
    691                 return ( valueA == valueB ) 
     692                // Return true if they match or if valueA is null and valueB is an empty string 
     693                return ( valueA == valueB || ( valueA === null && valueB === '') ) 
    692694        }, 
    693695 
    694696        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.

11/24/07 14:25:20 changed by fredck

  • milestone set to FCKeditor 2.6.

12/03/07 13:38:05 changed by alfonsoml

  • attachment 1503.patch added.

Proposed SVN patch

12/03/07 13:38:52 changed by alfonsoml

  • keywords changed from HasPatch to Review?.

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

01/12/08 22:52:50 changed by alfonsoml

  • owner set to alfonsoml.

01/19/08 17:57:52 changed by 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.

01/20/08 23:01:27 changed by alfonsoml

  • status changed from new to closed.
  • resolution set to fixed.

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

Fixed with [1378]

02/20/08 14:11:06 changed by w.olchawa

#1887 has been marked as DUP

02/22/08 10:08:42 changed by w.olchawa

#1904 has been marked as DUP