96394 - RTL block overflow is not correct when it has margin:auto

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Bernd]

This is an spinoff from bug 76726. The testcase attached there
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=45737 shows two problems 
- first the border is not handled  correctly for the div ( I think this is a
size issue, the border should be substracted for the inner content of the block
- the rtl attribute is not honored.

Tables are doing this right now. But if  you look on the patch applied for bug
76726 the right fix for block layout should be somewhere in the vicinity.

Comment 1

[Bernd]

The rtl problem is in http://bugzilla.mozilla.org/showattachment.cgi?attach_id=45758

Comment 2

[Alexandru Savulov]

let me check in first a patch for bug 95511 then we'll see...

Comment 3

[Alexandru Savulov]

hmmm... a special test case... none of the browsers (MOZ, IE, OP) displays 100%
correctly I think ....

Comment 4

[Alexey Chernyak]

I have filed bug 96463 about the border issue.
It's actually tables that are doing it wrong by changing their width.

Comment 5

[Bernd]

Attachment: more colors to show the div border overlap

Comment 6

[Alexey Chernyak]

Ah I see, you were talking about different border problem. The testcase I wrote
uncovered at least 6 bugs in Mozilla, and it gets rather confusing.

Let's leave this bug just for the direction of overflow.
I will file separate bugs for all other issues, and write simplified testcases
which target each specific problem.
By the way the problem happens only when either (or both) nested div's
horisontal margins are set to auto.
attaching simplified testcase.

Comment 7

[Alexey Chernyak]

Attachment: testcase

Comment 8

[Alexey Chernyak]

the overlaping borders issue is in bug 96655

Comment 9

[Alexandru Savulov]

setting milestone

Comment 10

[Bernd]

wfm 2002012403 win98

Comment 11

[Bernd]

the wfm 2002012403 win98 was only for the border issue, sorry

Comment 12

[Bernd]

The problem here is in my oppinion at 

// Given the width of the block frame and a suggested x-offset calculate
// the actual x-offset taking into account horizontal alignment. Also returns
// the actual left and right margin
void
nsBlockReflowContext::AlignBlockHorizontally(nscoord                 aWidth,
                                             nsBlockHorizontalAlign &aAlign)

first the auto-margins are set back to 0
then the remaining space is computed, which is negative.
As a result the function bails out, whithout correcting the left margin.

Comment 13

[Bernd]

Attachment: patch

Comment 14

[Bernd]

taking the bug

Comment 15

[David Baron :dbaron:]

Comment on attachment 88751 [details] [diff] [review]
patch

It looks like what you really want to do here is change the |remaningSpace > 0|
check to a |remainingSpace != 0|.  However, I don't think we're ready to do
that until we have a mechanism for scrolling to the left (or top) of the page
origin.  I think there's a bug on that somewhere, and if we don't fix it first,
then doing this will lead to pages having inaccessible text.

Comment 16

[Bernd]

No, I don't want to change it to |remainingSpace != 0|, if I would do that the
left margin would be < 0 under ltr conditions. I am 100% certain that it would
break the layout regression tests, which I can't do for personal reasons.
The intention of this patch is under margin:auto conditions: as for the ltr
conditions when the left margin goes to 0; to bring the right margin under rtl
conditions down to 0 and not the left one. This happens  already when the
margin:auto is not set. The not scrolling is a different bug and I dont see that
as long as I limit the scope of the patch that your argument applies.

Comment 17

[David Baron :dbaron:]

Well, anything other than the change I suggested would be violating the CSS1
spec.  It wouldn't break very many regression tests, and the tests it would
break would be corrections.  However, I don't want to do that until we have
right/top scrolling working, which is a problem equally for the centering and
for the default case.

Why is it that we're doing different things for an explicit left margin of auto
and an assumed one?

Comment 18

[Greg K.]

Is this All/All?

Comment 19

[Eyal Rozenberg]

(In reply to comment #17)
> Well, anything other than the change I suggested would be violating the CSS1
> spec. 

I know this reply comes 2 years after your comment, David, but could you be more
specific about the possible CSS 1 violation?

> It wouldn't break very many regression tests, and the tests it would
> break would be corrections.

Which is a good thing, right?

> However, I don't want to do that until we have
> right/top scrolling working, which is a problem equally for the centering and
> for the default case.

Seeing how 2 years do not seem to have brought any progress, should this
restriction still be in effect?

> 
> Why is it that we're doing different things for an explicit left margin of 
> auto and an assumed one?

Anybody have an answer to this question? Bernd?

Comment 20

[David Baron :dbaron:]

(In reply to comment #19)
> > However, I don't want to do that until we have
> > right/top scrolling working, which is a problem equally for the centering and
> > for the default case.
> 
> Seeing how 2 years do not seem to have brought any progress, should this
> restriction still be in effect?

Yes.

Comment 21

[Eyal Rozenberg]

The relevant CSS-1 section is 4.1.2, not 5.5.5; the problem is not with the
syntax of the margin command but rather with the horizontal formatting based on
the 'auto' value.

Comment 22

[Eyal Rozenberg]

Attachment: testcase without second margin:auto

The container's margin:auto (whose effect was to center the container in the
viewport) is somewhat confusing, so in order for it not to be interpreted as
part of the problem I have removed it. I also changed the internal element's
border color.

Comment 23

[Eyal Rozenberg]

> > Seeing how 2 years do not seem to have brought any progress, should this
> > restriction still be in effect?
> 
> Yes.

That's a bit laconic... how about this: suppose someone were to write a patch
with which the behavior with specific "margin: auto;" would be the same as the
behavior with the "margin: auto" implied by default (it _is_ implied by default,
right?). Would that be acceptable regardless of the scrolling issue?

Comment 24

[Eyal Rozenberg]

Please disregard my last comment; margin: 0 is the implied default, not margin: 
auto.

Comment 25

[Eyal Rozenberg]

I have a new question:

in nsBlockReflowContext::AlignBlockHorizontally() , why does the calculation of
the remaining horizontal space not take the left margin into account? Does the
suggested x-offset already include it?

Comment 26

[Uri Bernstein (Google)]

This appears not to depend on bug 318116 after all. Sorry for the spam.

Comment 27

[David Baron :dbaron:]

Attachment: patch

Comment 28

[David Baron :dbaron:]

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 29

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

Comment on attachment 214947 [details] [diff] [review]
patch

+        // But only use this value for too-narrow cases, not for
+        // too-wide cases.

"for cases where the content is narrower than the container, not..."

Comment 30

[David Baron :dbaron:]

Attachment: patch

...with comment fixed.

Comment 31

[David Baron :dbaron:]

Checked in to trunk.

Comment 32

[(no longer active)]

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