641444 - text-decoration problem if direction:rtl and text-indent <> 0

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[markus.winand]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.2.15) Gecko/20110303 Firefox/3.6.15
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.2.15) Gecko/20110303 Firefox/3.6.15

The text-decorations underline, overline and line-through are not placed correctly for text with direction:rtl if text-indent is not equal to zero.

The right end-point of the lines is correct while the left endpoint SEEMS to ignore the text-indent. That means, the line is too short if text-indent > 0 and too long if text-indent < 0.




Reproducible: Always

Steps to Reproduce:
1. visit http://fatalmind.com/text_decoration_problem.html
2.
3.
Actual Results:  
incorrectly placed text-decorations (lines not aligned to begin/end of the text)

Expected Results:  
lines that start/end with the text, like the "reference: ltr" section shows at the page end for the link.

Problem is also reproducible with:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0) Gecko/20100101 Firefox/4.0
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1.17) Gecko/20110123 SeaMonkey/2.0.12

And some very old versions as well.

It's probably a long-standing issue.

Comment 1

[Simon Montagu :smontagu]

Attachment: Patch

It's a little counter-intuitive to me that this works, rather than doing something like

    if (GetStyleVisibility()->mDirection == NS_STYLE_DIRECTION_LTR)
      start += indent;
    else
      start -= indent;

Comment 2

[Simon Montagu :smontagu]

Attachment: reftest

Comment 3

[Boris Zbarsky [:bzbarsky]]

Probably better if dbaron reviews this....

That said, I suspect the difference between LTR and RTL here is an asymmetry in how the mBounds of a line box is treated wrt text-indent.  For LTR the .x of the first line box is not affected by the text-indent, while the width is increased by the text-indent.  For RTL, the .x is reduced by the text-indent while the width is not changed.

It would make somewhat more sense to change the width of the line box for the RTL case as well as the LTR case, no?  Then we'd need to reduce the width by the text-indent for both cases (but only adjust "x" for the LTR case, since for the RTL case it's already correct if you think about the geometry).

Comment 4

[Boris Zbarsky [:bzbarsky]]

That said, the attached fix is certainly the safe "don't change anything other than the decoration drawing" fix.  I'm not sure what the impact would be of my proposed change to the line box width.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #3)
> Probably better if dbaron reviews this....
> 
> That said, I suspect the difference between LTR and RTL here is an asymmetry in
> how the mBounds of a line box is treated wrt text-indent.  For LTR the .x of
> the first line box is not affected by the text-indent, while the width is
> increased by the text-indent.  For RTL, the .x is reduced by the text-indent
> while the width is not changed.

So, let's see.  This is all set up in nsLineLayout::BeginLineReflow, which is given aX and aWidth that represent the available space between floats.  It sets up the "root span" with mLeftEdge == mX and mRightEdge to be the sides of the available space with text indent removed, and also sets up mTextIndent (a write-only variable!).

Then the line box bounds are actually set in nsLineLayout::VerticalAlignLine, to be m

(I'll pretend I didn't see the whole mX business... I guess the bidi code goes through and "fixes up" all the positions.)

So we could make LTR and RTL work the same by either:
 (a) making LTR behave like RTL does, by also adjusting mLeftEdge in BeginLineReflow in the LTR text-indent case
 (b) making RTL behave like LTR does, by *subtracting* mTextIndent from mLineBox->mBounds.width in VerticalAlignLine for RTL only

> It would make somewhat more sense to change the width of the line box for the
> RTL case as well as the LTR case, no?

You mean to *not* change the width of the line box?  Because it looks to me like text-indent currently reduces the width of RTL line boxes, but leaves LTR line boxes as-is (though they then shrink-wrap around the content, which also seems wrong to me in CSS terms).


That said, the patch seems correct given the current code, except you may as well move the direction check out to the top of AdjustForTextIndent, with an explanation.  With that, and a comment explaining why it's that way, I'd be ok with just fixing this rather than trying to change the line box size... though I'd also be happy with trying to change the line box size.

Really, though, all the standards mode text-decoration code should soon go away (bug 403524).

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 519109 [details] [diff] [review]
Patch

If you don't want to tackle making this more sane, r=dbaron if you move the bail-if-rtl up to the top of the function.  This'll be a case where the RTL code is faster than the LTR code...

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 519116 [details] [diff] [review]
reftest

r=dbaron

Comment 8

[Simon Montagu :smontagu]

http://hg.mozilla.org/mozilla-central/rev/a9d788b76428
http://hg.mozilla.org/mozilla-central/rev/7a8ebf42265a

Comment 9

[Simona Badau, Desktop QA]

WFM on: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0a1) Gecko/20110728 Firefox/8.0a1

Verified issue on the latest mozilla central on Mac OS X 10.6 - text decoration problem is fixed.

Shouldn't  this patch land on Firefox 6 beta as well (Target Milestone is mozilla 6)? Currently issue described in Comment 0 is still reproducible on Firefox 6.0b3.