editing/selection/click-left-of-rtl-wrapping-text.html and modify-up-on-rtl-wrapping-text.html added by the r72861 fail on Gtk, Qt, Win, Chromium Win, and Chromium Linux platforms. We need to fix the test.
Created attachment 75108 [details] fixes the tests
I verified that new tests pass on Mac and Chromium Windows. I'll really appreciate if someone could test my patch on Gtk, Qt, and other platforms.
Comment on attachment 75108 [details] fixes the tests View in context: https://bugs.webkit.org/attachment.cgi?id=75108&action=review > LayoutTests/editing/selection/click-left-of-rtl-wrapping-text.html:30 > + for (var i = 5; i == 5 || container.offsetHeight > heightOfLine * test.expected.length; i++) I don't understand this for loop. i starts and 5 and checks for == 5 ?
Comment on attachment 75108 [details] fixes the tests View in context: https://bugs.webkit.org/attachment.cgi?id=75108&action=review >> LayoutTests/editing/selection/click-left-of-rtl-wrapping-text.html:30 >> + for (var i = 5; i == 5 || container.offsetHeight > heightOfLine * test.expected.length; i++) > > I don't understand this for loop. i starts and 5 and checks for == 5 ? This is a pseudo do-while. I start at 5px and increase the width as long as the height is too big. But the height is initially heightOfLine (which is shorter than heightOfLine * test.expected.length) so we need to this condition to set width to 5px first so that the height is larger than heightOfLine * test.expected.length.
Created attachment 75184 [details] fixed more
(In reply to comment #5) > Created an attachment (id=75184) [details] > fixed more New tests should pass on Mac, Chromium Windows, and GTK+. We're currently blocked by Qt. Hopefully this new patch do the magic on Qt as well.
Comment on attachment 75184 [details] fixed more View in context: https://bugs.webkit.org/attachment.cgi?id=75184&action=review > LayoutTests/editing/selection/click-left-of-rtl-wrapping-text.html:35 > + var heightOfLine = container.offsetHeight; > + var width = 5; > + do { > + container.style.width = width + 'px'; > + width++; > + } while (container.offsetHeight > heightOfLine * test.expected.length); > + container.style.width = (width + 1) + 'px'; Please add a comment here and in the test below explaining why you're doing this. Just the same thing you told me in person.
(In reply to comment #6) > (In reply to comment #5) > > Created an attachment (id=75184) [details] [details] > > fixed more > > New tests should pass on Mac, Chromium Windows, and GTK+. We're currently blocked by Qt. Hopefully this new patch do the magic on Qt as well. It is failing on my machine, but might work on the Bots (since there can be font differences here). Ossy? --- /tmp/layout-test-results/editing/selection/modify-up-on-rtl-wrapping-text-expected.txt 2010-11-30 17:01:28.830294001 -0500 +++ /tmp/layout-test-results/editing/selection/modify-up-on-rtl-wrapping-text-actual.txt 2010-11-30 17:01:28.830294001 -0500 @@ -4,7 +4,7 @@ PASS: on ך לכ, caret is at 2 after moving upwards once PASS: on כ ככ כככ, caret is at 8 initially PASS: on כ ככ כככ, caret is at 5 after moving upwards once -PASS: on כ ככ כככ, caret is at 2 after moving upwards twice +FAIL: on כ ככ כככ, caret is at 5 after moving upwards twice but expected at 8 PASS: on גכ יגכ יגכ יגכ יגכ, caret is at 18 initially PASS: on גכ יגכ יגכ יגכ יגכ, caret is at 14 after moving upwards once PASS: on גכ יגכ יגכ יגכ יגכ, caret is at 10 after moving upwards twice
Per discussion with Ossy on IRC, it turned out that Qt DRT has a bug. The test passes on browser but fails on DRT. I'll land this patch for now, and file a separate bug to fix Qt DRT.
Thanks for the review, Ojan. Will be landing and re-enabling tests on all platforms but Qt.
Landed as http://trac.webkit.org/changeset/72977. The followup for Qt platform is filed as the bug 50291.
Created attachment 167180 [details] Patch Unskipp test that is passing after change [1] in testfonts. [1] https://gitorious.org/qtwebkit/testfonts/merge_requests/1
Comment on attachment 167180 [details] Patch Cleared review? from attachment 167180 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).
(In reply to comment #13) > (From update of attachment 167180 [details]) > Cleared review? from attachment 167180 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again). Sorry, it's my mistake.