REGRESSION (r138196): Regions with text-overflow: ellipsis; are being ellipsized unnecessarily

[Mark Rowe (bdash)]

After <http://trac.webkit.org/changeset/138196> I'm seeing unnecessarily ellipsized text on a number of different websites. For instance, on <https://github.com/scalatra/scalatra/> the folder listing includes many ellipsized items. In particular, the first item "akka" is displayed as "ak…" even though the following item is substantially longer and is not itself truncated.

This is also visible on Facebook in the news feed sorting widget. "Most recent" is truncated to "Must rece…", even though there's clearly room for the "nt" to be displayed.

[Radar WebKit Bug Importer]

<rdar://problem/12922386>

[Levi Weintraub]

If this is urgent for anyone, feel free to roll out the patch. Otherwise Emil or I will take a look tomorrow AM PST (assuming he doesn't look at it tonight).

[Emil A Eklund]

Looking into this now.

[Emil A Eklund]

I have not been able to reproduce the problem on the github site listed above nor on facebook in a tip of tree safari build regardless of zoom level.

You may need to disable screen font substitution and/or enable kerning and ligatures by default to see this. The relevant user defaults are:

WebKitScreenFontSubstitutionEnabled
WebKitKerningAndLigaturesEnabledByDefault

(In reply to comment #5)
> You may need to disable screen font substitution and/or enable kerning and ligatures by default to see this. The relevant user defaults are:
> 
> WebKitScreenFontSubstitutionEnabled

This is actually a WebKit preference, so for Safari’s WebKit2 content view you’ll need to use

com.apple.Safari.ContentPageGroupIdentifier.WebKit2ScreenFontSubstitutionEnabled

> WebKitKerningAndLigaturesEnabledByDefault

[Emil A Eklund]

Tried setting the defaults as but still does not reproduce. Are there any other settings I might need (or perhaps the defaults needs to be set in a different way)?

$ defaults write com.apple.Safari WebKitScreenFontSubstitutionEnabled 1
$ defaults write com.apple.Safari WebKitKerningAndLigaturesEnabledByDefault 1
$ defaults write com.apple.Safari.ContentPageGroupIdentifier WebKit2ScreenFontSubstitutionEnabled 1

[Mark Rowe (bdash)]

The last one should be:

$ defaults write com.apple.Safari com.apple.Safari.ContentPageGroupIdentifier.WebKit2ScreenFontSubstitutionEnabled 1

[Emil A Eklund]

(In reply to comment #8)
> The last one should be:
> 
> $ defaults write com.apple.Safari com.apple.Safari.ContentPageGroupIdentifier.WebKit2ScreenFontSubstitutionEnabled 1

Ah, thanks. Sadly I'm still not able to reproduce the problem.

(In reply to comment #9)
> (In reply to comment #8)
> > The last one should be:
> > 
> > $ defaults write com.apple.Safari com.apple.Safari.ContentPageGroupIdentifier.WebKit2ScreenFontSubstitutionEnabled 1
> 
> Ah, thanks. Sadly I'm still not able to reproduce the problem.

1 (or YES) is the default on Mountain Lion and earlier. In order to be able to reproduce the problem, please try

$ defaults write com.apple.Safari com.apple.Safari.ContentPageGroupIdentifier.WebKit2ScreenFontSubstitutionEnabled 0

[Emil A Eklund]

(In reply to comment #10)
> 1 (or YES) is the default on Mountain Lion and earlier. In order to be able to reproduce the problem, please try
> 
> $ defaults write com.apple.Safari com.apple.Safari.ContentPageGroupIdentifier.WebKit2ScreenFontSubstitutionEnabled 0

That did the trick, thanks!

[Emil A Eklund]

Created attachment 180761 [details]
Patch

[Darin Adler]

Comment on attachment 180761 [details]
Patch

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

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:3206
> +        int lineBoxEdge = ltr ? snapSizeToPixel(curr->x() + curr->logicalWidth(), curr->x()) : snapSizeToPixel(curr->x(), 0);

Is there a way to write the two snaps more consistently. I’m concerned that curr->x() ends up being the reference value when it’s LTR, but 0 when it’s RTL. Does not seem quite right. And the test case only covers LTR, so the RTL code could be wrong and the test would not show us.

[Emil A Eklund]

(In reply to comment #13)
> (From update of attachment 180761 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180761&action=review
> 
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:3206
> > +        int lineBoxEdge = ltr ? snapSizeToPixel(curr->x() + curr->logicalWidth(), curr->x()) : snapSizeToPixel(curr->x(), 0);
> 
> Is there a way to write the two snaps more consistently. I’m concerned that curr->x() ends up being the reference value when it’s LTR, but 0 when it’s RTL. Does not seem quite right. And the test case only covers LTR, so the RTL code could be wrong and the test would not show us.

I tested with both LTR and RTL content and both resulted in incorrect truncation. The proposed change fixes the problem for both LTR and RTL and the snapping matches the logic we use for snapping rtl content elsewhere. I'll updated the patch to include a test for RTL content.

[Emil A Eklund]

Created attachment 180811 [details]
Patch for landing

[Emil A Eklund]

Committed r138543: <http://trac.webkit.org/changeset/138543>