444015 - Table Alignment. Worked just fine in Firefox2.0.14/15 but not Firefox 3

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Loz]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0

Hi

I have a site called http://www.videoguide-toprofits.com
as you can see, the text is moved over to the left of the
main content holder, it's a normal table that has been
centered with text in side the table. When I developed this
in Firefox 2.0.14/15 all appeared to work just fine, now,
since you've updated to firefox 3 and over 8 million people
have upgraded, it would appear to me that a lot of sites
out there do not appear what they looked like in Firefox2.

This is also the same for IE8 as well, even though they are
in beta, firefox 3 is a published release that still seems
buggy.

Would you know when this will be corrected. I am a website
designer and I can no longer design sites in Firefox 3, and
it wont be long before the majority will upgrade to FF3.

Thanks


Looking forward to your reply.

Yours VERY concerned.

Loz

Reproducible: Always

Steps to Reproduce:
1. No need to reproduce error, worked find in firefox 2 and IE7 (but not in IE8)
2.
3.
Actual Results:  
The table isn't totally centered like it was in firefox2

Expected Results:  
To align properly like it was in firefox 2

N/A

Comment 1

[Ria Klaassen (not reading all bugmail)]

This bug was caused by the changes in Bug 300030.

Comment 2

[Martijn Wargers (dead)]

Attachment: testcase

Comment 3

[Loz]

Hi Ria & Martijn

I see that the problem has been fixed, is there a download to test the fixed changes?

Thanks.

Loz

Comment 4

[Martijn Wargers (dead)]

Loz, what do you mean? The problem has not been fixed yet.

Comment 5

[Loz]

Hmm, odd, I thought I saw somewhere at the bottom here that it was fixed... Just looked again, didn't realise that it was something to change something to "fixed" via the drop down menu.

Sorry about that, I'm not too familiar with this site, first time reporting a bug.

Sorry for the inconvenience.

BTW, any idea how soon these patches are fixed and uploaded as another release candidate?

Cheers

Loz

Comment 6

[Martijn Wargers (dead)]

Sorry, I don't know. If there's a patch and it is accepted for the 1.9.0.x branch, then there is a chance it will get into the 1.9.0.2 update, which is at least one month from now.

Comment 7

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

Is this really a bug? I assume <td align="center"> just sets text-align. But text-align doesn't actually center block-level elements. Or is this a quirk that's broken?

Comment 8

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

I suspect it's a quirk that's broken; we set text-align: -moz-center for a bunch of align attributes in HTML.

Comment 9

[Samuel Sidler (old account; do not CC)]

No patch for this and it's unfixed on trunk so we won't block on it, but nominating for 1.9.0.3 and marking as wanted1.9.0.x.

Comment 10

[Bernd]

Attachment: patch

The logic at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsHTMLReflowState.cpp&rev=1.300&mark=1922,1923,1932,1989,#1915

is flawed when a computed margin exists previous to the alignment. The margin will first reduce the marginspace that can be distributed but then it will be overwritten by the half of the remaining space so the sum of the widths will not fit the available width.

Comment 11

[Bernd]

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

To my big surprise this went smoothly trough the regression tests. David there is nobody who knows this code better than you do you think this will fly?

Comment 12

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

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

The change to fix mComputedMargin.left looks correct.

However, mComputedMargin.right is even worse.  We need to do a +=
for both.  So the code should actually look like:
  nscoord forLeft = availMarginSpace / 2;
  mComputedMargin.left += forLeft;
  mComputedMargin.right += (availMarginSpace - forLeft);
and there should also be a comment somewhere noting that the computed
margins need not be zero because the 'auto' could come from
overconstraint or from HTML alignment.

If you agree that all makes sense (since somebody else probably ought to 
review what I'm saying), r+sr=dbaron.

Comment 13

[Bernd]

Yep thats correct, I will attach a new patch soon and get it in.

Comment 14

[Bernd]

Attachment: patch that got checked in

Comment 15

[Martijn Wargers (dead)]

Bernd, so this bug should be marked FIXED now that you checked in the patch?

Comment 16

[Bernd]

yes, tinderbox was yesterday a bit flaky (the usual weekend blues). 
<ot> why the bug resolution is on top and not close the commit button is difficult to perceive</ot>

Comment 17

[Martijn Wargers (dead)]

(In reply to comment #16)
> <ot> why the bug resolution is on top and not close the commit button is
> difficult to perceive</ot>

I guess that is bug 452888, more or less. (fwiw, I don't understand a lot of the ui changes at all).

Comment 19

[Daniel Veditz [:dveditz]]

If this didn't block the trunk it's not going to block a security update release, but you can request approval on a branch patch.

Comment 20

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081013 Minefield/3.1b2pre and the testcase in Comment 2.

Comment 21

[Loz]

Hi guys,

I was wondering, have you guys found a fix yet? I've
noticed two updates since last and the problem still
persists, however, using CSS works ok.

Cheers

Loz

Comment 22

[Ryan VanderMeulen [:RyanVM]]

This is fixed on the nightly builds which will eventually lead to Firefox 3.1. You can test that it is fixed with either a nightly build from the link below or by downloading Firefox 3.1 beta 1, which should be released in a couple days.
ftp://ftp.mozilla.org/pub/firefox/nightly/2008/10/

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Oh, I forgot to mention that the mozilla-central nightlies are the ones you want if you're trying a nightly.