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

Status: RESOLVED FIXED

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

Description

[Ryan]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

Actually, it looks to me like the "unexpected behavior" is correct (the margin
should go negative) and the "expected behavior" is wrong...

Comment 2

[Brian Rogers]

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.

Comment 3

[Brian Rogers]

Attachment: Simplified testcase

It's having a left or right border that causes the exception to the rule.

Comment 4

[Hixie (not reading bugmail)]

*** Bug 220243 has been marked as a duplicate of this bug. ***

Comment 5

[Hixie (not reading bugmail)]

The WG will be examining this.
http://www.w3.org/mid/[e-mail]

Comment 6

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

*** Bug 228686 has been marked as a duplicate of this bug. ***

Comment 7

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

*** Bug 191626 has been marked as a duplicate of this bug. ***

Comment 8

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

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.)

Comment 9

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

The before-reflow stuff is nsHTMLReflowState::CalculateBlockSideMargins
The  after-reflow stuff is nsBlockReflowContext::AlignBlockHorizontally

Comment 10

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

Attachment: patch to make both even with the left edge

Comment 11

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

Attachment: same as previous, but diff -ub

Comment 12

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

I still don't understand why we are inconsistent between the border and
no-border cases.  And I also still don't understand AlignBlockHorizontally.

Comment 13

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

Attachment: 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).

Comment 14

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

Attachment: patch to make both even with the left edge (diff -u)

Comment 15

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

Attachment: patch to make both even with the left edge (diff -ub)

Comment 16

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

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...

Comment 17

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

Attachment: patch

Comment 18

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

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.

Comment 19

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

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 :-).

Comment 20

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

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.

Comment 21

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

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 22

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

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

okay

Comment 23

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

*** Bug 230762 has been marked as a duplicate of this bug. ***

Comment 24

[Boris Zbarsky [:bzbarsky]]

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?

Comment 25

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

Attachment: 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.

Comment 26

[Boris Zbarsky [:bzbarsky]]

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

sr=bzbarsky

Comment 27

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

Fix checked in to trunk, 2004-02-03 21:18 -0800.

Comment 28

[Mike Cowperthwaite]

xref bug 199385.

Comment 29

[Boris Zbarsky [:bzbarsky]]

*** Bug 234902 has been marked as a duplicate of this bug. ***

Comment 30

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

*** Bug 231325 has been marked as a duplicate of this bug. ***

Comment 31

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

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.