Ticket #1503 (closed Bug: fixed)

Opened 6 years ago

Last modified 6 years ago

Errors when styles applied to paragraphs as classname

Reported by: jonhg Owned by: 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 6 years ago.
styletest.fromxml.html (2.4 KB) - added by jonhg 6 years ago.
styletest.fromarray.config.js (435 bytes) - added by jonhg 6 years ago.
styletest.fromxml.config.js (252 bytes) - added by jonhg 6 years ago.
styletest.styles.css (1.1 KB) - added by jonhg 6 years ago.
styletest.styles.xml (372 bytes) - added by jonhg 6 years ago.
1503.patch (1.3 KB) - added by alfonsoml 6 years ago.
Proposed SVN patch

Change History

Changed 6 years ago by jonhg

Changed 6 years ago by jonhg

Changed 6 years ago by jonhg

Changed 6 years ago by jonhg

Changed 6 years ago by jonhg

Changed 6 years ago by jonhg

comment:1 Changed 6 years ago by alfonsoml

  • 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

     
    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 6 years ago 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.

comment:3 Changed 6 years ago by alfonsoml

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

  • Milestone set to FCKeditor 2.6

Changed 6 years ago by alfonsoml

Proposed SVN patch

comment:5 Changed 6 years ago by alfonsoml

  • Keywords Review? added; HasPatch removed

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

comment:6 Changed 6 years ago by alfonsoml

  • Owner set to alfonsoml

comment:7 Changed 6 years ago 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.

comment:8 Changed 6 years ago 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]

comment:9 Changed 6 years ago by w.olchawa

#1887 has been marked as DUP

comment:10 Changed 6 years ago by w.olchawa

#1904 has been marked as DUP

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