Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#1503 closed Bug (fixed)

Errors when styles applied to paragraphs as classname

Reported by: Jon Håvard Gundersen Owned by: Alfonso Martínez de Lizarrondo
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 (7)

styletest.fromarray.html (2.4 KB) - added by Jon Håvard Gundersen 10 years ago.
styletest.fromxml.html (2.4 KB) - added by Jon Håvard Gundersen 10 years ago.
styletest.fromarray.config.js (435 bytes) - added by Jon Håvard Gundersen 10 years ago.
styletest.fromxml.config.js (252 bytes) - added by Jon Håvard Gundersen 10 years ago.
styletest.styles.css (1.1 KB) - added by Jon Håvard Gundersen 10 years ago.
styletest.styles.xml (372 bytes) - added by Jon Håvard Gundersen 10 years ago.
1503.patch (1.3 KB) - added by Alfonso Martínez de Lizarrondo 10 years ago.
Proposed SVN patch

Download all attachments as: .zip

Change History (17)

Changed 10 years ago by Jon Håvard Gundersen

Attachment: styletest.fromarray.html added

Changed 10 years ago by Jon Håvard Gundersen

Attachment: styletest.fromxml.html added

Changed 10 years ago by Jon Håvard Gundersen

Changed 10 years ago by Jon Håvard Gundersen

Attachment: styletest.fromxml.config.js added

Changed 10 years ago by Jon Håvard Gundersen

Attachment: styletest.styles.css added

Changed 10 years ago by Jon Håvard Gundersen

Attachment: styletest.styles.xml added

comment:1 Changed 10 years ago by Alfonso Martínez de Lizarrondo

Component: GeneralCore : 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

     
    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

comment:2 Changed 10 years ago by Alfonso Martínez de Lizarrondo

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.

comment:3 Changed 10 years ago by Alfonso Martínez de Lizarrondo

Keywords: HasPatch added

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

  • fckstyle.js

     
    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.

comment:4 Changed 10 years ago by Frederico Caldeira Knabben

Milestone: FCKeditor 2.6

Changed 10 years ago by Alfonso Martínez de Lizarrondo

Attachment: 1503.patch added

Proposed SVN patch

comment:5 Changed 10 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review? added; HasPatch removed

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

comment:6 Changed 10 years ago by Alfonso Martínez de Lizarrondo

Owner: set to Alfonso Martínez de Lizarrondo

comment:7 Changed 10 years ago by Frederico Caldeira Knabben

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.

comment:8 Changed 10 years ago by Alfonso Martínez de Lizarrondo

Resolution: fixed
Status: newclosed

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

Fixed with [1378]

comment:9 Changed 10 years ago by Wojciech Olchawa

#1887 has been marked as DUP

comment:10 Changed 10 years ago by Wojciech Olchawa

#1904 has been marked as DUP

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