[FF] Multirange broken when command used several times

[Tobiasz Cudnik]

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)

[Frederico Caldeira Knabben]

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.

[Garry Yao]

This's related to #3361.

[Garry Yao]

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.

[Frederico Caldeira Knabben]

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.

[Frederico Caldeira Knabben]

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.

[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.

[Garry Yao]

I have no problem with both TCs after the patch.

[Martin Kou]

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.

[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.

[Frederico Caldeira Knabben]

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.

[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.

[Garry Yao]

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.

[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.

[Garry Yao]

Performance Test Case

[Frederico Caldeira Knabben]

Better to have this patch updated to the latest trunk.

[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.

[Piotrek Koszuliński]

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