Bug 134597

Summary: Convert fast/css/text-overflow-ellipsis* layout tests to reftests
Product: WebKit Reporter: Mario Sanchez Prada <mario>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: buildbot, bunhere, cdumez, commit-queue, dtrebbien, gyuyoung.kim, rniwa, sergio
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 129313    
Attachments:
Description Flags
Path proposal
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
none
Path proposal
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
none
Patch proposal (trying simply with a monospace font-family)
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion none

Description Mario Sanchez Prada 2014-07-03 09:47:48 PDT
This should pave the way to land fix for WebKit bug 129313 (see discussion there for more information)
Comment 1 Mario Sanchez Prada 2014-07-08 07:36:40 PDT
Created attachment 234560 [details]
Path proposal

I would love to have more tests converted in a go, but the ones related to text alignment need to be converted separately (see bug 129313 and bug 134724)
Comment 2 Ryosuke Niwa 2014-07-08 07:45:08 PDT
Comment on attachment 234560 [details]
Path proposal

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

> LayoutTests/fast/css/text-overflow-ellipsis-expected.html:18
> +.forcertl {
> +    direction: rtl;
> +    unicode-bidi: bidi-override;
> +}

I don't think we should be relying on UBA in the expected result for this test given where ellipses will be drawn will be affected by UBA itself.
Reorder the content manually instead.

> LayoutTests/fast/css/text-overflow-ellipsis-strict-expected.html:18
> +    direction: rtl;
> +    unicode-bidi: bidi-override;

Ditto.
Comment 3 Build Bot 2014-07-08 08:09:55 PDT
Comment on attachment 234560 [details]
Path proposal

Attachment 234560 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4782096186867712

New failing tests:
fast/css/text-overflow-ellipsis-strict.html
fast/css/text-overflow-ellipsis-bidi.html
fast/css/text-overflow-ellipsis.html
fast/dom/HTMLImageElement/sizes/image-sizes-js-change.html
Comment 4 Build Bot 2014-07-08 08:10:00 PDT
Created attachment 234562 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 5 Mario Sanchez Prada 2014-07-08 08:39:49 PDT
Created attachment 234563 [details]
Path proposal

