Ticket #3475 (closed Bug: fixed)

Opened 5 years ago

Last modified 11 months ago

[FF] Multirange broken when command used several times

Reported by: tobiasz.cudnik Owned by: garry.yao
Priority: Normal Milestone:
Component: General Version: SVN (CKEditor) - OLD
Keywords: Firefox Confirmed Cc:

Description (last modified by tobiasz.cudnik) (diff)

Multirange broken when command used several times.

Reproduction steps:

  1. Use bold on 2 separate chunks of text.
  2. Select part of 1st and part of 2nd chunk.
    <strong>^fo^o </strong>bar<strong> f^oo^2</strong>
    
  3. Click "bold" button several times.

Depending on first selection range start, there are 2 possible misbehaviors:

  1. Second selection will jump from proper selection to rest of previously bolded text (of 2nd chunk).
  2. Second (or both) selections will disappear after second or third use of bold command, but still they will make some DOM modifications.

35sec screencast presenting this issue: http://pub.meta20.net/fck-multirange.ogg

Env: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.9) Gecko/2009042219 Iceweasel/3.0.9 (Debian-3.0.9-1)

Attachments

3475.patch (1.1 KB) - added by garry.yao 5 years ago.
3475_2.patch (3.4 KB) - added by garry.yao 5 years ago.
3475_3.patch (7.9 KB) - added by garry.yao 5 years ago.
3475_4.patch (9.0 KB) - added by garry.yao 5 years ago.
3475_5.patch (9.4 KB) - added by garry.yao 5 years ago.
3475_6.patch (35.6 KB) - added by garry.yao 5 years ago.
3475_7.patch (35.8 KB) - added by garry.yao 5 years ago.
test-class-creation.html (1.7 KB) - added by garry.yao 5 years ago.
Performance Test Case

Change History

comment:1 Changed 5 years ago by tobiasz.cudnik

  • Description modified (diff)

comment:2 follow-up: ↓ 4 Changed 5 years ago by fredck

  • Keywords Confirmed added
  • Milestone set to CKEditor 3.0

I think it's a problem with the order the ranges are being handled. If we are first changing the first range, it may happen that the offset values for the second range will not anymore be valid, due to text nodes being broken, insertion of elements, etc. If that's true, we need to invert the order of processing them, from last to first.

It's just an intuition though, still to be checked.

comment:3 Changed 5 years ago by garry.yao

  • Status changed from new to assigned
  • Owner set to garry.yao

This's related to #3361.

Changed 5 years ago by garry.yao

comment:4 in reply to: ↑ 2 Changed 5 years ago by garry.yao

  • Keywords Review? added

Replying to fredck:

I think it's a problem with the order the ranges are being handled.

I confess it's would be a practical way to 'step off' the problem, since it's only happened on browser which support multiple ranges, we could temporarily use this fix, exactly like what we deal with the insertElement function on multi-ranges.
While a more sufficient fix would be pooling all the activated ranges and update them manually, it would be very cost.

comment:5 Changed 5 years ago by fredck

  • Keywords Review- added; Review? removed

At this point, all bookmarks created in the code executed by applyToRange and removeFromRange should not happen. It also means that other parts of the code using those functions need to consider this, and create bookmarks before calling these functions.

Changed 5 years ago by garry.yao

comment:6 Changed 5 years ago by garry.yao

  • Keywords Review? added; Review- removed

comment:7 follow-up: ↓ 8 Changed 5 years ago by fredck

  • Keywords Review- added; Review? removed

Trying the same thing with the following initial selection instead:

^fo^o <strong>bar</strong> f^oo^2

It works before the patch, but it's broken after it.

comment:8 in reply to: ↑ 7 Changed 5 years ago by garry.yao

Replying to fredck:

Trying the same thing with the following initial selection instead:...

When working on #3256, I found the similar problem, multiple ranges mangling is not that simple as my original thinking and need further fixes.

Changed 5 years ago by garry.yao

comment:9 Changed 5 years ago by garry.yao

  • Keywords Review? added; Review- removed

I have no problem with both TCs after the patch.

