Bug 3725 – REGRESSION in bidi.cpp r1.127: embedding levels of R EN in L context

• Status: CLOSED
• Resolution: FIXED
• Component: Layout and Rendering

In a R EN L sequence of runs, the EN should have the highest embedding level. As of bidi.cpp r1.127, if the 
L is not explicit but only implicit from context, the EN ends up with the lower embedding level, and the 
layout order is wrong.

Expected layout for the testcase is the digit 1 followed by Hebrew letter Aleph in all 3 cases (followed by 
nothing, a full stop or the letter Z). Firefox renders as expected.

Actual layout with TOT is Hebrew letter Aleph followed by 1 (followed by nothing or a full stop) in the first 
two cases, and as expected in the last case.

Created attachment 2654 [details]
Testcase

Created attachment 2657 [details]
A patch

This fixes the regression. The patch makes 14 of the layout tests fail, because
it splits a few runs, but they render the same with it. Still, it may be
possible to avoid this. There's also room for improvement in moving out stuff
that is repeated in both "if" and "else" cases (left that way for the sake of
explicitness while I'm working on it), so I'm not even submitting it for review
at this stage.

[Joost de Valk (AlthA)]

confirmed regression, moving to p1.

(In reply to comment #3)
> confirmed regression, moving to p1.

Hm... I don't know how this priority thing works (or how anything works for that matter), so I'll just state 
that

1. If the choice is between r1.127 and r1.126, then despite the regression, r1.127 is "less buggy", i.e. the 
regression is far less likely to occur in everyday use than the problem r1.127 fixes.
2. I don't think that those 14 layout tests failing is significant.
3. There are a few other bidi layout issues in TOT (which are not regressions). I'm working on identifying 
and fixing them. I'll submit a bug about them when I give up on fixing them all or think that I've done it.

attachment 2657 [details] also breaks this:
<p dir="rtl">
for 123 abc
</p>

Don't bother looking at it.

Created attachment 2659 [details]
A better patch

This patch corrects the problem mentioned in comment #5.
It also fixes the layout of <p dir="rtl">&#x05d0;1.</p> and of the following,
which were broken even before r1.127:
<p dir="rtl">123a &#x05d0;&#x05d1;&#x05d2;</p>
<p dir="rtl">123a+ &#x05d0;&#x05d1;&#x05d2;</p>
<p dir="rtl">&#x05d0;&#x05d1;&#x05d2; 1+5.2x, &#x05d3;&#x05d4;&#x05d5;</p>

This patch changes the layout of <p dir="rtl">def &#x0661;&#x0662;&#x0663;
abc</p>, which was broken anyway. I don't know if it's more severely broken
now. Other cases involving Arabic numbers are broken just the same with or
without this patch.

Finally, this patch fails 137(!) layout tests, because now instead of splitting
a few runs, it merges many runs. I haven't gone through all 137 of them yet,
but so far it seems that the failures are only a result of run merges (and
still a few run splits). These do not affect the layout but sometimes slightly
affect rendering (due to rounding and such, I presume). In general, I suppose
long runs are better, but it's just a daunting task to verify that all 137
failures are benign. Otherwise, the part that's responsible for them can be
left out (it's the different handling of the L after EN case), and then there
are far fewer failures, all of which are splits.

Attachment 2668 [details] to bug 3733 addresses this bug.

Fixed along with bug 3733.