423676 - weirdness with layout and selection of mixed-direction text

Status: RESOLVED FIXED

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

Description

[Joe Drew (not getting mail)]

Attachment: initial layout of the URL

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b5pre) Gecko/2008031804 Minefield/3.0b5pre

Go to the URL specified, which contains some Arabic text. For me, when I first load the URL, the third paragraph is misdrawn next to the second paragraph, but the text is unselectable. Moving to a different tab and back makes everything draw correctly.

Select the text starting after the link, as shown in selection.png. Then click on the empty area. The missing text shows up. For me, I can then repeat the action: select the text, click on the third paragraph, and it disappears.

Apologies if this is hard to understand; I'm more than willing to clarify.

Comment 1

[Joe Drew (not getting mail)]

Attachment: Weird selection

Comment 2

[Joe Drew (not getting mail)]

Attachment: after clicking on second paragraph

Comment 3

[Uri Bernstein]

This regressed between 2008-02-16 and 2008-02-17. I strongly suspect bug 392785.

I can't create a testcase because I can't reproduce this with a local file.

Comment 4

[Uri Bernstein]

Attachment: testcase

In this testcase, the word "English" should appear on its own line, below the word in Arabic.
The buggy behaviour ("English" initially appearing on the same line as the Arabic, selection weirdness later) can be triggered with this testcase in one of two ways:

1. append "#a" to the URL of the testcase, and the bug is immediately apparent. This is similar to the original bugzilla URL, and regressed, as I wrote above, on 2008-02-17.

2. Load the testcase with the regular URL (no #a), then click on the link at the top. "English" jumps up to the Arabic line. This regressed in late June 2007, probably due to the new tnsTextFrame (bug 367177).

So I suspect the real bug was introduced by bug 367177, and bug 392785 just caused it to be triggered in additional scenarios.

Comment 5

[Uri Bernstein]

Some further investigation suggests that the problem is that the second line doesn't get reflowed after bidi resolution, because it's not marked dirty by the bidi resolver, because (I think) lineNeedsUpdate isn't set when it should be.
(ref: bug 339699, bug 330373).

I'll try to continue investigating tomorrow. 

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

-> all/all

this is really layout issue.

Comment 7

[Uri Bernstein]

I compared what happens with the testcase (broken) with what happens in a similar case, in which the line right after the <a> element starts with LTR (hence-force: "the reference"). The reference does not exhibit the bug. 

In both cases, only the first line (the one with the <a> element) is marked dirty by Resolve().

In the reference case, the next line is reflowed, because it is marked dirty by nsBlockFrame::ReflowInlineFrame(). This happens because the newline textframe on the first line has its status is set to NS_FRAME_NOT_COMPLETE, in nsTextFrame::Reflow. This, in turn, happens because maxContentLength for this frame, as computed by nsTextFrame::GetInFlowContentLength(), is > 1.

In the testcase, nsTextFrame::GetInFlowContentLength() returns 1 (because there's a bidi continuation starting right after the newline), the line status is therefore set to NS_FRAME_COMPLETE, the next line isn't marked dirty, and is therefore not reflowed, and hence everything afterwards remains on the same line.

So, I'm still not sure where this should be fixed. Either Resolve() should mark the second line dirty, or the logic for setting the line's status to NS_FRAME_NOT_COMPLETE in nsTextFrame::Reflow should be changed. I tend to think it should be the former, but on the other hand, the bidi code as it is used to work fine with the old nsTextFrame.

Roc, dbaron - can you offer some help?

Comment 8

[Uri Bernstein]

Attachment: patch

This fixes it, but might negate some (or most) of the performance benefit from bug 330373 (assuming there actually was benefit).

Requesting reviews from the reviewers of bug 330373, although if either of you (or dbaron) are comfortable with reviewing it alone, I don't mind, of course.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

I'm not comfortable reviewing this at all, if the review is needed in the next few weeks.  I just don't recall this code well enough.

Comment 10

[Uri Bernstein]

(In reply to comment #9)
> (From update of attachment 310648 [details] [diff] [review])
> I'm not comfortable reviewing this at all, if the review is needed in the next
> few weeks.  I just don't recall this code well enough.
> 

I doubt if there is anyone who recalls this code any better (you guided me through bug 330373), but perhaps roc will be able to help.

Comment 11

[Boris Zbarsky [:bzbarsky]]

I'm kind of hoping he will; I'm just totally swamped right now...  Finding time to sort this out (esp. the perf implications) is very unlikely.

Comment 12

[Uri Bernstein]

Simon might also be able to help, of course.

Comment 13

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

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

Looks good to me, but needs Simon

Comment 14

[Simon Montagu :smontagu]

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

Looks OK to me.

Comment 15

[Mike Schroepfer]

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

a+ schrep

Comment 16

[Reed Loden [:reed]]

Checking in layout/base/nsBidiPresUtils.cpp;
/cvsroot/mozilla/layout/base/nsBidiPresUtils.cpp,v  <--  nsBidiPresUtils.cpp
new revision: 1.115; previous revision: 1.114
done
Checking in layout/base/nsBidiPresUtils.h;
/cvsroot/mozilla/layout/base/nsBidiPresUtils.h,v  <--  nsBidiPresUtils.h
new revision: 1.29; previous revision: 1.28
done

Comment 17

[Uri Bernstein]

I need to make a reftest for this.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #17)
> I need to make a reftest for this.

Note that after bug 421353, the selecting will not rise reflow. So, the selection changing is not a good trigger for testing the regression.

Comment 19

[Uri Bernstein]

I checked in a reftest that doesn't require focusing links, or underlines.