Bug 83826 - REGRESSION (r114032): Layout Tests fast/text/drawBidiText.html fast/text/international/bidi-listbox-atsui.html are failing
Summary: REGRESSION (r114032): Layout Tests fast/text/drawBidiText.html fast/text/inte...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-12 15:40 PDT by Vincent Scheib
Modified: 2012-04-14 11:16 PDT (History)
9 users (show)

See Also:


Attachments
Patch (3.96 KB, patch)
2012-04-13 16:47 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (2.62 KB, patch)
2012-04-13 16:48 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.