Bug 16077 - XSLT processor <xsl:sort> algorithm is incompatible with other browser implementations
Summary: XSLT processor <xsl:sort> algorithm is incompatible with other browser implem...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords: HasReduction, InRadar, YahooBug
Depends on:
Blocks:
 
Reported: 2007-11-20 18:03 PST by darren
Modified: 2007-11-23 00:30 PST (History)
1 user (show)

See Also:


Attachments
XML file to Sort (883 bytes, application/xml)
2007-11-20 18:59 PST, darren
no flags Details
XSL file to do sorting transformation (736 bytes, text/xml)
2007-11-20 18:59 PST, darren
no flags Details
proposed fix (109.23 KB, patch)
2007-11-21 10:41 PST, Alexey Proskuryakov
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description darren 2007-11-20 18:03:08 PST
When sorting an XML dom using an XSL transformation  (via the <xsl:sort>)  the algorithm doesn't appear to be to spec:

http://www.w3.org/TR/xslt#sorting

from the spec:
case-order has the value upper-first or lower-first; this applies when data-type="text", and specifies that upper-case letters should sort before lower-case letters or vice-versa respectively. For example, if lang="en", then A a B b are sorted with case-order="upper-first" and a A b B are sorted with case-order="lower-first". The default value is language dependent.
....

Safari appears to be sorting a list with upper and lower case entries in it like this

  A B a b

rather than like this

 A a B b  *or* a A b B (depending on the value of the case-order attribute).

For a test case, download the XML and XSL files on this page:
   http://www.w3schools.com/xsl/xsl_sort.asp

... then edit the entries to include an artist with all lower-case chars.  Display this in Safari,
FF, and IE to see the difference.
Comment 1 Maciej Stachowiak 2007-11-20 18:42:54 PST
A simple standalone reduction based on that example would be helpful.
Comment 2 darren 2007-11-20 18:59:00 PST
Created attachment 17423 [details]
XML file to Sort

XML file to sort
Comment 3 darren 2007-11-20 18:59:46 PST
Created attachment 17424 [details]
XSL file to do sorting transformation

XSL file to do sorting transformation.
Comment 4 darren 2007-11-20 19:03:23 PST
To Repro:

1. Download the XML and XSL files attached.
2. Load them In the browser:

Result:
sorted as:
  Angie
  Brad
  abby
  brent

Expected:
sorted as:
  abby
  Angie
  Brad
  brent
Comment 5 Alexey Proskuryakov 2007-11-20 23:35:08 PST
I have searched libxslt bugs, and it appears to be a well known issue that they refuse to fix: <http://bugzilla.gnome.org/show_bug.cgi?id=143383>.

It's possible to override the broken default implementation with xsltSetCtxtSortFunc().
Comment 6 Maciej Stachowiak 2007-11-21 01:27:26 PST
<rdar://problem/5609785>
Comment 7 Alexey Proskuryakov 2007-11-21 10:41:34 PST
Created attachment 17433 [details]
proposed fix
Comment 8 Eric Seidel (no email) 2007-11-21 11:34:57 PST
Comment on attachment 17433 [details]
proposed fix

I would think that xml/XSLTUnicodeSort.cpp should be called xml/XSLTUnicodeSortICU.cpp, or at least the ICU parts abstracted out into another file.  Otherwise we're just going to get lots of #ifdefs when Qt comes and adds their implementation, etc.

Otherwise the change looks fine.
Comment 9 Alexey Proskuryakov 2007-11-21 12:00:57 PST
I agree that we will eventually need to abstract out collation from here - but I feel that it's something that needs to settle down a bit. Immediately introducing abstractions when adding a new concept sometimes gives poor results due to incomplete understanding of needs.
Comment 10 Maciej Stachowiak 2007-11-22 22:48:31 PST
Comment on attachment 17433 [details]
proposed fix

It's unfortunate that we have to cut & paste so much XSLT code, but I guess there's no better way. Too bad you can't just provide a string compare function for it to use.

r=me
Comment 11 Alexey Proskuryakov 2007-11-23 00:30:36 PST
Committed revision 27984.