Closed Bug 412093 Opened 14 years ago Closed 14 years ago

background images messed up on non-trivial bidi inlines

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: uriber, Assigned: uriber)

References

Details

(Keywords: regression, testcase)

Attachments

(6 files, 3 obsolete files)

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.
Attached patch WIP (obsolete) — Splinter Review
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.
Attached patch patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → uriber
Attachment #296861 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #297096 - Flags: review?(roc)
Depends on: 400057
Attached file another testcase
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.
Flags: blocking1.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.
Attachment #297096 - Attachment is obsolete: true
Attachment #297096 - Flags: review?(roc)
Attached patch patch v2Splinter Review
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).
Attachment #297226 - Flags: review?(roc)
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.
Attachment #298018 - Flags: review?(roc)
Comment on attachment 298018 [details] [diff] [review]
also fix asymmetry

+        if((isRtlBlock && frameXOffset > curOffset) ||
+           (!isRtlBlock && frameXOffset < curOffset)) {

isRTLBlock == (frameXOffset > curOffset)
Attachment #298018 - Flags: superreview+
Attachment #298018 - Flags: review?(roc)
Attachment #298018 - Flags: review+
Attachment #298018 - Flags: approval1.9+
Have you tested this with the different values of -moz-background-inline-policy?  http://developer.mozilla.org/en/docs/CSS:-moz-background-inline-policy
Flags: blocking1.9? → blocking1.9+
Priority: -- → P4
(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.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Sorry, I completely forgot this depends on the fix for 400057 going in first.
Status: REOPENED → ASSIGNED
Attached patch reftest v1 (obsolete) — Splinter Review
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.
Attachment #299819 - Flags: review?(smontagu)
Priority: P4 → P3
Attached patch reftest v2Splinter Review
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?)
Attachment #299819 - Attachment is obsolete: true
Attachment #299819 - Flags: review?(smontagu)
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...
Status: ASSIGNED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Thanks, Rob! I'll check in the reftest probably later today.
Reftest checked in.
Flags: in-testsuite? → in-testsuite+
Depends on: 421203
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: layout.bidi → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.