Bug 83826

Summary:

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

Product:

WebKit

Reporter:

Vincent Scheib <scheib>

Component:

Text

Assignee:

Dave Hyatt <hyatt>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

andersca, ap, eric, hyatt, leviw, mitz, rniwa, tkent, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none

Description Vincent Scheib 2012-04-12 15:40:00 PDT

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.

Comment 1 Levi Weintraub 2012-04-13 16:27:39 PDT

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

Comment 2 Levi Weintraub 2012-04-13 16:30:48 PDT

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.

Comment 3 Eric Seidel (no email) 2012-04-13 16:34:49 PDT

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

Comment 4 Dave Hyatt 2012-04-13 16:36:29 PDT

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

Comment 5 Dave Hyatt 2012-04-13 16:47:51 PDT

Created attachment 137178 [details]
Patch

Comment 6 Dave Hyatt 2012-04-13 16:48:37 PDT

Created attachment 137179 [details]
Patch

Comment 7 Eric Seidel (no email) 2012-04-13 22:11:01 PDT

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.

Comment 8 Eric Seidel (no email) 2012-04-13 22:12:12 PDT

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. :)

Comment 9 Dave Hyatt 2012-04-14 11:06:32 PDT

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

Comment 10 WebKit Review Bot 2012-04-14 11:16:06 PDT

Comment on attachment 137179 [details]
Patch

Clearing flags on attachment: 137179

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

Comment 11 WebKit Review Bot 2012-04-14 11:16:11 PDT

All reviewed patches have been landed.  Closing bug.