Bug 49295 – REGRESSION (r71566): r71566 breaks bidi-control-chars-treated-as-ZWS.html

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

[Xiaomei Ji]

r71566 breaks bidi-control-chars-treated-as-ZWS.html (in ATSUI handling).
"è" is displayed as empty square.

I disabled the test temporarily while investigation.

The problem appears to be that the indices reported in m_atsuiIndices no longer correspond to the original string.

Created attachment 73457 [details]
Use the correct offset between indices in m_characters and indices in the text passed to ATSUI

[Xiaomei Ji]

Hi Dan,

Thanks very much for solving the problem! Appreciated!

I tried this afternoon and noticed that
at some point, the m_atsuIndices[0] is 0, which points to \x202d;
but m_atsuIndices[1] is 2, which points to \x0300.

So, in ComplexTextController::adjustGlyphsAndAdvances(),
'ch' points to \x202d and 'nextch' points to \x0300. I guess that is the reason it printed as empty square.

But I could not figure out why m_atsuIndices is assigned wrongly at ComplexTextControllerATSUI.cpp:Line 80.

Now, I understand.

Since I already disabled the test in mac-leopard/Skipped, maybe you could enable that in the patch?

Thanks!
Xiaomei

[Dave Hyatt]

Comment on attachment 73457 [details]
Use the correct offset between indices in m_characters and indices in the text passed to ATSUI

r=me

Fixed in <http://trac.webkit.org/projects/webkit/changeset/71763>.

[WebKit Review Bot]

http://trac.webkit.org/changeset/71763 might have broken Leopard Intel Release (Tests)
The following tests are not passing:
fast/text/international/bidi-control-chars-treated-as-ZWS.html
fast/text/international/bidi-neutral-run.html

(In reply to comment #6)
> http://trac.webkit.org/changeset/71763 might have broken Leopard Intel Release (Tests)
> The following tests are not passing:
> fast/text/international/bidi-control-chars-treated-as-ZWS.html

Looks like the change had no effect here.

> fast/text/international/bidi-neutral-run.html

This had incorrect results checked in in r71706.

[Xiaomei Ji]

(In reply to comment #7)
> (In reply to comment #6)
> > http://trac.webkit.org/changeset/71763 might have broken Leopard Intel Release (Tests)
> > The following tests are not passing:
> > fast/text/international/bidi-control-chars-treated-as-ZWS.html
> 
> Looks like the change had no effect here.

I applied the change locally and run layout test and pixel test this morning.  And it passed.
Do not know why the width of ""\x{202D}e\x{300}"" increased here.

> 
> > fast/text/international/bidi-neutral-run.html
> 
> This had incorrect results checked in in r71706.

why is it wrong?

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > http://trac.webkit.org/changeset/71763 might have broken Leopard Intel Release (Tests)
> > > The following tests are not passing:
> > > fast/text/international/bidi-control-chars-treated-as-ZWS.html
> > 
> > Looks like the change had no effect here.
> 
> I applied the change locally and run layout test and pixel test this morning.  And it passed.
> Do not know why the width of ""\x{202D}e\x{300}"" increased here.
> 
> > 
> > > fast/text/international/bidi-neutral-run.html
> > 
> > This had incorrect results checked in in r71706.
> 
> why is it wrong?

If you look at the rendering before and after your change you can see that in some lines some characters (such parentheses) are missing, as a result of the index mismatch.

Keeping open since the fix was incomplete.

Looking at this some more, I think the reasons that control characters were elided no longer apply, since ComplexTextController omits the resulting glyphs if necessary, so really none of the changes were necessary.

Created attachment 73559 [details]
Revert unnecessary changes to ComplexTextControllerATSUI

r71787