comment:10 Changed 5 years ago by martinkou

  • Keywords Review- added; Review? removed

There're still some trick cases where the patch fails.

e.g.

  1. Open replacebyclass.html in Firefox.
  2. Select as follows:
    <p>
    	This is so^me <strong>sa^mple te^xt</strong>. Yo^u are using <a href="http://www.fckeditor.net/">CKEditor</a>.</p>
    
  3. Press "Bold" twice.
  4. The selection is wrong.

Pressing Italics twice would give the correct result here - apparently we need to account for the tag merging logic in the style plugin.

comment:11 Changed 5 years ago by garry.yao

Nice catch Martin, now our theory of 'processing ranges in reverse order' doesn't any more fit this case, so I'm proposing a new way to resist dirty ranges.

Changed 5 years ago by garry.yao

comment:12 Changed 5 years ago by garry.yao

  • Keywords Review? added; Review- removed

comment:13 Changed 5 years ago by fredck

  • Keywords Review- added; Review? removed

Part of this patch is being fixed at #3256, so I would wait for that one to get committed first, and them propose a new patch here.

Changed 5 years ago by garry.yao

comment:14 Changed 5 years ago by garry.yao

  • Keywords Review? added; Review- removed

comment:15 Changed 5 years ago by garry.yao

Updates contain in this patch:

  1. Introducing a selection iterator to batch process multiple ranges to make sure during processing, each range doesn't mangle the following ones.
  2. Fixing a regression logic error of #3256 when creating bookmarks, check the TC test_createBookmarks_3475 in selection.html.

comment:16 Changed 5 years ago by garry.yao

  • Keywords RC Review- added; Review? removed

After a peer review with Fred, the following updates should be considered:

  1. Introduce a dedicated class for represent range list, which encapsulate necessary range manipulation (e.g. bookmark creation) operation and also implementing the iterator interface;
  2. Bookmark creation logic regard dirty range update could be further simplified into a inline template function.

Changed 5 years ago by garry.yao

comment:17 Changed 5 years ago by garry.yao

  • Keywords Review? added; Review- removed

comment:18 Changed 5 years ago by fredck

  • Keywords RC removed

comment:19 Changed 5 years ago by garry.yao

rangelist class is in a module pattern, while the encapsulation is perfect, but it's performance con is reveal by the attached deduced TC test-class-creation.html, where it's amazingly to see that in v8 with some inline caching VM level optimization, the difference is even as huge as 10x.

Based on the PF testing result, considering the fact that the rangelist is one of the frequently constructed object out there, I would revert it to a plain old prototype based form with the new patch with other changes remained.

Changed 5 years ago by garry.yao

Changed 5 years ago by garry.yao

Performance Test Case

comment:20 Changed 5 years ago by fredck

  • Milestone changed from CKEditor 3.0 to CKEditor 3.1

comment:21 Changed 5 years ago by garry.yao

  • Owner garry.yao deleted
  • Status changed from assigned to new

comment:22 Changed 5 years ago by garry.yao

  • Status changed from new to assigned
  • Owner set to garry.yao

comment:23 Changed 5 years ago by fredck

  • Milestone changed from CKEditor 3.1 to CKEditor 3.2

Better to have this patch updated to the latest trunk.

comment:24 Changed 5 years ago by fredck

  • Keywords Review- added; Review? removed

comment:25 Changed 5 years ago by fredck

  • Milestone changed from CKEditor 3.2 to CKEditor 3.3

comment:26 Changed 4 years ago by fredck

  • Milestone changed from CKEditor 3.3 to CKEditor 3.4

comment:27 Changed 4 years ago by garry.yao

This's becoming more of a refactoring task than a bug fixing, if we decide to have it in this milestone, we need to do it in ASAP manner.

comment:28 Changed 4 years ago by fredck

  • Milestone CKEditor 3.4 deleted

comment:29 Changed 11 months ago by Reinmar

  • Status changed from review_failed to closed
  • Resolution set to fixed

Fixed. And what's more Mozilla is planning to drop multi-range selections support.

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