Closed Bug 96394 Opened 20 years ago Closed 15 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Future

People

(Reporter: bernd_mozilla, Assigned: dbaron)

References

(Blocks 1 open bug, )

Details

(4 keywords, Whiteboard: [CSS1-4.1.2][HTML4-8.2][patch])

Attachments

(3 files, 3 obsolete files)

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.
let me check in first a patch for bug 95511 then we'll see...
hmmm... a special test case... none of the browsers (MOZ, IE, OP) displays 100%
correctly I think ....
I have filed bug 96463 about the border issue.
It's actually tables that are doing it wrong by changing their width.
Keywords: testcase
Summary: block overflow is not correct → rtl block overflow is not correct
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.
Summary: rtl block overflow is not correct → RTL block overflow is not correct when it has margin:auto
Attached file testcase (obsolete) —
the overlaping borders issue is in bug 96655
setting milestone
Target Milestone: --- → Future
wfm 2002012403 win98
the wfm 2002012403 win98 was only for the border issue, sorry
Blocks: 137995
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.
Attached patch patch (obsolete) — Splinter Review
taking the bug
Assignee: alexsavulov → bernd.mielke
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.
Attachment #88751 - Flags: needs-work+
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.
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?
Keywords: css1
Is this All/All?
Keywords: html4
Whiteboard: [CSS1-5.5.5][HTML4-8.2]
Blocks: 247691
OS: Windows NT → All
Hardware: PC → All
(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?
(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.
Assignee: bernd_mozilla → nobody
Component: Layout → Layout: Block and Inline
QA Contact: chrispetersen → core.layout.block-and-inline
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.
Whiteboard: [CSS1-5.5.5][HTML4-8.2] → [CSS1-4.1.2][HTML4-8.2]
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.
Attachment #46891 - Attachment is obsolete: true
> > 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?
Please disregard my last comment; margin: 0 is the implied default, not margin: 
auto.
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?
Depends on: 318116
No longer depends on: 6976
This appears not to depend on bug 318116 after all. Sorry for the spam.
No longer depends on: 318116
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → dbaron
Attachment #88751 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Whiteboard: [CSS1-4.1.2][HTML4-8.2] → [CSS1-4.1.2][HTML4-8.2][patch]
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.
Attachment #214947 - Flags: superreview?(roc)
Attachment #214947 - Flags: review?(roc)
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..."
Attachment #214947 - Flags: superreview?(roc)
Attachment #214947 - Flags: superreview+
Attachment #214947 - Flags: review?(roc)
Attachment #214947 - Flags: review+
Attached patch patchSplinter Review
...with comment fixed.
Attachment #214947 - Attachment is obsolete: true
Checked in to trunk.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
No longer blocks: 247691
Depends on: 331385
Depends on: 336736
Depends on: 383026
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
You need to log in before you can comment on or make changes to this bug.