Summary: | REGRESSION (r138196): Regions with text-overflow: ellipsis; are being ellipsized unnecessarily | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Rowe (bdash) <mrowe> | ||||||
Component: | Layout and Rendering | Assignee: | Emil A Eklund <eae> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | eae, eric, leviw, mitz, ojan.autocc, webkit-bug-importer, webkit.review.bot | ||||||
Priority: | P1 | Keywords: | InRadar, Regression | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
URL: | https://github.com/scalatra/scalatra/ | ||||||||
Attachments: |
|
Description
Mark Rowe (bdash)
2012-12-20 18:35:47 PST
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). Looking into this now. 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 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 The last one should be: $ defaults write com.apple.Safari com.apple.Safari.ContentPageGroupIdentifier.WebKit2ScreenFontSubstitutionEnabled 1 (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 (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! Created attachment 180761 [details]
Patch
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. (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. Created attachment 180811 [details]
Patch for landing
Committed r138543: <http://trac.webkit.org/changeset/138543> |