Bug 149170 – Honor isolate bidi unicode codepoints

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Text

[Myles C. Maxfield]

Currently, our isolation logic only works on a RenderElement level. It should work inside a single RenderText too.

[Radar WebKit Bug Importer]

<rdar://problem/22704517>

[Myles C. Maxfield]

InlineBidiResolver::appendRun() will need to be fixed up, and potentially much more.

[Myles C. Maxfield]

Currently, BidiResolverBase::createBidiRunsForLine() has no concept of U_RIGHT_TO_LEFT_ISOLATE. Because it doesn't appear as a case in any switch statements, the default is getting hit, which means we seem to ignore it completely (for the purposes of bidi).

[Myles C. Maxfield]

These isolate characters don't force the complex text code path.

[Myles C. Maxfield]

Created attachment 281243 [details]
WIP

[Myles C. Maxfield]

Created attachment 281245 [details]
Example

[Myles C. Maxfield]

Created attachment 281247 [details]
Example 2

[Myles C. Maxfield]

Example 2 is explained by us not understanding how to deal with isolate characters inside BidiResolverBase::createBidiRunsForLine(). In particular, we have this general structure:

UCharDirection last = ...
while(true) {
    UCharDirection direction = u_charDirection(c);
    switch (direction) {
    case U_LEFT_TO_RIGHT:
        switch (last) {
        default:
            break;
        }
    case U_RIGHT_TO_LEFT:
        ...
    case U_WHITE_SPACE_NEUTRAL:
    default:
        break;
    }

    last = direction;
}

So, if the pattern of directions is:
U_RIGHT_TO_LEFT U_WHITE_SPACE_NEUTRAL U_FIRST_STRONG_ISOLATE U_LEFT_TO_RIGHT

When we hit the U_FIRST_STRONG_ISOLATE, we hit the outer default:. Then we set last = U_FIRST_STRONG_ISOLATE, and the next character is U_LEFT_TO_RIGHT, so we end up hitting the inner default:. So then we keep going, and never emit the run for the U_RIGHT_TO_LEFT parts.

Compare this to what you would get if you removed the ISOLATE characters. In that case, it would work correctly because the default:s wouldn't get hit.

[Myles C. Maxfield]

Created attachment 281249 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 281249 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281249&action=review

> Source/WTF/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=149170

<rdar://problem/26527378>

[Simon Fraser (smfr)]

Comment on attachment 281249 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281249&action=review

> Source/WTF/ChangeLog:3
> +        Honor unicode bidi unicode codepoints

One too many "unicode"s here?

> Source/WebCore/ChangeLog:12
> +        Because BidiResolver doesn't have any concept of isolate Unicode code points,
> +        its output is unexpected when they are present in content. Instead of this,
> +        we should consider the code points the same as whitespace for the purposes
> +        of the bidi algorithm. This is a stop-gap measure until we can support the
> +        codepoints fully in our Bidi algorithm.

This reads awkwardly.

"BidiResolver doesn't have any concept of isolate Unicode code points, so produces unexpected output when they are present". Fix by considering such code points as whitespace in the bidi algorithm. This is..."

You should reference a follow-up bug to fix this the correct way.

> LayoutTests/fast/text/isolate-ignore-expected.html:4
> +This test makes sure that isolate codepoints are ignored (for the time being).

"for the time being" is never a good thing to put into code or a test. It quickly becomes anachronistic.

[Myles C. Maxfield]

Committed r202057: <http://trac.webkit.org/changeset/202057>

[Ryan Haddad]

This change broke the Windows build:
<https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/78422>

[Ryan Haddad]

(In reply to comment #13)
> This change broke the Windows build:
> <https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/
> builds/78422>

The change was rolled out in <http://trac.webkit.org/changeset/202059>

[Darin Adler]

Can we add a regression test?

[Myles C. Maxfield]

This patch broke the Windows build because we're building against ICU 6.1 on Windows, which doesn't know what any of the U_*_ISOLATE constants are.

[Myles C. Maxfield]

It may be difficult to update https://developer.apple.com/opensource/internet/WebKitAuxiliaryLibrary.zip.

[Myles C. Maxfield]

(In reply to comment #16)
> This patch broke the Windows build because we're building against ICU 6.1 on
> Windows, which doesn't know what any of the U_*_ISOLATE constants are.

Even though the header file on Windows doesn't include the U_*_ISOLATE symbols, the library on Windows does correctly return the correct constants from u_charDirection().

[Myles C. Maxfield]

(In reply to comment #15)
> Can we add a regression test?

I thought the test I added (fast/text/isolate-ignore.html) is a regression test. Are you referring to something else?

[Myles C. Maxfield]

Created attachment 281320 [details]
Patch for committing

[WebKit Commit Bot]

Attachment 281320 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/text/BidiResolver.h:635:  U_FIRST_STRONG_ISOLATE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/text/BidiResolver.h:636:  U_LEFT_TO_RIGHT_ISOLATE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/text/BidiResolver.h:637:  U_RIGHT_TO_LEFT_ISOLATE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/text/BidiResolver.h:638:  U_POP_DIRECTIONAL_ISOLATE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 281320 [details]
Patch for committing

Clearing flags on attachment: 281320

Committed r202083: <http://trac.webkit.org/changeset/202083>

[Darin Adler]

(In reply to comment #19)
> I thought the test I added (fast/text/isolate-ignore.html) is a regression
> test.

Sorry, somehow I missed it!

Can you retitle this bug so that it’s clear that it’s about isolate codepoints? Embed and override codepoints had been working since KHTML.

[Myles C. Maxfield]

*** This bug has been marked as a duplicate of bug 178960 ***