Ticket #1271 (closed Bug: worksforme)

Opened 7 years ago

Last modified 6 years ago

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

Reported by: fredck Owned by: hallvord@…
Priority: High 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

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

Change History

comment:1 Changed 7 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 7 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 7 years ago by fredck

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

comment:3 follow-up: ↓ 5 Changed 7 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 7 years ago by hallvord@…

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

comment:4 Changed 7 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 7 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 7 years ago by fredck

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

comment:7 Changed 7 years ago by fredck

  • Priority changed from Normal to High

comment:8 Changed 7 years ago by fredck

  • Owner fredck deleted

comment:9 Changed 7 years ago by hallvord@…

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

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

comment:10 Changed 6 years ago by hallvord@…

sadly still with us in build 9755

comment:11 Changed 6 years ago by hallvord@…

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

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 6 years ago by hallvord@…

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

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