Opened 16 years ago

Closed 16 years ago

#3231 closed Bug (fixed)

Unable to apply inline styles when selection touches end of area with the same style already applied.

Reported by: Martin Kou Owned by: Garry Yao
Priority: Must have (possibly next milestone) Milestone: CKEditor 3.0
Component: Core : Styles Version: SVN (CKEditor) - OLD
Keywords: Oracle Review+ Cc: Senthil

Description

To reproduce:

  1. Open replacebyclass.html in Windows Firefox.
  2. Double click on the first word "This ".
  3. Click the Bold button - the text is bolded.
  4. Double click on the second word "is ".
  5. Click the Bold button again.
  6. Nothing happens.

Attachments (3)

3231.patch (1.8 KB) - added by Garry Yao 16 years ago.
3231_2.patch (6.8 KB) - added by Garry Yao 16 years ago.
3231_3.patch (3.1 KB) - added by Garry Yao 16 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 16 years ago by Garry Yao

Owner: set to Garry Yao
Status: newassigned

comment:2 in reply to:  description Changed 16 years ago by Garry Yao

Resolution: duplicate
Status: assignedclosed

Replying to martinkou:

  1. Double click on the second word "is ".

After this step, Firefox NativeRange is weirdly report the selection as:

<p>
	<strong>This ^</strong>is^ some ....

which confuse our selection checking system which just check the first element path.
This's been already reported at #2767.

comment:3 Changed 16 years ago by Frederico Caldeira Knabben

Resolution: duplicate
Status: closedreopened

While this ticket is related to #2767, it's not a duplicate of it. Here we're talking about selections that touch the ending boundary of an element. A solution could be found in this case, while #2767 will much probably remain opened for a while.

Changed 16 years ago by Garry Yao

Attachment: 3231.patch added

comment:4 Changed 16 years ago by Garry Yao

Keywords: Review? added

The proposed path now checking the range root node instead of the first node for disambiguition explained here.

comment:5 Changed 16 years ago by Garry Yao

Priority: NormalHigh

comment:6 Changed 16 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

i think the right fix here would have getStartElement returning the proper element in this situations.

After a chat with Garry, it looks like the correct solution would have this range...

<p>
	<strong>This ^</strong>is^ some ....

... getting fixed to this...

<p>
	<strong>This </strong>^is^ some ....

... in this case before trying to detect the start element (for non collapsed selections only).

Changed 16 years ago by Garry Yao

Attachment: 3231_2.patch added

comment:7 Changed 16 years ago by Garry Yao

Keywords: Review? added; Review- removed

The new patch is proposing the following changes:

  1. Introduce a new method of 'shrink' as well as related TC, which helps to deal with the above situation;
  2. Moving 'getChild' and 'getChildCount' from node.js into element.js where they should belonged to.

comment:8 Changed 16 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

The solution is good, and it works well.

I just see one problem here. You are introducing a new public function to the API (range::shrink), but that function is not complete because it doesn't consider two features that we don't need to care about for now:

  • It acts only in the start boundary (as specified in the comments).
  • It should have no effect in a collapsed range.

As we don't need the above features, it makes less sense placing them into the API just to make the shrink function complete. So I think it's better to simply move the code into line 626 at the selection plugin (adding a comment about it, pointing to this ticket).

Also, some minor enhancements to the code:

  • Instead of "getText().length" you can use "getLength()".
  • "this.startContainer" has been used four times in that code block. It would be better to have a local variable to point to it.

Changed 16 years ago by Garry Yao

Attachment: 3231_3.patch added

comment:9 Changed 16 years ago by Garry Yao

Keywords: Review? added; Review- removed

comment:10 Changed 16 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

Instead of doing do{}while(true), you can can go simpler with while(true){} . Please fix it before committing.

comment:11 Changed 16 years ago by Garry Yao

Resolution: fixed
Status: reopenedclosed

Fixed with [3456]. Click here for more info about our SVN system.

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy