In a paragraph with mixed-direction text and style applied to :first-line, the style may be applied to some parts of later lines. This is a regression, and doesn't happen on the 1.8 branch.

Experiment with different window widths to see different applications of the bug.

This regressed between builds 2006080610 and 2006080710. Bug 169139 seems to be a likely candidate. A different bug with :first-line was corrected within the same window: before that rtl paragraphs with :first-line were display aligned right but with ltr direction. See http://www.mozilla.org.il/board/viewtopic.php?p=21199 (Hebrew).

The problem here is that code in nsFirstLineFrame (e.g., [1], but probably in other places as well) assumes that "the frame has no prev-in-flow" means the same as "this frame is the on the first line". That was true before bug 299065 and bug 169139, but is no longer, because nsFirstLineFrames are now subject to bidi-splitting. The quick-and-dirty solution would be to avoid bidi-splitting nsFirstLineFrames (by making them report they're not of type eBidiInlineContainer). That fixes this bug, but also breaks caret movement, because bidi caret navigation code now relies on the fact that all inline frames are bidi-split [2]. So we'll have to find a better way for nsFirstLineFrame to determine whether it's on the fist line or not, and use it in all applicable places. This seems difficult, but I'll try to work on it sometime. Hmmm... thinking again, I might be able to change the code in [2] to test for a non-eBidiInlineContainer frame instead of for a block frame. Perhaps I'll try that first. [1] http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsInlineFrame.cpp&rev=3.266&mark=989#988 [2] http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsFrameList.cpp&rev=3.42&mark=449#447

I decided that not splitting nsFirstLineFrames is the right way to go, but doing this is a bit more complicated than I originally thought. This patch seems to work, but is ugly, and needs more work to make sure line frames are handled correctly everywhere. It might be better to split eBidiInlineContainer into two separate frame types (one for "not bidi leaf" and the other for "bidi-splittable"), where nsFirstLineFrame is of the former type, but not of the latter. I also note that I lied to Boris in bug 169139 comment 11 (the first answer), probably because I didn't understand what line frames really are.

Don't bidi-split line frames (but still consider them of type eBidiInlineContainers). This requires special-casing line frames when reordering lines, and when walking the frame tree visually.

Updated for bug 355300.

I think I'll need a bit of handholding to review this. What does the change to nsBidiPresUtils::ReorderFrames do? Why the nsFrameList changes?

(In reply to comment #7) > What does the change to nsBidiPresUtils::ReorderFrames do? nsBidiPresUtils::ReorderFrames is called with aFirstFrameOnLine pointing to a direct child of a block frame. Usually only direct children of block frames need to be reordered using the bidi algorithm, because the children of each one of them are guaranteed to be of the same bidi level (and therefore can be reordered trivially). nsLineFrame is now a special case in that it is not subject to bidi-splitting, and therefore its children can be of different bidi levels, and require reordering. This is done *instead* of reordering the lineframe with its on-line siblings, because presumably there is only one lineframe on the line (so no on-line siblings). > Why the nsFrameList changes? The code in Get[Prev|Next]VisualFor requires calling GetFrameTo[Right|Left]Of when the order of the frames is not trivial to find out. Again, this was, until now, true only for direct children of block frames (descendants of any these frames are guaranteed to have the same bidi level as each other). Now that not only block frames, but also line frames can have bidi-reordered children, we need to call GetFrameTo[Right|Left]Of for their direct children as well. However, we don't need (and can't easily use) the line-aware code that does this for children of block frames.

Comment on attachment 250564 [details] [diff] [review] patch v1.1 (diff -w) >Index: mozilla/layout/base/nsBidiPresUtils.cpp > nsBidiPresUtils::ReorderFrames(nsPresContext* >+ aNumFramesOnLine = -1; Why is -1 ok here? r=bzbarsky with that commented in the code.

Comment on attachment 250564 [details] [diff] [review] patch v1.1 (diff -w) I'll add a comment explaining that all children of the lineframe are on the same line, so there is no need to limit on the number of frames.

Comment on attachment 250564 [details] [diff] [review] patch v1.1 (diff -w) Sounds good.

I'm mostly offline this week, so I'll probably check this in sometime during the weekend.

Checked in: Checking in mozilla/layout/base/nsBidiPresUtils.cpp; /cvsroot/mozilla/layout/base/nsBidiPresUtils.cpp,v <-- nsBidiPresUtils.cpp new revision: 1.89; previous revision: 1.88 done Checking in mozilla/layout/generic/nsFrameList.cpp; /cvsroot/mozilla/layout/generic/nsFrameList.cpp,v <-- nsFrameList.cpp new revision: 3.43; previous revision: 3.42 done