434733 - colgroup width tag is not overridden when col width is specified

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Heuel]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9) Gecko/2008051206 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9) Gecko/2008051206 Firefox/3.0

I found that the colgroup width is not overriden, when the col width is specified. Expected behaviour in http://www.w3.org/TR/html401/struct/tables.html#h-11.2.4.2

Reproducible: Always

Steps to Reproduce:
1. Load attached HTML

Actual Results:  
The tables with the colgroup width tag are not rendered as expected.

Expected Results:  
Expected behaviour in http://www.w3.org/TR/html401/struct/tables.html#h-11.2.4.2

Comment 1

[Heuel]

Attachment: Test case for not overridden col width

Comment 2

[Henri de Solages]

Attachment: Your HTML file on my brower

On my Firefox 2.0.0.18 for Linux, it works as expected.

Comment 3

[Heuel]

Attachment: Screenshot for test case (Mac FF 3.0)

I currently don't have access to an XP machine, but on my Mac, I have the same problem with FF3. So the test cases seems to behave differently for FF2 and/or Linux.

Comment 4

[Alice0775 White]

Same Issue
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090316 Minefield/3.2a1pre
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090315 Shiretoko/3.1b4pre

Why did not anybody check the bug?

Comment 5

[Makoto Kato [:m_kato]]

In BasicTableLayoutStrategy::ComputeColumnIntrinsicWidths(), it always overrides width value.  I believe that This behavior is invalid.

I will take it.

Comment 6

[Makoto Kato [:m_kato]]

Attachment: patch v1

Now I am tesing on all platforms.

Comment 7

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

How do other browsers behave in this case?  (IE6, IE8, Firefox 2, WebKit, Opera?)

Comment 8

[philippe (part-time)]

(In reply to comment #7)
> How do other browsers behave in this case?  (IE6, IE8, Firefox 2, WebKit,
> Opera?)

All other browsers behave the same: the second table in attachment (id=321751) is 238px wide, just as the first table.

Firefox 2 / OS X 10.5.6
Webkit nightly, Safari 4b, Safari3.2
Opera 10.a & Opera 9.6
IE6 / Win XP
IE8 / Win Vista

Comment 9

[Makoto Kato [:m_kato]]

David, this is a regression from Firefox 3.0.

Maybe, the following check in causes this regression. (bug 30030).

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/layout/tables&command=DIFF_FRAMESET&file=BasicTableLayoutStrategy.cpp&rev1=3.234&rev2=3.235&root=/cvsroot

Comment 10

[Makoto Kato [:m_kato]]

add regression keyword.  This works fine until 2.0.x, and other browser such as Trident and WebKit.

Comment 11

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

(In reply to comment #9)
> Maybe, the following check in causes this regression. (bug 30030).

I'm sure it did.


+        if (colFrame->GetMinCoord() == 0 && colFrame->GetPrefCoord() == 0 &&
+            colFrame->GetPrefPercent() == 0.0f) {

I think it would be slightly clearer if these checks used colInfo.minCoord, colInfo.prefCoord, and colInfo.prefPercent.


Did you also test what happens when the width on the colgroup is a percentage width and the width on the column is an integer?  And then the other way around?  We should both:
 * check those cases in other browsers, and
 * have reftests for them.
(If you'd prefer I do this, let me know.)


Also, you should call the reftest 434733-1.html rather than 434733.html (same change for -ref) so that more tests can be added (as described above).

r=dbaron with those changes

Comment 12

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

Comment on attachment 368913 [details] [diff] [review]
patch v1

Can you prepare the revised patch, or should I?

Comment 13

[Makoto Kato [:m_kato]]

sorry for delay reply.  I forgot this.

I will modify it, then re-send a review to you.

Comment 14

[Makoto Kato [:m_kato]]

Attachment: patch v2

Comment 15

[Makoto Kato [:m_kato]]

Comment on attachment 386273 [details] [diff] [review]
patch v2

add reftest for percentage

Also, after fix this, Fx become same result of WebKit (Saferi 4) and Trident (IE 8).

Comment 16

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

Comment on attachment 386273 [details] [diff] [review]
patch v2

r=dbaron

I wrote two additonal tests as well, and I renamed all of the tests and put them in layout/reftests/table-width/, and I have the whole thing ready to commit next time I land patches.

Comment 17

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

http://hg.mozilla.org/mozilla-central/rev/1b43edccce82
http://hg.mozilla.org/mozilla-central/rev/4ccc577273db

Thanks for the patch, and sorry for the delay in getting it in.