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.

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.

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.

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.

-> all/all this is really layout issue.

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?

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 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.

(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.

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.

Simon might also be able to help, of course.

Comment on attachment 310648 [details] [diff] [review] patch Looks good to me, but needs Simon

Comment on attachment 310648 [details] [diff] [review] patch Looks OK to me.

Comment on attachment 310648 [details] [diff] [review] patch a+ schrep

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

I need to make a reftest for this.

(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.

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