121633 - [bidi]reflowinlineframes accumulates margins on the right side.

Status: RESOLVED FIXED

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

Description

[Bernd]

see attached testcase, under rtl conditions the images are not separated. The
picture should be symmetric. Use IE to see the correct rendering. 2002-01-18-03

Comment 1

[Bernd]

Attachment: testcase

Comment 2

[Simon Montagu :smontagu]

Accepting. This looks like a similar problem to bug 108187.

Comment 3

[Chris Waterson]

Interesting. The patch in bug 46918 does not fix the problem, unfortunately. It
looks like we're not accounting for _any_ left margin; i.e., removing
`margin-right' altogether shows no space between the two images.

Comment 4

[Chris Waterson]

The problem here is that we're not including the margins in
nsLineLayout::HorizontalAlignFrames. I'll include this change in the patch for
bug 46918.

Comment 5

[Chris Waterson]

Well, I was wrong! I think that we do need to make some changes to
nsLineLayout::HorizontalAlignFrames, which I'll attach, but the problem is
deeper than that. Specifically, I think the code in
nsBidiPresUtils::RepositionInlineFrames is going to need some serious
re-thinking, as it needs to account for margins during re-ordering: margins may
be arbitrarily nested through spans containing the text frames. It may make the
most sense to try to incorporate this logic with the reflow instead of trying to
do it out-of-line.

Comment 6

[Chris Waterson]

Attachment: partial fix

The above patch may be a nudge in the right direction. Or it may not! ;-)

Comment 7

[Chris Waterson]

Attachment: oops, last patch missed a chunk

Comment 8

[Chris Waterson]

Attachment: argh!

Sigh. Attached wrong patch above.

Comment 9

[Eyal Rozenberg]

*** Bug 258928 has been marked as a duplicate of this bug. ***

Comment 10

[Eyal Rozenberg]

See dupe bug 258928 for a testcase with span elements. This bug is also
manifested with padding rather than margins.

Comment 11

[Simon Montagu :smontagu]

*** Bug 266194 has been marked as a duplicate of this bug. ***

Comment 12

[Eyal Rozenberg]

An ignorant's question regarding the patch: Is it just me or is the file no
longer in the same  place and no longer with the same content?

Comment 13

[Bernd]

http://groups-beta.google.com/group/netscape.public.mozilla.layout/browse_thread/thread/d988d51486493e9f/4e59d088c1e4ad41?q=dbaron+layout+files&rnum=3&hl=en#4e59d088c1e4ad41

Comment 14

[Eyal Rozenberg]

Comment on attachment 66991 [details] [diff] [review]
argh!

nsLineLayout.cpp has moved to layout/generic/ and its contents has changed.

Comment 15

[Haamed Gheibi]

i debugged the source code, it seems that margin-left isn't applied to RTL inline-frames at all. (width of frame is equal to width of content + margin-right)
i commented out nsBidiPresUtils::RepositionInlineFrames() to ensure it's not reordered, but the output does not have left margins still.
in nsLineLayout::ApplyStartMargin() we have:

    if (ltr)
        pfd->mBounds.x += pfd->mMargin.left;

with no else for LTR.

Comment 16

[Mohsen Naderi]

Attachment: a LONG! patch

Hi,
  please take a look at the patch. We beleive that the problem is in the algorithm since it could not be done top down. We need to have a bottom-up method instead. We will submit a document describing the problem and our solution soon.

Thanks to Haamed to his hardwork

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So, for what it's worth, I'm a bit scared of pages that mess with inline frames for Bidi reasons right now -- at least until bug 299065 lands.  Bug 299065 is needed so that a unique directionality can be associated with each inline frame.

Even once that happens, I'm not entirely sure what 'margin-left' and 'margin-right' should mean on inlines that have complex directionality, although I suspect the answer is that it should depend on 'direction':  that is, for 'direction:ltr' ['direction:rtl'] frames, the 'margin-left' ['margin-right'] should be on the left [right] edge of the first logical part of the frame and 'margin-right' ['margin-left'] on the right [left] edge of the last logical part of the frame.

What did this patch implement?

Comment 18

[Uri Bernstein]

(In reply to comment #17)
> Even once that happens, I'm not entirely sure what 'margin-left' and
> 'margin-right' should mean on inlines that have complex directionality,
> although I suspect the answer is that it should depend on 'direction':  that
> is, for 'direction:ltr' ['direction:rtl'] frames, the 'margin-left'
> ['margin-right'] should be on the left [right] edge of the first logical part
> of the frame and 'margin-right' ['margin-left'] on the right [left] edge of the
> last logical part of the frame.

I think http://www.w3.org/TR/CSS21/box.html#q11 (section 8.6) disagrees with you here. It says the "left-most" (or "right-most") part is the one getting the margins, paddings, and borders, not the first/last logical parts. I think that actually makes sense (although perhaps more difficult to implement).

Thanks to smontagu for providing me with that reference.

Comment 19

[Eyal Rozenberg]

Just setting the flags correctly.

BTW, it's not such a long patch...

Comment 20

[Uri Bernstein]

(In reply to comment #17)
> So, for what it's worth, I'm a bit scared of pages that mess with inline frames
> for Bidi reasons right now -- at least until bug 299065 lands.  Bug 299065 is
> needed so that a unique directionality can be associated with each inline
> frame.

Forgot to say this: I'm actually re-writing this very code (nsBidiPresUtils::ReorderFrames()) as part of my patch for bug 299065 (see bug 299065 comment 25). While my patch doesn't fix margins (i.e., this bug), it does fix borders and paddings (bug 237370 and bug 258928), modulo bug 299063 and the issues mentioned in comment 17 and 18 here.
So I'm joining dbaron's recommendation to wait with this until after bug 299065 is fixed.

Mohsen and Haamed, it seems like bad luck that you guys worked on this at the very same time I started working on bug 299065. I will look at your patch to see if it gives me ideas for fixing the margin issues after 299065 is fixed.

Comment 21

[Haamed Gheibi]

here is a document about bug:
http://ccsv.lsbu.ac.uk/~mnaderi/mozilla/121633/reordering.html

As Uri said, w3.org has cleared what should be done about margins and paddings.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That part of the spec is relatively new; if we think it's wrong, it's definitely open to change, especially if current implementations don't interoperably agree with it.

Comment 23

[Mohsen Naderi]

HI everybody,
Our documentation for the patch is available at:
  http://www.metanetworking.com/Mozilla/121633/reordering.html

The previous link does not work.

Comment 24

[Uri Bernstein]

Fixed by Haamed's patch for bug 328168.