Opened 10 years ago

Closed 9 years ago

Last modified 9 years ago

#1271 closed Bug (worksforme)

Opera: Small space is appended when applying styles to collapsed selections

Reported by: fredck Owned by: hallvord@…
Priority: Must have (possibly next milestone) Milestone: Opera Compatibility
Component: Core : Styles Version: SVN (FCKeditor) - Retired
Keywords: Cc: hallvord@…

Description

This bug is a follow up to #253. This specific feature has been introduced for that ticket, but Opera presents a strange behavior.

Steps to Reproduce

  1. Load sample01.html;
  2. Place the cursor in the middle of the word "some". For example, after the "o";
  3. Press CTRL+B several times. The toolbar will behave correctly to the command, but note that a small space is added for each time it get pressed.

Analyzing the DOM after CTRL+B, all we have is "so<b></b>me", with the selection inside <b>. That was our intention, but the small space get rendered for the empty <b>.

The code which applies the style can be found in the "_ApplyInlineStyle" function in fckstyle.js. It can be condensed to the following code, which is the only way we have found to make it work with Opera:

// Here the <b> tag is build.
var collapsedElement = this.BuildElement( range.Window.document ) ;
collapsedElement.innerHTML = '&nbsp;' ;
range.InsertNode( collapsedElement ) ;
range.MoveToNodeContents( collapsedElement ) ;
range.Select() ;
FCKSelection.Delete() ;

To note that "range" is FCKDomRange, not a standard DOM Range object.

Attachments (2)

collapsed_select_test.html (2.2 KB) - added by fredck 10 years ago.
TC. Changed it to make it easier to see the whitespace issue.
firebug-ctrl-b.png (9.4 KB) - added by hallvord@… 10 years ago.
screen shot of Firebug DOM HTML view after several ctrl-b presses

Download all attachments as: .zip

Change History (14)

comment:1 Changed 10 years ago by fredck

I've even simplified the Opera implementation for it with [854]:

var collapsedElement = this.BuildElement( doc ) ;
collapsedElement.appendChild( doc.createTextNode('') ) ;
range.InsertNode( collapsedElement ) ;
range.MoveToNodeContents( collapsedElement ) ;
range.Select() ;

But we still have the undesired whitespace behavior. Not only, if we move the caret to the right with the keyboard right after pressing CTRL+B, when returning to the left, the cursor will fell inside the empty <b></b>, which is also not desired. It must simply be ignored when moving the caret, as it does with IE, FF and Safari.

To note that, empty <b></b> is ignored when loading a document with it. It is not being ignored only at our style case, probably because it contains an empty text node inside of it.

comment:2 Changed 10 years ago by fredck

I've attached a simple TC for it. It may be handy to Hallvord.

It shows both bugs:

  • The whitespace issue;
  • The caret positioning issue. This one happens only when moving the caret to the left over the empty element. Moving to the right is ok.

Changed 10 years ago by fredck

TC. Changed it to make it easier to see the whitespace issue.

comment:3 follow-up: Changed 10 years ago by hallvord@…

I can no longer reproduce the "extra whitespace on multiple ctrl-b" issue with that test case but I can in the editor :(

I don't quite understand the code for inserting/removing styles. It is very complex. As far as I can see, on Ctrl-B FCKCoreStyleCommand.Execute runs but IsActive is only false on the first Ctrl-B. On all subsequent calls it is true, so new keypresses will try to remove the B tag again and again, and apparently fail to do so. RemoveFromRange is called, it creates a "bookmark" SPAN inside the B tag and moves it outside the B tag, then removes it again.

According to Firebug, the empty B tags are not removed in Firefox either, but the input cursor is placed outside the tag. Is this how it is meant to work?

Changed 10 years ago by hallvord@…

screen shot of Firebug DOM HTML view after several ctrl-b presses

comment:4 Changed 10 years ago by hallvord@…

  • Owner set to fredck

I have filed this as Opera bug 287177 but I think you may want to fix it by reviewing the logic for removing formatting because it doesn't look right to me.

comment:5 in reply to: ↑ 3 Changed 10 years ago by fredck

Replying to hallvord@opera.com:

I can no longer reproduce the "extra whitespace on multiple ctrl-b" issue with that test case but I can in the editor :(

The TC is not really related to multiple whitespaces and CTRL+B. Actually, you should not use CTRL+B on the TC, as it will be triggering Operas CTRL+B action. In the TC you can not only one whitespace added when loading the page.

I don't quite understand the code for inserting/removing styles. It is very complex.

Ohh yes it is. It gave me lots of sleepless nights :)

As far as I can see, on Ctrl-B FCKCoreStyleCommand.Execute runs but IsActive is only false on the first Ctrl-B. On all subsequent calls it is true, so new keypresses will try to remove the B tag again and again, and apparently fail to do so. RemoveFromRange is called, it creates a "bookmark" SPAN inside the B tag and moves it outside the B tag, then removes it again.

According to Firebug, the empty B tags are not removed in Firefox either, but the input cursor is placed outside the tag. Is this how it is meant to work?

You have spotted a bug introduced with [865], actually if you try it right before that changeset, you will see that the <b> will go away (sorry but Opera is still disable there).

I'll work to fix it now, so you can properly test it again soon.

comment:6 Changed 10 years ago by fredck

I've opened #1298 for the empty style removal issue.

comment:7 Changed 10 years ago by fredck

  • Priority changed from Normal to High

comment:8 Changed 10 years ago by fredck

  • Owner fredck deleted

comment:9 Changed 9 years ago by hallvord@…

  • Owner set to hallvord@…
  • Status changed from new to assigned

On Opera's side this is bug 287177. Following up on it now..

comment:10 Changed 9 years ago by hallvord@…

sadly still with us in build 9755

comment:11 Changed 9 years ago by hallvord@…

  • Resolution set to worksforme
  • Status changed from assigned to closed

This annoyance finally seems resolved and the fix (hopefully the final fix..the last of several attempts actually) is in builds - tested with internal build 9825.

comment:12 Changed 9 years ago by hallvord@…

I noticed another minor issue while testing this - filed as #1994

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