449519 - Text-shadow with non-zero blur does not render for Hebrew and Arabic text

Status: VERIFIED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Yehuda]

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

Comment 1

[Yehuda]

Attachment: test case

Comment 2

[Yehuda]

Attachment: Firefox rendering of the test-case

Comment 3

[Yehuda]

Attachment: Safari rendering of the test-case

Comment 4

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

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

Comment 5

[Michael Ventnor]

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?

Comment 6

[Michael Ventnor]

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

Comment 7

[Michael Ventnor]

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.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: textframe fix

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.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: gfx fix

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 10

[Simon Montagu :smontagu]

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.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 12

[Simon Montagu :smontagu]

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.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You're right, I messed that up.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed the gfx patch as 806a38eff686.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: textframe fix v2

Comment 16

[Simon Montagu :smontagu]

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.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed 1a0205ddbd4f.

Comment 18

[Carsten Book [:Tomcat]]

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