(In reply to comment #2)
> (From update of attachment 234560 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234560&action=review
> 
> > LayoutTests/fast/css/text-overflow-ellipsis-expected.html:18
> > +.forcertl {
> > +    direction: rtl;
> > +    unicode-bidi: bidi-override;
> > +}
> 
> I don't think we should be relying on UBA in the expected result for this test given where ellipses will be drawn will be affected by UBA itself.
> Reorder the content manually instead.
> 
Good point. Attaching now a new patch following that suggestion.

> > LayoutTests/fast/css/text-overflow-ellipsis-strict-expected.html:18
> > +    direction: rtl;
> > +    unicode-bidi: bidi-override;
> 
> Ditto.
Comment 6 Build Bot 2014-07-08 11:04:33 PDT
Comment on attachment 234563 [details]
Path proposal

Attachment 234563 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4560418697641984

New failing tests:
fast/css/text-overflow-ellipsis-strict.html
fast/css/text-overflow-ellipsis-bidi.html
fast/css/text-overflow-ellipsis.html
Comment 7 Build Bot 2014-07-08 11:04:39 PDT
Created attachment 234575 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 8 Build Bot 2014-07-08 12:05:31 PDT
Comment on attachment 234563 [details]
Path proposal

Attachment 234563 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5382742799810560

New failing tests:
fast/css/text-overflow-ellipsis-strict.html
fast/css/text-overflow-ellipsis-bidi.html
fast/css/text-overflow-ellipsis.html
Comment 9 Build Bot 2014-07-08 12:05:36 PDT
Created attachment 234579 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 10 Build Bot 2014-07-08 12:45:39 PDT
Comment on attachment 234563 [details]
Path proposal

Attachment 234563 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5759239632977920

New failing tests:
media/W3C/video/src/src_reflects_attribute_not_source_elements.html
fast/css/text-overflow-ellipsis-bidi.html
fast/css/text-overflow-ellipsis.html
fast/css/text-overflow-ellipsis-strict.html
Comment 11 Build Bot 2014-07-08 12:45:43 PDT
Created attachment 234582 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 12 Build Bot 2014-07-08 13:35:47 PDT
Comment on attachment 234563 [details]
Path proposal

Attachment 234563 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5413391686434816

New failing tests:
fast/css/text-overflow-ellipsis-strict.html
fast/css/text-overflow-ellipsis-bidi.html
fast/css/text-overflow-ellipsis.html
Comment 13 Build Bot 2014-07-08 13:35:51 PDT
Created attachment 234590 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 14 Mario Sanchez Prada 2014-07-18 06:48:32 PDT
(In reply to comment #12)
> (From update of attachment 234563 [details])
> Attachment 234563 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.appspot.com/results/5413391686434816
> 
> New failing tests:
> fast/css/text-overflow-ellipsis-strict.html
> fast/css/text-overflow-ellipsis-bidi.html
> fast/css/text-overflow-ellipsis.html

For some reason that I can't explain, the actual and expected result provided by the ref test do not match in the Mac because of one additional character being printed in the Mac before the ellipsis.

I've tried to do a WebKit build in a Mac I have over here, but then faced some other problems with Xcode that I'm still trying to decipher and couldn't advance much either.

Any idea?
Comment 15 Mario Sanchez Prada 2014-07-18 07:36:22 PDT
Comment on attachment 234563 [details]
Path proposal

Clearing the r? since it's clear this patch can't get in
Comment 16 Mario Sanchez Prada 2014-07-21 08:17:26 PDT
Created attachment 235220 [details]
Patch proposal (trying simply with a monospace font-family)

This small change seemed to help in the local experiments I did. Let's see what the EWS think about it...
Comment 17 Build Bot 2014-07-21 09:12:14 PDT
Comment on attachment 235220 [details]
Patch proposal (trying simply with a monospace font-family)

Attachment 235220 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5428177178460160

New failing tests:
fast/css/text-overflow-ellipsis-strict.html
fast/css/text-overflow-ellipsis-bidi.html
fast/css/text-overflow-ellipsis.html
Comment 18 Build Bot 2014-07-21 09:12:19 PDT
Created attachment 235221 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 19 Build Bot 2014-07-21 09:45:19 PDT
Comment on attachment 235220 [details]
Patch proposal (trying simply with a monospace font-family)

Attachment 235220 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4712264749809664

New failing tests:
fast/css/text-overflow-ellipsis-strict.html
fast/css/text-overflow-ellipsis-bidi.html
fast/css/text-overflow-ellipsis.html
Comment 20 Build Bot 2014-07-21 09:45:25 PDT
Created attachment 235223 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 21 Build Bot 2014-07-21 10:44:47 PDT
Comment on attachment 235220 [details]
Patch proposal (trying simply with a monospace font-family)

Attachment 235220 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5741758746984448

New failing tests:
fast/css/text-overflow-ellipsis-strict.html
fast/css/text-overflow-ellipsis-bidi.html
fast/css/text-overflow-ellipsis.html
Comment 22 Build Bot 2014-07-21 10:44:52 PDT
Created attachment 235228 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 23 Mario Sanchez Prada 2014-07-24 06:27:56 PDT
It seems that part of the reason why these tests are failing here only in the mac is because of the calls to drawSkipInkUnderline() functions, which are not being called in WebKitGTK+ (because of the ENABLE(CSS3_TEXT_DECORATION_SKIP_INK) guard).

When that function is called, translateIntersectionPointsToSkipInkBoundaries() is called too where it seems that "way too many" dashes (and the gaps in the middle) are being returned as the result DashArray. Reason for that seems to be that is not taking into consideration the fact that the whole text might be ellipsized, and therefore adding tuples of coordinates definining those gaps which are out of the range defined by the visible part of the text is wrong, causing the underline to show up shorter than what it actually is if there's a single character later on the line requiring a "ink skip".

The following experiment "fixes" some cases of this weird behaviour, even though it does not seem correct (we would probably need to properly consider text direction and things like that):

--- a/Source/WebCore/rendering/InlineTextBox.cpp
+++ b/Source/WebCore/rendering/InlineTextBox.cpp
@@ -80,8 +80,11 @@ static DashArray translateIntersectionPointsToSkipInkBoundaries(const DashArray&
     
     // Step 1: Make pairs so we can sort based on range starting-point. We dilate the ranges in this step as well.
     Vector<std::pair<float, float>> tuples;
-    for (auto i = intersections.begin(); i != intersections.end(); i++, i++)
+    for (auto i = intersections.begin(); i != intersections.end(); i++, i++) {
+        if (*i <= -totalWidth || *i >= totalWidth)
+            continue;
         tuples.append(std::make_pair(*i - dilationAmount, *(i + 1) + dilationAmount));
+    }

Unfortunately, I don't have much time anymore this week and I'll be away for a while after it, so I can't keep devoting time to this issue (which happens to show up in the Mac only, btw), so I'm posting this here in the hope that someone with a better understanding of this part can take a look and see whether it makes sense or not.

PS: drawLineForText() works perfectly fine, the problem is with drawSkipInkUnderline() because translateIntersectionPointsToSkipInkBoundaries() returns gaps not belonging to the visible (not wrapped) test, which causes that weird effect.