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

RESOLVED FIXED in Future

Status

()

RESOLVED FIXED
18 years ago
11 years ago

People

(Reporter: bernd_mozilla, Assigned: dbaron)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
Future
css1, html4, rtl, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

18 years ago
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.
(Reporter)

Comment 1

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

Comment 2

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

Comment 3

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

Comment 4

18 years ago
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
(Reporter)

Comment 5

18 years ago

Comment 6

18 years ago
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

Comment 7

18 years ago
Posted file testcase (obsolete) —

Comment 8

18 years ago
the overlaping borders issue is in bug 96655

Comment 9

17 years ago
setting milestone
Target Milestone: --- → Future
(Reporter)

Comment 10

17 years ago
wfm 2002012403 win98
(Reporter)

Comment 11

17 years ago
the wfm 2002012403 win98 was only for the border issue, sorry
Blocks: 137995
(Reporter)

Comment 12

17 years ago
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.
(Reporter)

Comment 13

17 years ago
Posted patch patch (obsolete) — Splinter Review
(Reporter)

Comment 14

17 years ago
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+
(Reporter)

Comment 16

17 years ago
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

Comment 18

17 years ago
Is this All/All?
Keywords: html4
Whiteboard: [CSS1-5.5.5][HTML4-8.2]
OS: Windows NT → All
Hardware: PC → All

Comment 19

15 years ago
(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.
(Reporter)

Updated

15 years ago
Assignee: bernd_mozilla → nobody
Component: Layout → Layout: Block and Inline
QA Contact: chrispetersen → core.layout.block-and-inline

Comment 21

15 years ago
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]

Comment 22

15 years ago
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

Comment 23

15 years ago
> > 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

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

Comment 25

15 years ago
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
This appears not to depend on bug 318116 after all. Sorry for the spam.
No longer depends on: 318116
Posted 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.
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+
Checked in to trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
No longer blocks: 247691
Depends on: 336736
Depends on: 383026

Comment 32

11 years ago
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.