Opened 7 years ago

Closed 7 years ago

#3868 closed Bug (fixed)

[chrome] SCAYT toolbar options are in reversed order

Reported by: tobiasz.cudnik Owned by: tobiasz.cudnik
Priority: Normal Milestone: CKEditor 3.0
Component: General Version:
Keywords: Confirmed V8 Review+ Cc:

Description

SCAYT toolbar options are in reversed order. Whats interesting, other menus aren't. On Safari everything works fine.

Screenshot attached.

Attachments (7)

2009-07-01-090620_195x216_scrot.png (6.1 KB) - added by tobiasz.cudnik 7 years ago.
3868.patch (1.7 KB) - added by tobiasz.cudnik 7 years ago.
2009-07-02-115240_1410x1080_scrot.png (113.4 KB) - added by tobiasz.cudnik 7 years ago.
2009-07-02-115227_1410x1080_scrot.png (210.4 KB) - added by tobiasz.cudnik 7 years ago.
2009-07-02-115235_1410x1080_scrot.png (167.4 KB) - added by tobiasz.cudnik 7 years ago.
3868_2.patch (1.8 KB) - added by tobiasz.cudnik 7 years ago.
3868_3.patch (1.2 KB) - added by tobiasz.cudnik 7 years ago.

Download all attachments as: .zip

Change History (17)

Changed 7 years ago by tobiasz.cudnik

comment:1 Changed 7 years ago by tobiasz.cudnik

  • Owner set to tobiasz.cudnik
  • Status changed from new to assigned

Changed 7 years ago by tobiasz.cudnik

comment:2 Changed 7 years ago by tobiasz.cudnik

  • Keywords V8 Review? added

As it turns out Chrome differently interprets Array#sort "no-change" result (which is supposed to be 0). Changing it to -1 resolves the problem.

It should work for all other browsers, but i've limited this change to Chrome-only, which required new property in CKEDITOR.env.js.

comment:3 Changed 7 years ago by tobiasz.cudnik

  • Keywords Review- added; Review? removed

There are still issues with newest Chrome 3 and Safari 3. I will post next patch after more extensive tests.

Changed 7 years ago by tobiasz.cudnik

Changed 7 years ago by tobiasz.cudnik

Changed 7 years ago by tobiasz.cudnik

comment:4 Changed 7 years ago by tobiasz.cudnik

Affected are:

  • Chrome 3
  • Chrome 2
  • Safari 3
  • FF 2 (in other way)

Attaching screenshots of all.

comment:5 Changed 7 years ago by tobiasz.cudnik

  • Keywords Review? added; Review- removed

I've decided to implement sorting manually. Thanks to that it works same way in all browsers. I've used bubble sort, because of low amount of data which going to be sorted.

This solution doesn't needed new CKEDITOR.env property.

Changed 7 years ago by tobiasz.cudnik

comment:6 Changed 7 years ago by alfonsoml

The reason of this problem is that for Chrome the sort function is not stable. That means that when two elements compare as equal in a custom sort, as it does happen in this situation, the behavior is undefined. It doesn't have to keep the original order.

They explain it briefly in http://code.google.com/p/v8/issues/detail?id=90 which points to https://bugzilla.mozilla.org/show_bug.cgi?id=224128 that was checked for Firefox 3, so that explains also the difference with Firefox 2.

So that means that it's not a browser bug, just a difference of opinions; it can't be detected, as the behavior is undefined, the test might say that it has been sorted correctly, but it was just a lucky situation; and the solutions are to add a third key to provide the default sort or use a custom function like patch 2.

A simple script to test it:

function dump(data)
{
	document.write("<p>Contents of the array: ")
	for(var i=0; i<data.length; i++)
	{
		document.write(data[i].name);
		if (i<(data.length-1)) document.write(", ");
	}
	document.write("</p>");
}
var simpleArray = [ {name:'enable (1)'}, {name:'options (2)'}, {name:'languages (3)'}, {name:'about (4)'}]
	dump(simpleArray);
		simpleArray.sort( function( itemA, itemB )
			{
				return 0; // No sort
			});
	dump(simpleArray);

comment:7 Changed 7 years ago by garry.yao

It seems for me the menu item array is immutable, if that's true, we could easily alter the CKEDITOR.menu::add function on L107 to build a second column for used as the secondary comparison key:

add : function( item )
{
	// Later we may sort the items, but Array#sort is not stable in
	// some browsers, here we're forcing the original sequence with
	// 'order' attribute if it hasn't been assigned.
	if ( !item.order )
		item.order = this.items.length;
	this.items.push( item );
},

Changed 7 years ago by tobiasz.cudnik

comment:8 Changed 7 years ago by tobiasz.cudnik

Garry's solution is best way in my opinion, so i've made a patch with it, after checking in all browsers.

comment:9 Changed 7 years ago by garry.yao

  • Keywords Review+ added; Review? removed

comment:10 Changed 7 years ago by tobiasz.cudnik

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

Fixed with [3824].

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