overflowing block with margin:auto centers if border:none (centering, width, wide, inconsistent, scrolling)

RESOLVED FIXED in mozilla1.7alpha

Status

()

Core
Layout: Block and Inline
P1
normal
RESOLVED FIXED
15 years ago
13 years ago

People

(Reporter: Ryan, Assigned: dbaron)

Tracking

Trunk
mozilla1.7alpha
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch])

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

15 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2.1) Gecko/20021130
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2.1) Gecko/20021130

http://www.darkprism.com/soul/test/centering1.html shows the unexpected behavior
http://www.darkprism.com/soul/test/centering2.html shows the expected behavior

My screen resolution: 1024x768x32 bit color

The first URL (centering1.html) renders fine when the browser is maximized. 
But, if I resize the browser window to not be maximized, it will clip off the
left side of my layout.  The smaller the window, the more of the left side that
gets clipped.  It's much easier to see than it is to explain.

The second URL (centering2.html) renders to be what I consider correct,
regardless of the size of the browser window.  Nothing gets clipped.

The difference between the two, is that centering2.html has "border: 0.04px
dashed;" in the div.header declaration.  I've tried different border widths, and
0.04 is the smallest that I can go to make the page center correctly.

Both pages are rendered correctly under IE6.

Reproducible: Always

Steps to Reproduce:
1. Load my test URLs
2. Resize the browser window
Actual Results:  
centering1.html rendered incorrectly
centering2.html rendered correctly

Expected Results:  
Both pages should have rendered correctly
Actually, it looks to me like the "unexpected behavior" is correct (the margin
should go negative) and the "expected behavior" is wrong...

Comment 2

15 years ago
Confirming.
According to <http://www.w3.org/TR/REC-CSS2/visudet.html#q6>, when both the left
and right margins are set to 'auto', they are given equal value, which centers
the element.  However, for elements that are too wide, this would mean both
margins get set to a negative value, and thus extend equally off both sides of
their containing block.

I don't see anything in the specs about a special behavoir for when the element
is too wide to fit, but it seems Mozilla does implement some sort of exception,
and it's only applied when there's a border.

It sounds like having a negative left margin in this case is correct.  One way
or another, this is inconsistent behavoir, so there's a bug here.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 3

15 years ago
Created attachment 109326 [details]
Simplified testcase

It's having a left or right border that causes the exception to the rule.
OS: Windows 2000 → All
Hardware: PC → All
*** Bug 220243 has been marked as a duplicate of this bug. ***
The WG will be examining this.
http://www.w3.org/mid/20031103011613.GA16565@darby.dbaron.org
Summary: Standards compliant CSS layout using DIV for structure incorrectly centered without a defined border → overflowing block with margin:auto centers if border:none (centering, width, wide)
Summary: overflowing block with margin:auto centers if border:none (centering, width, wide) → overflowing block with margin:auto centers if border:none (centering, width, wide, inconsistent, scrolling)
(Assignee)

Updated

14 years ago
Priority: -- → P1
(Assignee)

Comment 6

14 years ago
*** Bug 228686 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 7

14 years ago
*** Bug 191626 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 8

14 years ago
This probably has some to do with nsBlockReflowContext::AlignBlockHorizontally.
 (But it's worth noting that we do before-reflow positioning and after-reflow
positioning, and we're probably doing the latter far more often than we should
and we should make sure the former works correctly.)
(Assignee)

Comment 9

14 years ago
The before-reflow stuff is nsHTMLReflowState::CalculateBlockSideMargins
The  after-reflow stuff is nsBlockReflowContext::AlignBlockHorizontally
(Assignee)

Comment 10

14 years ago
Created attachment 139322 [details] [diff] [review]
patch to make both even with the left edge
(Assignee)

Comment 11

14 years ago
Created attachment 139324 [details] [diff] [review]
same as previous, but diff -ub
(Assignee)

Comment 12

14 years ago
I still don't understand why we are inconsistent between the border and
no-border cases.  And I also still don't understand AlignBlockHorizontally.
(Assignee)

Comment 13

14 years ago
Created attachment 139325 [details]
testcase that previous patch breaks

This is simplified from the top of http://www.apple.com/ipod/ , which is a site
that this patch otherwise fixes.  However, the top of the page moves to the
right on each reflow (this testcase -- try zooming in and out).
(Assignee)

Comment 14

14 years ago
Created attachment 139621 [details] [diff] [review]
patch to make both even with the left edge (diff -u)
Attachment #139322 - Attachment is obsolete: true
Attachment #139324 - Attachment is obsolete: true
Attachment #139325 - Attachment is obsolete: true
(Assignee)

Comment 15

14 years ago
Created attachment 139622 [details] [diff] [review]
patch to make both even with the left edge (diff -ub)
(Assignee)

Updated

14 years ago
Attachment #139622 - Flags: superreview?(bz-vacation)
Attachment #139622 - Flags: review?(roc)
(Assignee)

Updated

