This is a follow-up on bug 411046, which, as it turns out, fixed only a special case of background images on multi-frame bidi inlines: the case where the frames are all laid out from left to right. However, when going beyond this trivial case, things are still messed up, because the background image chunks are applied to the frames in logical order, rather than visual order (so e.g. the leftmost portion of the image is always applied to the logical beginning of the text, even if its not the leftmost one). Testcase attached

In some situations logical order would be what you want. Does any spec say anything about this? I had a quick look at the CSS2.1 spec and didn't see anything.

As I see it, the fact that we break the inline element into frames is an implementation detail. Someone setting a background image on that element would not expect that image to be chopped up into pieces, regardless of the text that happens to be contained in it. Additionally, our current behavior is not shared by any other browser I know of, and breaks at least one real-world WordPress theme (see http://www.2jk.org/praxis/ - notice the positioning of the little calendar and "responses" icons). I think I might have an idea of how to hack this up in InlineBackgroundData: keep track of the leftmost frame on the current line together with its "continuation point", and use that point + the offset from that frame to the current one for positioning the current frame's frame's background. When we detect we're on a new line, find this line's leftmost frame, and set its "continuation point" to mContinuationPoint (which we keep accumulating as now, but not using otherwise) ... or something like that. There's still an inherent left/right asymmetry here, but I think other browsers have that as well (at least Safari does), and it's much less of a problem, I think.

This fixes some of the cases, but not others (specifically, the "comments" icon on the site linked above is broken/missing - I don't have time now to check whether this is due to a simple bug or to a totally flawed design). If this ever becomes a real patch, we'll probably want to limit the ugly hacks to the actually-bidi cases. We'll also need a whole bunch of testcases to make sure this doesn't break anything. I hope to be able to get back to this sometime this week, but I can't make much promises, so if someone wants to take this from here, that would be great.

This fixes two significant bugs in the previous patch: 1. Frame offsets are measured relative to the containing block, not to the parent. 2. Determining whether a continuation is on the same line as the "current" frame is done (in the case of a non-fluid continuation) using nsBlockInFlowLineIterator. Additionally, the new code is now only executed if PresContext()->BidiEnabled() is true. I can also put the changes in #ifdef IBMBIDI, if that's desired. This patch uses code from the patch in bug 400057.

Some more tests. You can see the LTR/RTL asymmetry here. While I believe it's a bug, it is not a regression from 1.8, and also exists in Safari 3.0 (need to check IE too).

This is a visible regression from 1.8, so it should probably block 1.9.

+ rv = frame->QueryInterface(kBlockFrameCID, (void**)&mBlockFrame); nsLayoutUtils::GetAsBlock + if(frameXOffset < mLineLeftOffset) { + mLineLeftOffset = frameXOffset; + } Use PR_MIN Maybe put the bodies of the while loops in a helper function? > (aFrame->GetOffsetTo(mBlockFrame).x - mLineLeftOffset) Will this actually work? is it guaranteed that the frames of an inline frame on the same line are visually contiguous? My bidi-fu is weak.

Comment on attachment 297096 [details] [diff] [review] patch v1 (In reply to comment #7) > Will this actually work? is it guaranteed that the frames of an inline frame on > the same line are visually contiguous? My bidi-fu is weak. You're right, of course. The frames are not necessarily contiguous, and this doesn't work if they aren't.

This takes care of the non-contiguous frames issue. The down side is, of course, that it is O(n^2) (where n is the number of bidi continuations on the line). I can't see an easy way around this. I don't see the value of extracting the content of the two while loops into a function in this patch, as that would be a three-line function with an i/o parameter (yuck).

This also fixes the asymmetry issue mentioned above: In an RTL block, the fragments of background image taken for each line should go from right to left (i.e., the first line should have the rightmost, not leftmost, fragment as its background). While the extra issue I'm fixing here was present in 1.8 (and is present in Safari), it is undoubtedly a bug. I originally thought to file a separate bug on this after the previous patch went in, but since it involves exactly the same code, I'd now rather piggyback on this one, if you don't mind.

Comment on attachment 298018 [details] [diff] [review] also fix asymmetry + if((isRtlBlock && frameXOffset > curOffset) || + (!isRtlBlock && frameXOffset < curOffset)) { isRTLBlock == (frameXOffset > curOffset)

Have you tested this with the different values of -moz-background-inline-policy? http://developer.mozilla.org/en/docs/CSS:-moz-background-inline-policy

(In reply to comment #13) > Have you tested this with the different values of > -moz-background-inline-policy? > http://developer.mozilla.org/en/docs/CSS:-moz-background-inline-policy > The patch only affects the codepath used for "continuous" (the default policy). I verified that it doesn't affect the rendering when using either of the other policies. Arguably, there's still a bug with -moz-background-inline-policy:each-box - the background is applied separately to each bidi continuation, rather than being applied once to each visually-contiguous box. However, that's very minor. not a regression, and not affected by this patch.

(In reply to comment #12) > (From update of attachment 298018 [details] [diff] [review]) > + if((isRtlBlock && frameXOffset > curOffset) || > + (!isRtlBlock && frameXOffset < curOffset)) { > > isRTLBlock == (frameXOffset > curOffset) > I did s/>/>=/ before submitting. Checking in mozilla/layout/base/nsCSSRendering.cpp; /cvsroot/mozilla/layout/base/nsCSSRendering.cpp,v <-- nsCSSRendering.cpp new revision: 3.338; previous revision: 3.337 I'll try to create a reftest sometime soon.

You checked in with orange unit tests, and also you caused bustage.

(In reply to comment #16) > You checked in with orange unit tests, and also you caused bustage. > Sorry, I'll revert in a minute.

Sorry, I completely forgot this depends on the fix for 400057 going in first.

Attempt at reftest. This was surprisingly difficult, and I'm not too happy about the result, which is very fragile (I don't understand why I need that half-pixel padding-bottom on the paragraphs, for example). Simon - if you have a better idea how to do this, please let me know.

Updated reftest. The 0.5px thing is no longer necessary, thanks to the fix for bug 400813. Also, use classes instead of ids in the reference (what was I thinking?)

Checked in. Please check in the testcase when you get a chance, I'm juggling too many things in my tree at the last minute here...

Thanks, Rob! I'll check in the reftest probably later today.

Reftest checked in.