Opened 15 years ago

Closed 15 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 15 years ago.
3868.patch (1.7 KB) - added by Tobiasz Cudnik 15 years ago.
2009-07-02-115240_1410x1080_scrot.png (113.4 KB) - added by Tobiasz Cudnik 15 years ago.
2009-07-02-115227_1410x1080_scrot.png (210.4 KB) - added by Tobiasz Cudnik 15 years ago.
2009-07-02-115235_1410x1080_scrot.png (167.4 KB) - added by Tobiasz Cudnik 15 years ago.
3868_2.patch (1.8 KB) - added by Tobiasz Cudnik 15 years ago.
3868_3.patch (1.2 KB) - added by Tobiasz Cudnik 15 years ago.

Download all attachments as: .zip

Change History (17)

Changed 15 years ago by Tobiasz Cudnik

comment:1 Changed 15 years ago by Tobiasz Cudnik

Owner: set to Tobiasz Cudnik
Status: newassigned

Changed 15 years ago by Tobiasz Cudnik

Attachment: 3868.patch added

comment:2 Changed 15 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 15 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 15 years ago by Tobiasz Cudnik

Changed 15 years ago by Tobiasz Cudnik

Changed 15 years ago by Tobiasz Cudnik

comment:4 Changed 15 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 15 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 15 years ago by Tobiasz Cudnik

Attachment: 3868_2.patch added

comment:6 Changed 15 years ago by Alfonso Martínez de Lizarrondo

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 15 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 15 years ago by Tobiasz Cudnik

Attachment: 3868_3.patch added

comment:8 Changed 15 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 15 years ago by Garry Yao

Keywords: Review+ added; Review? removed

comment:10 Changed 15 years ago by Tobiasz Cudnik

Resolution: fixed
Status: assignedclosed

Fixed with [3824].

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