318116 - Overflowing content in fixed-width RTL blocks overflows to the right instead of to the left

Status: RESOLVED FIXED

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

Description

[Uri Bernstein]

When an RTL block with a specified width contains a line of text or an image, which is wider than the width of the block, the text or image overflows to the right of the containing block, instead of to the left.

If the overflowing content is itself a block with a specified width, the problem only occurs if the inner block has "margin:auto;" (see bug 96394).

Bug 202386 also seems like a special case of this bug (where the containing block is the body element, and the overflowing content is a table).

The example in the URL field is a very complex testcase. If you reduce the window width so that the image does not fit, it will overflow to the right and cover the menus.
I'll attach a much simpler testcase soon.

Comment 1

[Uri Bernstein]

Attachment: testcase

The first two blocks demonstrate the problem with overflowing text (they behave the same, regardless of the directionality of the text itslef).
The third block demonstrates the same problem with an image.
The fourth block has a fixed-width DIV as content, and does not exhibit the problem.
The fifth block is the same as the fourth, but with "margin:auto;", causing the problem to re-appear.

Comment 2

[Uri Bernstein]

Attachment: testcase with overflow:auto

This is the same testcase, but with overflow:auto on the container divs. The problem basically remains, but notice:
- The scroll thumbs are initially positioned incorrectly (the divs themselves appear scrolled all the way to the right, but the scrollbars indicate that they are somewhat to the left)
- The fourth block, which seemed to be OK in the previous testcase, lacks a scrollbar altogether (this might be bug 6976).

Comment 3

[Bernd]

This is probably not a bidi problem but more that we can't scroll left if content overflows there.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If we fix this then in containers with scrollbars we won't be able to scroll to the left-overflowing content.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

... unless of course we make it possible to scroll to the left of the origin, which should indeed be possible, and probably wouldn't be that hard as it would only touch nsGfxScrollFrame.cpp and nsScrollPortView.cpp, I think. Just make scroll offset (0,0) correspond to the top-left of the scrolled frame's overflow area instead of the scrolled frame's origin.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

But you'd need to make sure that the default scroll position had the child scrolled so the child's origin is at the scrolled frame's origin.

Comment 7

[Eyal Rozenberg]

I'm having trouble figuring out the exact relationships between this bug, bug 6976 and bug 192767. Are they all separate issues or are some of them dupes of others? What blocks what? etc. 

In comment 0 I'm not reading anything about scrolling yet the discussion seems to revolve around that.

Comment 8

[Uri Bernstein]

(In reply to comment #7)
> I'm having trouble figuring out the exact relationships between this bug, bug
> 6976 and bug 192767. Are they all separate issues or are some of them dupes of
> others? What blocks what? etc.  

As far as I understand, this is a separate bug, which is not directly related to scrolling (hence no scrollbars mentioned in comment #0 or in the first testcase). However, as ROC pointed out in comment #4, fixing this bug will result in additional scrolling problems, which, I think, are really the same as bug 6976 / bug 192767 (which I think are actually dupes of each other).

ROC - is this accurate? If so, should this bug be marked as depending on bug 6976?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

correct, and yes.

Comment 10

[lɛʁi לערי ריינהארט]

one question
please resize the width of
http://egytopia.wikicities.com/index.php?title=%D8%B9%D9%84%D9%89_%D8%AE%D9%84%D9%81%D9%8A%D8%A9_%D8%A3%D8%B2%D9%85%D8%A9_%D9%85%D8%AD%D8%B1%D9%85_%D8%A8%D9%83&oldid=74
The images will overlap with the "adds" area.

Is this the same bug? If not which one?

Thanks for your feedback in advance.
best regards reinhardt [[user:gangleri]]

Comment 11

[Uri Bernstein]

Attachment: patch

This patch fixes the top three divs in the attached testcase (the fifth one is probably a separate issue - bug 96394).

As roc noted in comment #4, scrollbars are indeed missing in the overflow:hidden case (and in the textarea case). However, applying dbaron's patch for bug 192767 fixes this problem.

The line I removed from nsLineLayout::PlaceFrame() was added there as part of the fix for bug 169620. I don't really understand why it had to be added there (frankly, I don't really understand a lot of this code), but removing it does not regress that bug. I had to remove it in order to be able to detect and handle the overflow condition in nsLineLayout::HorizontalAlignFrames().

I removed "// XXXldb What if it's less than 0??" because I'm now handling that case (although, apparently it only needs handeling in the RTL case). Note that before I removed the MAX(...) line from nsLineLayout::PlaceFrame(), this could not happen: remainingWidth was never negative.

Comment 12

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

The patches to bug 192767 (scrolling to right), bug 96394 (block overflow should go to right), and bug 318116 (inline overflow should go to right) should all be landed at about the same time.

Comment 13

[Hideo Saito]

Attachment: patch with padding

I'm sorry, uri.  I posted this patch since it seems that a problem is shown in the display of left-padding. If your patch is corrected, I think that it is good.

Comment 14

[Hideo Saito]

Attachment: testcase for padding

Comment 15

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

Does mComputedBorderPadding already account for GetSkipSides, or do you need to check that?

Comment 16

[Uri Bernstein]

(In reply to comment #13)
> Created an attachment (id=214984) [edit]
> patch with padding
> 
> I'm sorry, uri.  I posted this patch since it seems that a problem is shown in
> the display of left-padding. If your patch is corrected, I think that it is
> good.
> 

Not that I mind, but as far as I can tell, the testcase you posted looks exactly the same with your patch as with mine (and seems correct in both cases).

Comment 17

[Hideo Saito]

(In reply to comment #16)
> Not that I mind, but as far as I can tell, the testcase you posted looks
> exactly the same with your patch as with mine (and seems correct in both
> cases).

Really? I am sorry, since I tested using source code which is not latest.

Comment 18

[Uri Bernstein]

Checked in:
Checking in layout/generic/nsLineLayout.cpp;
/cvsroot/mozilla/layout/generic/nsLineLayout.cpp,v  <--  nsLineLayout.cpp
new revision: 3.231; previous revision: 3.230
done

Comment 19

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).