Bug 3676 – MIR Image replacement technique (negative letter-spacing) does not always work

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Niels Leenheer (HTML5test)]

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.

[Niels Leenheer (HTML5test)]

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".

[Niels Leenheer (HTML5test)]

Created attachment 2581 [details]
Possible patch

This seems too simple, but it does solve the problem on 
various testcases and live websites. Any comments?

[Ian 'Hixie' Hickson]

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.

[Ian 'Hixie' Hickson]

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=298579

[Niels Leenheer (HTML5test)]

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.

[Dave Hyatt]

Agreed.

[Ian 'Hixie' Hickson]

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.

[Dave Hyatt]

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.

[Robert O'Callahan]

I think it's a bug that we should fix in Gecko.

[Brent Fulgham]

We do not have the same behavior as Chrome or Firefox for the final line of the test case (the string with multiple colors).

[Radar WebKit Bug Importer]

<rdar://problem/96534455>

[Karl Dubost]

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.

[Karl Dubost]

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

[zalan]

Created attachment 464239 [details]
Test reduction

Thank you Karl for the negative letter-spacing test case.

[zalan]

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);

[zalan]

Created attachment 464242 [details]
Test case screenshot (2)

[zalan]

Created attachment 464243 [details]
test case screenshot (original)

[zalan]

Created attachment 464249 [details]
Patch

[Antti Koivisto]

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!

[Antti Koivisto]

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.

[zalan]

(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)

[EWS]

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].