Last Comment Bug 185411 - overflowing block with margin:auto centers if border:none (centering, width, wide, inconsistent, scrolling)
: overflowing block with margin:auto centers if border:none (centering, width, ...
Status: RESOLVED FIXED
[patch]
:
Product: Core
Classification: Components
Component: Layout: Block and Inline (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.7alpha
Assigned To: David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
: Hixie (not reading bugmail)
Mentors:
: 191626 220243 228686 230762 231325 234902 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-12-14 16:23 PST by Ryan
Modified: 2004-08-08 20:47 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Simplified testcase (841 bytes, text/html)
2002-12-14 22:30 PST, Brian Rogers
no flags Details
patch to make both even with the left edge (8.86 KB, patch)
2004-01-17 22:48 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
same as previous, but diff -ub (8.41 KB, patch)
2004-01-17 22:50 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
testcase that previous patch breaks (740 bytes, text/html)
2004-01-17 23:10 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details
patch to make both even with the left edge (diff -u) (7.13 KB, patch)
2004-01-21 16:02 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
patch to make both even with the left edge (diff -ub) (7.01 KB, patch)
2004-01-21 16:03 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
patch (7.60 KB, patch)
2004-01-22 16:53 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
roc: review+
Details | Diff | Splinter Review
patch (7.76 KB, patch)
2004-02-03 20:37 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: superreview+
Details | Diff | Splinter Review

Description Ryan 2002-12-14 16:23:04 PST
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 [:bz] (still a bit busy) 2002-12-14 22:01:04 PST
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 2002-12-14 22:11:03 PST
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 2002-12-14 22:30:17 PST
Created attachment 109326 [details]
Simplified testcase

It's having a left or right border that causes the exception to the rule.
Comment 4 Hixie (not reading bugmail) 2003-11-28 05:02:06 PST
*** Bug 220243 has been marked as a duplicate of this bug. ***
Comment 5 Hixie (not reading bugmail) 2003-11-28 05:04:06 PST
The WG will be examining this.
http://www.w3.org/mid/20031103011613.GA16565@darby.dbaron.org
Comment 6 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2003-12-16 12:03:13 PST
*** Bug 228686 has been marked as a duplicate of this bug. ***
Comment 7 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2003-12-16 12:10:44 PST
*** Bug 191626 has been marked as a duplicate of this bug. ***
Comment 8 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-01-17 22:15:56 PST
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-7 (busy September 14-25) 2004-01-17 22:21:13 PST
The before-reflow stuff is nsHTMLReflowState::CalculateBlockSideMargins
The  after-reflow stuff is nsBlockReflowContext::AlignBlockHorizontally
Comment 10 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-01-17 22:48:51 PST
Created attachment 139322 [details] [diff] [review]
patch to make both even with the left edge
Comment 11 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-01-17 22:50:43 PST
Created attachment 139324 [details] [diff] [review]
same as previous, but diff -ub
Comment 12 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-01-17 22:55:01 PST
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-7 (busy September 14-25) 2004-01-17 23:10:16 PST
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).
Comment 14 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-01-21 16:02:36 PST
Created attachment 139621 [details] [diff] [review]
patch to make both even with the left edge (diff -u)
Comment 15 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-01-21 16:03:20 PST
Created attachment 139622 [details] [diff] [review]
patch to make both even with the left edge (diff -ub)
Comment 16 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-01-21 16:09:01 PST
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-7 (busy September 14-25) 2004-01-22 16:53:28 PST
Created attachment 139703 [details] [diff] [review]
patch
Comment 18 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-01-22 17:49:08 PST
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) 2004-01-26 08:44:43 PST
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-7 (busy September 14-25) 2004-01-26 09:21:39 PST
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-7 (busy September 14-25) 2004-01-26 09:22:56 PST
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) 2004-01-26 12:15:07 PST
Comment on attachment 139703 [details] [diff] [review]
patch

okay
Comment 23 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-01-26 21:25:18 PST
*** Bug 230762 has been marked as a duplicate of this bug. ***
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2004-02-03 20:25:17 PST
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-7 (busy September 14-25) 2004-02-03 20:37:31 PST
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.
Comment 26 Boris Zbarsky [:bz] (still a bit busy) 2004-02-03 20:40:52 PST
Comment on attachment 140569 [details] [diff] [review]
patch

sr=bzbarsky
Comment 27 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-02-03 21:19:20 PST
Fix checked in to trunk, 2004-02-03 21:18 -0800.
Comment 28 Mike Cowperthwaite 2004-02-06 12:51:28 PST
xref bug 199385.
Comment 29 Boris Zbarsky [:bz] (still a bit busy) 2004-02-19 16:06:29 PST
*** Bug 234902 has been marked as a duplicate of this bug. ***
Comment 30 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-04-27 18:17:08 PDT
*** Bug 231325 has been marked as a duplicate of this bug. ***
Comment 31 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-05-05 16:13:02 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.