1281988 - The bidi engine resolves levels incorrectly when there are continuous formatting characters

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

See bug 1221034 comment 2 for the description of the issue.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1281988 - Skip BN characters when resolving implicit levels in bidi engine.

Review commit: https://reviewboard.mozilla.org/r/60490/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60490/

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

The code changes in this patch are from the ICU code at the following places correspondingly:
https://dxr.mozilla.org/mozilla-central/rev/d1102663db10b3d4b9358f3cf4e16b7c56902352/intl/icu/source/common/ubidi.c#2752-2754
https://dxr.mozilla.org/mozilla-central/rev/d1102663db10b3d4b9358f3cf4e16b7c56902352/intl/icu/source/common/ubidi.c#2161-2163
https://dxr.mozilla.org/mozilla-central/rev/d1102663db10b3d4b9358f3cf4e16b7c56902352/intl/icu/source/common/ubidi.c#2243-2244

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

With this and bug 1160847, we pass all unicode-bidi tests in the csswg repo.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c87e83b69d9d
I accdentally triggered reftests for all platforms...

Comment 5

[Jonathan Kew [:jfkthame]]

Comment on attachment 8764807 [details]
Bug 1281988 - Skip BN characters when resolving implicit levels in bidi engine.

https://reviewboard.mozilla.org/r/60490/#review57672

r=me, but please make the loops a little less cryptic so that future readers don't worry they're a mistake. :)

::: layout/base/nsBidi.cpp:1526
(Diff revision 1)
>  
>    for (i = aStart; i <= aLimit; i++) {
>      if (i >= aLimit) {
> -      if (aLimit > aStart) {
> -        dirProp = mDirProps[aLimit - 1];
> +      int32_t k;
> +      for (k = aLimit - 1;
> +           k > aStart && (DIRPROP_FLAG(dirProps[k]) & MASK_BN_EXPLICIT); k--);

Please move the semicolon that forms the (empty) loop body here to a separate line, perhaps with a comment to help readability, e.g.:

    /\* empty loop body \*/ ;

Actually, I suppose it's arguable that Gecko style would expect braces around the body, even if it's empty. So maybe:

    for (...) {
      // empty loop body
    }

is stylistically more consistent.

::: layout/base/nsBidi.cpp:1573
(Diff revision 1)
>        }
>      }
>    }
>  
> -  dirProp = dirProps[aLimit - 1];
> +  for (i = aLimit - 1;
> +       i > aStart && (DIRPROP_FLAG(dirProps[i]) & MASK_BN_EXPLICIT); i--);

As above.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

https://reviewboard.mozilla.org/r/60490/#review57672

> Please move the semicolon that forms the (empty) loop body here to a separate line, perhaps with a comment to help readability, e.g.:
> 
>     /\* empty loop body \*/ ;
> 
> Actually, I suppose it's arguable that Gecko style would expect braces around the body, even if it's empty. So maybe:
> 
>     for (...) {
>       // empty loop body
>     }
> 
> is stylistically more consistent.

Actually I intentionally did so to make it look more similiar to the corresponding ICU code. It seems to me the whole nsBidi doesn't really follow the Gecko style, which made me think that there is some other rules for this file. But I'm fine to improve the readability.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/23a3e2678c02
Skip BN characters when resolving implicit levels in bidi engine. r=jfkthame

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/23a3e2678c02