120114 - support surrogate in ResolveBackwards()

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Shanjian Li]

This is a leftover of bug 118000. I did not modify function ResolveBackwards
to support surrogates pair. It should be fixed once 118000 get in.

Comment 1

[Shanjian Li]

Attachment: patch

Comment 2

[Shanjian Li]

simon, could you review my patch? Logic is very similar to ResolveForward.

Comment 3

[Simon Montagu :smontagu]

With this patch the test case at
http://www.geocities.com/i18nguy/unicode-example-plane1.html still doesn't
display correctly on my system. The Etruscan characters are all displayed as ?????

Comment 4

[Shanjian Li]

simon, 
I only want to address ResolveBackwards in this bug. I set a breakpoint at 
this function and visited that page, it never stopped there. (Did you once 
mentioned that there are more than one options to do right-to-left rendering?)

I guess the problem is because we did not group surrogate pair together when 
rendering those characters. I filed a new bug 122800 and assign it to you.

Comment 5

[Simon Montagu :smontagu]

On second thoughts, maybe either this patch is unnecessary, or else there is a
problem with the logic that calls ResolveForwards or ResolveBackwards. We call
ResolveBackwards when mRightToLeftText is true, and mRightToLeftText is set when
we display right-to-left characters on a platform that does its own bidi
reordering, i.e. when we are not reversing the text ourselves but passing it in
logical order to the platform rendering APIs. Was that the intention?

There are no plane 1 or higher characters with right-to-left directionality, so
until that changes, ResolveBackwards doesn't need to know how to handle them.

Comment 6

[Shanjian Li]

Do we only use ResolveBackwards with characters with right-to-left directionality?
How about those Etruscan characters after they do get into plane 1? If you are sure 
about the answers, we can future this. Otherwise, we might want to just check it in. 
There is not significant negative impact after all. 

Comment 7

[Simon Montagu :smontagu]

Only *if* the Bidi category of the Etruscan characters in UCDB changes from L to
R (which seems possible, if it is correct that Etruscan was historically written
from right to left), and *if* operating systems start honoring this (which seems
rather unlikely), will we need to treat them in the same way as other
right-to-left scripts like Arabic and Hebrew.

I think futuring this is the way to go.

Comment 8

[Shanjian Li]

Give this to simon, who know when this is needed if it is ever needed at all. 

Comment 9

[Simon Montagu :smontagu]

It's time to fix this: there are now RTL characters in plane 1 (Cypriot
syllabary) and at least one font is available for them at
http://home.att.net/~jameskass/code2001.htm

Comment 10

[Simon Montagu :smontagu]

Attachment: w-i-p patch

This brings the previous patch in line with the current version of
ResolveForwards and adds proper surrogate handling in
nsBidiPresUtils::CalculateCharType. It still needs cleaning up and porting to
other platforms that implement ResolveBackwards.

Comment 11

[Simon Montagu :smontagu]

Attachment: Patch ready for testing

I patched nsFontMetricsOS2FT::ResolveBackwards as well (for consistency with
ResolveForwards, even though OS/2 doesn't display non-plane 0 characters).

Comment 12

[Simon Montagu :smontagu]

Comment on attachment 169903 [details] [diff] [review]
Patch ready for testing

mkaply told me on IRC that the patch builds on OS/2 and doesn't break anything.

Comment 13

[rbs]

Comment on attachment 169903 [details] [diff] [review]
Patch ready for testing

r+sr=rbs

Comment 14

[Simon Montagu :smontagu]

Patch checked in.