282760 - Rendering gives Hebrew text OUTSIDE of HTML table cell for some window sizes.

Status: VERIFIED FIXED

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

Description

[Christopher V. Kimball]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.3) Gecko/20040910

In http://www.cvkimball.com/Tanach/TanachSearch.xml?Gen37:1-37:4, if the 
containing horizontal window size is adjusted about 75% smaller than the 
average user would start the page, eventually (when Zilpah is the last word on 
the third line, Zilpah is the 16-th word in verse 37:2) the Hebrew text on the 
fourth line occurs outside the table.  When this occurs, a horizontal slide bar 
occurs, which if shifted to the right, shows the text outside the table cell.

Reproducible: Always

Steps to Reproduce:
1. Open the referenced web page with a nearly full screen horizontal window 
size. 
2. Gradually reduce the horizontal window size, looking for text outside the 
table border or for the appearance of a horizontal position slider.
3. Observe the text is outside the table.

Actual Results:  
Text is outside the table and may be missed by the user.

Expected Results:  
The text should always be within the table border.

Occurs in all Mozilla browsers, Firefox, Netscape, Mozilla.  Works fine in IE6.
Occurs without the latest MS usp10.dll module.

Text is XSLT converted into the textarea.

I have NOT been able to replicate this with a stand-alone HTML file.

This results in a missing word which is major problem for this public service 
site which is used in Biblical Hebrew classes throughout the world.

Comment 1

[dolphinling]

Not sure if this is BiDi-specific, but it definitely doesn't belong in firefox:
general...

Comment 2

[Christopher V. Kimball]

Please see much-simplified example at:

 http://www.cvkimball.com/MozillaTest/Genesis.DH.xml 

which doesn't use JavaScript or internal XSLT transformations.

To see the problem, reduce the horizontal size of the browser window until text
appears outside the table boundary on the right, as in the original description.

Comment 3

[Christopher V. Kimball]

The out-of-cell text can be eliminated by
removing one set of <span> tags and by eliminating non-Hebrew characters
inside spans in which Hebrew font is specified.

1. The original text has an initial <span> tag setting the right-to-left
direction (line 293).  Included later in this span is another span
setting the font to SBL Hebrew and font size (line 343/348).  If the second span
at line 343 is eliminated with the font and font size information being placed
with the first span, the original problem on browser line 4 disappears.

2. For some choices of horizontal width, however, some other text falls outside
the table cells.  If the 00A0's at lines 930 and 935 are placed within the <span>
tag rather than outside, the text remains in the table for all observed horizontal
widths.

A Tanach.xsl.xml file modified as above is in:

http://www.cvkimball.com/MozillaTest/Fixed/Genesis.DH.xml.

Discussion: I'm not aware of any imbedding restrictions on <span> tags.  The second
set of <span> tags occur only 6 times in the example and at one level, so it's
not likely that a counter is being over-run.

*** The file at

http://www.cvkimball.com/Tanach/TanachSearch.xml

will be repaired shortly, please use the simplified example at

http://www.cvkimball.com/MozillaTest/Genesis.DH.xml.

to explore this problem from now on.  ***

Comment 4

[Simon Montagu :smontagu]

Attachment: Minimized testcase

The text should wrap to the second line without overflowing the lime-coloured
background.

Comment 5

[Simon Montagu :smontagu]

Attachment: Minimized testcase

Comment 6

[Christopher V. Kimball]

The minimized test case doesn't demonstrate the problem.

The problem seems to depend on which characters start the line and on character
size.

Comment 7

[Uri Bernstein]

Attachment: patch

I found the the problem is that ComputeWordFragmentDimensions() is passing the
wrong (maximum) word length to tx.GetNextWord(). It's passing the length of
"this" frame, while GetNextWord is actually working on the next frame.
As a result, in a case such as <span>aa</span>bbbbb, GetNextWord() will only
return the first two "b"s (there are two "a"s in this example), which might
lead to the false conclusion that the entire word (aabbbbb) fits in the line,
when actually only aabb does.

So the main thing this patch does is to look at the content offset and length
of the next (text) frame, rather than "this" frame, when calculating a
(maximum) wordLen.

Additionally, I renamed aTextFrame to aNextFrame, to be cosistent with the
method declaration (on line 825). Although it *is* a text frame, I think the
more important information about it is that it is the *next* text frame.

I realize that casting aNextFrame to nsTextFrame* looks somewhat unsafe. It is
guaranteed to be a nsTextFrame, because it is ultimately received from
nsLineLayout::FindNextText(), which can only return a text frame (or null).
Perhaps a more correct solution would be to change the return type of
nsLineLayout::FindNextText() to nsTextFrame*, and have it do the casting?

Another question is do I have to change mState to
((nsTextFrame*)aNextFrame)->mState as well when checking for NS_FRAME_IS_BIDI.
It seems to me that if one frame is NS_FRAME_IS_BIDI than so is the other, but
I might be missing something.

Finally, I want to apologize for working on a bug which is assigned to someone
else (Simon). I only noticed this after I already did a lot of investigagtion
work, so I thought it was worth publishing my resluts anyway. Simon - if my
approach seems OK, and if you don't mind, could you please re-assign this to
me?

Comment 8

[Simon Montagu :smontagu]

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

I think it would be better to use aNextFrame->GetStateBits() and
aNextFrame->GetOffsets() rather than accessing its members directly. This also
means you don't have to bother about a cast, because GetOffsets() on a non-text
frame will just return zeros.

Comment 9

[Uri Bernstein]

Attachment: patch v. 2

Use aNextFrame->GetStateBits() and aNextFrame->GetOffsets() as suggested. I had
to expand the ?: construct into an "if" to avoid calling GetOffsets() when
NS_FRAME_IS_BIDI is off.

Comment 10

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

Comment on attachment 186367 [details] [diff] [review]
patch v. 2

nice

Comment 11

[Uri Bernstein]

Comment on attachment 186367 [details] [diff] [review]
patch v. 2

Any chance of getting this to 1.8? The fix is pretty simple and
straightforward.

Comment 12

[Simon Montagu :smontagu]

Checked in.

Comment 13

[Uri Bernstein]

Verified fixed with:
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050621
Firefox/1.0+

Comment 14

[Uri Bernstein]

*** Bug 177726 has been marked as a duplicate of this bug. ***