Bug 83826 – REGRESSION (r114032): Layout Tests fast/text/drawBidiText.html fast/text/international/bidi-listbox-atsui.html are failing

• Component: Text
• Status: RESOLVED FIXED

[Vincent Scheib]

http://trac.webkit.org/changeset/114032
This patch modifies RenderText so that it scans all of its characters up front...

Caused these Layout Tests 
fast/text/drawBidiText.html 
fast/text/international/bidi-listbox-atsui.html are failing

to start having pixel errors, visible here:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Ftext%2FdrawBidiText.html%20fast%2Ftext%2Finternational%2Fbidi-listbox-atsui.html

I can not tell if this is acceptable and should be rebaselined, or represents an unintended result.

[Levi Weintraub]

The arabic is no longer being combined, but instead each character is being rendered individually. That's definitely wrong :(

[Levi Weintraub]

Actually, likewise with the Hebrew. There are dots that are supposed to modify the adjacent character, but the extra spacing between the letters is the result of those dots being given the length of a character instead of the proper combined rendering.

[Eric Seidel (no email)]

Presumably the simple-path is being taken for these runs when it shouldn't be.

[Dave Hyatt]

I will look into it. Optimized code path probably being incorrectly chosen for some reason.

[Dave Hyatt]

Created attachment 137178 [details]
Patch

[Dave Hyatt]

Created attachment 137179 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 137179 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2025
> +    TextRun textRun(string, length, false, 0, 0, TextRun::AllowTrailingExpansion, direction, override, true, TextRun::NoRounding);

How could this have worked before?  There are separate constructors for these cases?  Clearly we need to clean up these constructors... probably using some sort of nicely named create variants, as well as enums instead of these true/flase/0 nonsense.

[Eric Seidel (no email)]

I guess what I'm saying is that this mistake was not a product of Dave's coding skills, but of what appears to be horrible design on the part of TextRun constructors.. which we'll need to fix, or we'll just repeat this sort of mistake. :)

[Dave Hyatt]

What we need to do is remove the rounding hacks parameter from the constructors IMO.

[WebKit Review Bot]

Comment on attachment 137179 [details]
Patch

Clearing flags on attachment: 137179

Committed r114205: <http://trac.webkit.org/changeset/114205>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.