REGRESSION(r72861): editing/selection/click-left-of-rtl-wrapping-text.html and modify-up-on-rtl-wrapping-text.html fail on all but Mac platform

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 75108 [details]
fixes the tests

[Ryosuke Niwa]

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.

[Ojan Vafai]

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 ?

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 75184 [details]
fixed more

[Ryosuke Niwa]

(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.

[Ojan Vafai]

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.

[Antonio Gomes]

(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

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Thanks for the review, Ojan.  Will be landing and re-enabling tests on all platforms but Qt.

[Ryosuke Niwa]

Landed as http://trac.webkit.org/changeset/72977.

The followup for Qt platform is filed as the bug 50291.

[Tullio Lucena]

Created attachment 167180 [details]
Patch

Unskipp test that is passing after change [1] in testfonts.

[1] https://gitorious.org/qtwebkit/testfonts/merge_requests/1

[Eric Seidel (no email)]

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

[Tullio Lucena]

(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.