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 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.
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
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.
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
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
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
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
(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?
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...
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
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
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
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.
2014-07-08 07:36 PDT, Mario Sanchez Prada
2014-07-08 08:10 PDT, Build Bot
2014-07-08 08:39 PDT, Mario Sanchez Prada
2014-07-08 11:04 PDT, Build Bot
2014-07-08 12:05 PDT, Build Bot
2014-07-08 12:45 PDT, Build Bot
2014-07-08 13:35 PDT, Build Bot
2014-07-21 08:17 PDT, Mario Sanchez Prada
2014-07-21 09:12 PDT, Build Bot
2014-07-21 09:45 PDT, Build Bot
2014-07-21 10:44 PDT, Build Bot