345954 - RTL alt-image-text displayed incorrectly

Status: RESOLVED FIXED

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

Description

[Niro]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060724 Minefield/3.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060724 Minefield/3.0a1

On Windows, RTL alt text with punctuation is displayed incorrectly.
for details, see the upcoming testcase.
Thanks to Uri Bernstein I could figure out the whole issue.



Reproducible: Always

Comment 1

[Niro]

Attachment: testcase

Comment 2

[Uri Bernstein]

On Mac the problem appears worse: the entire text is laid out left-to-right, letter by letter.

Confirming, changing to All/All, and moving to BiDi.

Comment 3

[Simon Montagu :smontagu]

On Linux (at least without pango) this is similar to what Uri describes on Mac. This is a regression, but it may be a very old one (i.e. the last time I remember testing this was several years ago)

Comment 4

[Simon Montagu :smontagu]

From discussion on IRC about the exact scope of the bug, it doesn't seem to be a regression. Pure right-to-left text on Windows with no punctuation displays correctly.

Taking bug.

Comment 5

[Simon Montagu :smontagu]

Just making the text rendered correctly is fairly trivial, but there are other cans of worms that this opens:

If the image has right-to-left direction (specified or inherited) should the text be displayed with right-to-left base direction?

If the image has right-to-left direction, when the image is replaced by a place holder box with an icon in quirks mode, should the text and the icon be aligned right within the box?

In strict mode, when the image is replaced by inline text, should the text participate in continuous Bidi reordering of the line in which it appears, or should it open a Bidi embedding?

I think the answers to those questions should be "yes", "yes" and "embedding", but no specification seems to address them. I haven't yet tested strict mode in other browsers, but Safari, IE6 on Windows and Opera on Windows all completely ignore the direction in quirks mode. Gecko in strict mode treats the alt text as if it were within <span dir="rtl">, without embedding.

Comment 6

[Uri Bernstein]

To summarize my opinion as I expressed it on IRC:

I agree on "yes" and "yes" for the first two questions, but (weakly) disagree with "embedding" on the third, that is, I think the text should participate in continuous Bidi reordering.

Comment 7

[Simon Montagu :smontagu]

Attachment: patch

OK, this patch handles the bidi reordering, alignment within the placeholder frame and alignment of the icon in the RTL (quirks-mode) case, and doesn't attempt to create embeddings in the standards-mode case.

-      aRenderingContext.DrawRect(aPt.x, aPt.y,size,size);
+      nscoord iconXPos = (vis->mDirection ==   NS_STYLE_DIRECTION_RTL) ?
+                         inner.XMost() - size : inner.x;
+      aRenderingContext.DrawRect(iconXPos, inner.y,size,size);

The change from aPt.x/y to inner.x/y is actually a fix for a different bug: when the icon is not available we only display the bottom right quadrant of the rectangle drawn here - the rest is outside the clip area set at the top of this hunk. The result looks like a small square with a red circle in the centre, not (what I assume was intended) a rectangle of the same dimensions as the icon, with a red circle in the bottom right corner.

Comment 8

[Uri Bernstein]

Comment on attachment 230990 [details] [diff] [review]
patch

It took me a while to understand the red circle stuff, but I think I did eventually.

Looks good to me.

Comment 9

[Simon Montagu :smontagu]

from IRC:
	<Hixie>	we should stop the bidi algorithm from "leaking" out of alt text
	<Hixie>	the same applies to generated content and :before, maybe

Comment 10

[Niro]

I've just noticed that on windows the bug regressed.
On Firefox 1.5 I can see the alt text in the testcase correctly.

Comment 11

[Niro]

(In reply to comment #10)
> I've just noticed that on windows the bug regressed.
> On Firefox 1.5 I can see the alt text in the testcase correctly.
> 

On windows, It's probably cairo-related due to the regression range:
20060222 - 20060226
(I can't narrow the range)

So maybe on Mac and Linux it's a separate bug and the correct component for windows is GFX: Thebes (?)

Comment 12

[Simon Montagu :smontagu]

It's possible that Cairo (or rather Uniscribe script and font selection) causes the exact form that the bug takes on Windows trunk builds, but I still think it's fundamentally the same bug on all platforms. I will make sure to test the fix on Cairo and non-Cairo builds though.

Comment 13

[Simon Montagu :smontagu]

Attachment: patch to html.css to make a bidi embedding for the alt text

Comment 14

[Uri Bernstein]

Comment on attachment 231299 [details] [diff] [review]
patch to html.css to make a bidi embedding for the alt text 

If Hixie says so...

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 230990 [details] [diff] [review]
patch

>Index: layout/generic/nsImageFrame.cpp

>+        nsRect dest((vis->mDirection == NS_STYLE_DIRECTION_RTL) ?
>+                    inner.XMost() - size : inner.x,inner.y,size,size);

This could really use better formatting.  For example, put the inner.y part on a new line, spaces after commas, maybe indent the inner.XMost() part so it's clear what it belongs with...

>Index: layout/generic/nsImageFrame.h

>+  nscoord MeasureString(const PRUnichar*     aString,

Document the return value, please.

sr=bzbarsky with the nits picked.

Comment 16

[Simon Montagu :smontagu]

Fix checked in.

Comment 17

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).