14 years ago
Assignee: core.layout.block-and-inline → dbaron
Whiteboard: [patch]
Target Milestone: --- → mozilla1.7alpha
(Assignee)

Comment 16

14 years ago
Comment on attachment 139622 [details] [diff] [review]
patch to make both even with the left edge (diff -ub)

Actually, the availMarginSpace < 0 case might not be quite right for tables. 
And I should probably run the regression tests...
Attachment #139622 - Flags: superreview?(bz-vacation)
Attachment #139622 - Flags: review?(roc)
(Assignee)

Comment 17

14 years ago
Created attachment 139703 [details] [diff] [review]
patch
Attachment #139621 - Attachment is obsolete: true
Attachment #139622 - Attachment is obsolete: true
(Assignee)

Comment 18

14 years ago
Comment on attachment 139703 [details] [diff] [review]
patch

This passes regression tests -- the only visible change was on:
table/marvin/table_overflow_td_align_right.html

The table-related code here is *extremely* fragile, and I don't really
understand why.
Attachment #139703 - Flags: superreview?(bz-vacation)
Attachment #139703 - Flags: review?(roc)
This looks pretty good as far as I understand it, which isn't as far as I'd like.

Here's one thing I don't understand about the table case with negative available
space: why are we just wiping out mComputedMargin? Suppose a table has a big
margin specified, which causes it to just slightly overflow its available width.
We'd blow the margin away completely which doesn't seem like the right thing to
do. Or perhaps there's something to do with table-outer-frames that I'm
neglecting? If so please explain :-).

Another thing I don't understand:
+  if (!isAutoLeftMargin && !isAutoRightMargin && !isTable) {
Why aren't we doing the overconstrained thing for tables?

I could use a comment --- or perhaps an assertion --- near the bottom reminding
that when the margin CSS value is auto, the mComputedMargin value is zero. But
maybe that's just me :-).
(Assignee)

Comment 20

14 years ago
I tried to make tables do what seemed like the right thing to me, but it broke a
whole bunch of testcases, so I gave up and changed only what I could change
without breaking a bunch of things.  (The testcases things tended to break were
attachment 139325 [details] and
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/185411-1.html
 .) 

I suspect some of the table stuff is fixed up in
nsBlockReflowContext::AlignBlockHorizontally, but I really don't understand the
whole business yet.
(Assignee)

Comment 21

14 years ago
Note that one of the things that makes tables difficult is that the width isn't
known until after reflow, so we really can't figure out the margins before reflow.
Comment on attachment 139703 [details] [diff] [review]
patch

okay
Attachment #139703 - Flags: review?(roc) → review+
(Assignee)

Comment 23

14 years ago
*** Bug 230762 has been marked as a duplicate of this bug. ***
Comment on attachment 139703 [details] [diff] [review]
patch

>Index: nsHTMLReflowState.cpp

>+    if (!isTable) {
>+      if (mStyleVisibility->mDirection == NS_STYLE_DIRECTION_LTR) {

Why is this using our mDirection instead of the parent's?  Is there a good
reason?  Of so, please comment it.

>+    } else {
>+      if (mStyleVisibility->mDirection == NS_STYLE_DIRECTION_RTL) {

Same.

In LTR mode we don't set the right margin to be negative for tables?  I guess
they'll pin to the left anyway even without that, huh?

>+  if (!isAutoLeftMargin && !isAutoRightMargin && !isTable) {

So basically, overconstraints on tables are ignored?  Again, maybe comment why?

>+    if (prs &&
>+        (prs->mStyleText->mTextAlign == NS_STYLE_TEXT_ALIGN_MOZ_CENTER ||
>+    }
>+    else if (NS_STYLE_DIRECTION_LTR == prs->mStyleVisibility->mDirection) {

We should either null-check prs consistently (preferred) or not null-check it
at all.  I'd say if prs is null we want to default to an auto right margin, no?
(Assignee)

Comment 25

14 years ago
Created attachment 140569 [details] [diff] [review]
patch

This fixes the null-check and the parent-vs.-self direction issue in one shot,
since that should have been checking its own direction, as the CSS spec says. 
I put a general comment about |isTable| at the top.
(Assignee)

Updated

14 years ago
Attachment #139703 - Attachment is obsolete: true
Comment on attachment 140569 [details] [diff] [review]
patch

sr=bzbarsky
Attachment #140569 - Flags: superreview+
Attachment #139703 - Flags: superreview?(bzbarsky)
(Assignee)

Comment 27

14 years ago
Fix checked in to trunk, 2004-02-03 21:18 -0800.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 28

14 years ago
xref bug 199385.
*** Bug 234902 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 30

13 years ago
*** Bug 231325 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 31

13 years ago
Bernd pointed out to me that one reason I may have had so much trouble with
tables is that margins for tables are sometimes NS_UNCONSTRAINEDSIZE, and I was
doing math on NS_UNCONSTRAINEDSIZE.
You need to log in before you can comment on or make changes to this bug.