If a letter-spacing is applied to text each character is written seperately. After each letter is written the width of that character is added to the x coordinate. Then the letter-spacing will be added and the next character is drawn at the new x coordinate. The difference between the old x and the new x is what I call deltax below. A positive letter-spacing means that deltax is larger than it would have been if the letter-spacing was not applied. Similarly, a negative letter-spacing means that deltax is smaller than it would have been if the letter-spacing was not applied. CURRENT BEHAVOIR: If you increase the negative letter-spacing it will eventually become larger than the width of the characters and at that point deltax will become negative and the text starts to flow backwards. If the width of the negative letter-spacing is larger than the width of a character the text will run backwards. This can be demonstrated by the following little snipped of HTML: <h1 style='padding-left: 10em; letter-spacing: -32px;'>The quick brown fox...</h1>" This is displayed as: ...xof nworb kciuq ehT DESIRED BEHAVOIR: If you increase the negative letter-spacing it will eventually become larger than the width of the characters and at that point deltax will become negative. If deltax is don't draw the character. This means that text that used to flow backwards now simply is not shown at all, as desired by the MIR image replacement technique.
Created attachment 2580 [details] Testcase This testcase contains 10 lines. - Line 1 - 3 should contain the string "The quick brown fox jumps..." in various letter spacings. - Line 4 - 9 should be completely empty. - Line 10 should contain the string "The quick brown over the lazy dog".
Created attachment 2581 [details] Possible patch This seems too simple, but it does solve the problem on various testcases and live websites. Any comments?
This is invalid. Negative letter spacing _should_ make the text go backwards. That fact that it doesn't in other browsers is a bug in those browsers.
Filed https://bugzilla.mozilla.org/show_bug.cgi?id=298579
The CSS 2.1 spec says the following about negative letter-spacing: Values may be negative, but there may be implementation-specific limits. At the moment the two browsers that are used most, Internet Explorer and Firefox both have an implementation-specific limit that result in the text disappearing when a large negative value is used. Emulating the behavoir of those two browsers and adding an artificial limit to KHTML/Webcore is not contrary to the CSS 2.1 spec. On the other hand it will improve compatibility with websites that employ the MIR image replacement technique, which depends on the behavoir of IE and Firefox.
Agreed.
Doing what IE does here is stupid. It makes it impossible to achieve several dynamic effects. Opera already does the right thing here, as will Mozilla in due course. The implementation-specific limits clause is not supposed to mean you can fail to do the right thing if you want to, it's to allow small devices that can't handle negative numbers to do a "best effort" rendering. This bug is INVALID. If it is "fixed" it should be in quirks mode only, and important pages that it breaks should be listed.
Can someone from Gecko comment on whether they intend to fix the bug? If they don't, then I feel that we should change to match.
I think it's a bug that we should fix in Gecko.
We do not have the same behavior as Chrome or Firefox for the final line of the test case (the string with multiple colors).
<rdar://problem/96534455>
Created attachment 464215 [details] testcase for negative-letter-spacing As Brent said, Only the last test case is now failing. Let's make it simpler and easier to understand what is happening.
Created attachment 464216 [details] rendering in safari, firefox, chrome Tested on macOS 13.3 --- Safari Technology Preview 160 18615.1.14.3 Firefox Nightly 110.0a1 11022.12.14 Google Chrome Canary 111.0.5499.0 5499.0
Created attachment 464239 [details] Test reduction Thank you Karl for the negative letter-spacing test case.
Created attachment 464241 [details] Test case screenshot This fixes both test cases. diff --git a/Source/WebCore/layout/formattingContexts/inline/InlineLine.cpp b/Source/WebCore/layout/formattingContexts/inline/InlineLine.cpp index a1d927521680..7d84517d22cd 100644 --- a/Source/WebCore/layout/formattingContexts/inline/InlineLine.cpp +++ b/Source/WebCore/layout/formattingContexts/inline/InlineLine.cpp @@ -403,7 +403,8 @@ void Line::appendInlineBoxEnd(const InlineItem& inlineItem, const RenderStyle& s // https://drafts.csswg.org/css-text-3/#letter-spacing-property See example 21. removeTrailingLetterSpacing(); m_contentLogicalWidth -= removeBorderAndPaddingEndForInlineBoxDecorationClone(inlineItem); - auto logicalLeft = lastRunLogicalRight(); + // Negative letter spacing should only shorten the content to the boundary of the previous run. + auto logicalLeft = style.letterSpacing() < 0 ? m_contentLogicalWidth : lastRunLogicalRight(); m_runs.append({ inlineItem, style, logicalLeft, logicalWidth }); // Do not let negative margin make the content shorter than it already is. m_contentLogicalWidth = std::max(m_contentLogicalWidth, logicalLeft + logicalWidth);
Created attachment 464242 [details] Test case screenshot (2)
Created attachment 464243 [details] test case screenshot (original)
Created attachment 464249 [details] Patch
Comment on attachment 464249 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=464249&action=review > COMMIT_MESSAGE:2 > +https://bugs.webkit.org/show_bug.cgi?id=3676 nice!
Comment on attachment 2581 [details] Possible patch View in context: https://bugs.webkit.org/attachment.cgi?id=2581&action=review > font.cpp:107 > - if ( d == QPainter::RTL ) > + > + if ( d == QPainter::RTL ) { > + if (chw > 0) > + return; > + > x -= chw; > + } else if (chw < 0) > + return; I do like this patch too.
(In reply to Antti Koivisto from comment #20) > Comment on attachment 2581 [details] > Possible patch > > View in context: https://bugs.webkit.org/attachment.cgi?id=2581&action=review > > > font.cpp:107 > > - if ( d == QPainter::RTL ) > > + > > + if ( d == QPainter::RTL ) { > > + if (chw > 0) > > + return; > > + > > x -= chw; > > + } else if (chw < 0) > > + return; > > I do like this patch too. will check if it applies (need to install cvs first)
Committed 258356@main (4aee4f4a4f9d): <https://commits.webkit.org/258356@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 464249 [details].