Closed Bug 449519 Opened 13 years ago Closed 13 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: yehudab, Assigned: roc)

References

Details

(Keywords: rtl, testcase, verified1.9.1)

Attachments

(5 files, 1 obsolete file)

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
Attached file test case
Status: UNCONFIRMED → NEW
Ever confirmed: true
The same problem is present on Linux, so changing from Macintosh/Mac OS X to All/All.
OS: Mac OS X → All
Hardware: Macintosh → 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.
Attached patch textframe fix (obsolete) — Splinter Review
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.
Assignee: nobody → roc
Status: NEW → ASSIGNED
Attachment #332830 - Flags: review?(smontagu)
Attached patch gfx fixSplinter Review
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.
Attachment #332832 - Flags: review?(vladimir)
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.
Blocks: text-shadow
Keywords: rtl, testcase
Attached patch textframe fix v2Splinter Review
Attachment #332830 - Attachment is obsolete: true
Attachment #333504 - Flags: review?
Attachment #332830 - Flags: review?(smontagu)
Attachment #333504 - Flags: review? → review?(smontagu)
Attachment #333504 - Flags: review?(smontagu) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
Flags: wanted1.9.1? → wanted1.9.1+
Keywords: fixed1.9.1
Keywords: verified1.9.1
Keywords: fixed1.9.1
You need to log in before you can comment on or make changes to this bug.