User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1a1) Gecko/2008072510 Shiretoko/3.1a1 Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1a1) Gecko/2008072510 Shiretoko/3.1a1 When text-shadow is specified for an element on the page, and this element contains either Hebrew or Arabic letters, this shadow will show up only if the blur radius (third length parameter) is either 0, or not specified. If the blur radius is specified as greater then 0, no shadow is visible bellow the Hebrew and Arabic text (except for a small "stain" to the right of the rightmost letter). See attached test case and screen shots with Firefox 3.1a1 vs. Safari Reproducible: Always Steps to Reproduce: 1. Open test case page 2. Observe the missing text shadow bellow the Hebrew and Arabic text on the second paragraph. Actual Results: Only the first paragraph (with blur radius of 0px) displays a shadow bellow the Hebrew and Arabic text Expected Results: Both paragraphs should displays a shadow bellow the Hebrew and Arabic text

The same problem is present on Linux, so changing from Macintosh/Mac OS X to All/All.

I'm thinking this is a problem with the way we do text. When you select the first line you can see part of the arabic text actually come above the selection background, which shouldn't happen. I may not have much time to look at this; roc, could you have a go when you have time?

On the other hand, it may be because I haven't taken RTL into account somewhere.

Definitely roc's bug; for some reason mTextRun->MeasureText() is returning a width of 0 for those RTL textruns. Checked by putting if (blurRadius && mTextRun->IsRightToLeft()) printf("%d, %d for shadow\n", shadowRect.Width(), shadowRect.Height()); near line 4045 underneath the shadowRect.MoveBy() call.

Actually there are two bugs here and the first one is Michael's :-). The main issue is that mBoundingBox is relative to the *left* end of the baseline, but in RTL situations aTextBaselinePt is the *right* end of the baseline. So we need to use aFramePt.x instead of aTextBaselinePt.x since aFramePt.x is always the left end. I also noticed that we don't properly adjust the metrics for a soft hyphen, so I fixed that while I'm here.

This testcase also revealed another bug, at least with the fonts I have. We were getting the wrong mBoundingBox out --- the y and height were all wrong. The problem is that gfxFont::Measure starts off setting mBoundingBox to width 0, y = -ascent and height = ascent + descent. Now if the *first* glyph overflows the font-box, we take the path that computes the glyph's rect and calls mBoundingBox.Union. Unfortunately Union notices mBoundingBox is empty (zero width) and totally ignores it, so we lose the font ascent and descent information. This patch adds the font-box information at the end to avoid the problem.

Comment on attachment 332830 [details] [diff] [review] textframe fix >+ if (aBaseTextRun->IsRightToLeft()) { >+ // Char comes before text, so the bounding box is moved to the >+ // right by aWidth >+ aMetrics->mBoundingBox.MoveBy(gfxPoint(width, 0)); >+ } else { >+ // char is moved to the right by mAdvanceWidth >+ charRect.MoveBy(gfxPoint(width, 0)); The comments don't correspond to the code here, and in the LTR case, the comment sounds more correct. This should be reftestable by having RTL text in the test and the same text reversed between LRO-PDF in the reference.

It should say "hyphen" rather than "char", and "width" instead of "aWidth". Other than that the comment and code are correct, right?

In the LTR case, the comment says "the char is moved to the right by mAdvanceWidth", which I assumed means aMetrics->mAdvanceWidth, i.e. it is moved by the width of the text run, taking it from the left edge of the text run to the right edge of the text run. However, the code moves it to the right by its own width, which seems wrong if all my assumptions are correct.

You're right, I messed that up.

Pushed the gfx patch as 806a38eff686.

By the way, it seems wrong to me that the length attributes for text-shadow presuppose left-to-right (and top-to-bottom), rather than depending on text direction, but that's a question to take up on www-style rather than here.

Pushed 1a0205ddbd4f.

verified fixed using the testcase and Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.1b1pre) Gecko/20080910043000 Minefield/3.1b1pre