207208 - {inc}Table lays out too wide when image is not pulled from image cache

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect, P2)

Description

[Brian Ryner (not reading)]

If you visit this site for the first time or shift+reload it, you'll likely see
a large horizontal gap down the middle of the page.  I reduced this down to an
apparent timing problem where if  we don't know an image's size up front, we end
up allocating too much space for the table.  I have a reduced testcase that I'll
be attaching in a minute.

Comment 1

[Brian Ryner (not reading)]

Attachment: gif file #1 for testcase

Comment 2

[Brian Ryner (not reading)]

Attachment: gif file #2 for testcase

Comment 3

[Brian Ryner (not reading)]

Attachment: gif file #3 for testcase

Comment 4

[Brian Ryner (not reading)]

Attachment: html for testcase

To see the problem, load this attachment and notice the dark blue strip to the
extreme right.	Click reload (not shift+reload) and notice that it goes away. 
The second behavior, when the image is cached, is the correct layout.

Comment 5

[Brian Ryner (not reading)]

I should also note that here:

        <td width="170" align="left" valign="top">
          <img src="http://bugzilla.mozilla.org/attachment.cgi?id=124265&action=
view" border="0">
        </td>

if I add width="201" to the <img> tag (that's the actual image width), the
problem goes away.  This seems to support the theory that not knowing the image
size up front causes a problem.

Comment 6

[José Jeria]

What Mozilla version do you use. 

Comment 7

[Brian Ryner (not reading)]

This is with a current CVS build.

Comment 8

[Brian Ryner (not reading)]

Attachment: reflow log from the table sizing too wide

Comment 9

[Brian Ryner (not reading)]

Attachment: reflow log with the width=201 workaround

I wanted to get a reflow log for reloading and pulling the image from the
cache, but Mac viewer wasn't cooperating.  So, instead, I added width=201 to
the image, which I'm hoping has the same effect (the image width is known
immediately).

Comment 10

[tenthumbs]

Take a look at bug 207005 which appears to have similar symptoms.

Comment 11

[Dan Backes]

Confirmed with 2003052708 winXP

Comment 12

[Brian Ryner (not reading)]

Bernd, do you happen to know where the code is at that's supposed to handle a
pixel width specified on a cell, where the cell's contents are wider than that?
 Any other ideas on this bug?

Comment 13

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

The width attribute on TD (and unfortunately the CSS 'width' property as well)
acts like 'min-width', not 'width'.  So without the image, there's a good bit of
extra space to distribute in the table, but once the image arrives, it isn't
redistributed (probably the code calls it rebalanced) the same way it would have
been had the image been there initially.

Comment 14

[Bernd]

Its like dbaron said, we are missing a table rebalance once the cell size
changed on the incremental reflow. The functions to indicate this are
http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/nsTableFrame.h#790 -
SetNeedStrategyBalance  the soft variant

and
http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/nsTableFrame.h#787 - 
SetNeedStrategyInit the hardcore reset.

The debugging of the table layout strategy is documented at
http://lxr.mozilla.org/seamonkey/source/layout/doc/debugtable.html . Its very
unlikely that the layout strategy is wrong, its more likely the necessary flag
is not set at all or the too soft flag is set. Setting these flags is
*expensive* as a rebalance causes a resize reflow for all table children.

off topic:

          block 0x182f410 r=0 a=UC,UC c=UC,UC cnt=22 
           text 0x182f570 r=0 a=UC,UC c=UC,UC cnt=23 
           text 0x182f570 d=0,0 me=0 
           img 0x182f7e0 r=0 a=UC,UC c=UC,UC cnt=24 
           img 0x182f7e0 d=360,360 me=360 
           text 0x182f860 r=0 a=UC,UC c=UC,UC cnt=25 
           text 0x182f860 d=60,240 me=0 
          block 0x182f410 d=360,360 me=360 o=(0,0) 360 x 420

I am very suspicous that a block on a unconstrained inital reflow produces an
overflow.
         

Comment 15

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

> Setting these flags is
> *expensive* as a rebalance causes a resize reflow for all table children.

Why?  That seems ridiculous.

Comment 16

[Bernd]

s/all table children/all table children that should change theire size due to
the changed column widths/ 

Comment 17

[Samir Gehani]

adt: need info.  Brian, is this a regression from Mach V?

Comment 18

[Bernd]

Attachment: patch

This bug is certainly not a recent regression. Like I said before the problem
was that on change of the cell-mew the columns have been only rebalanced (too
soft). The space distribution for colspan is done during the
strategy-initialize. So whenever a colspan is involved the strategy needs to be
reinitialized. In nsTableFrame::CellChangedWidth we checked before that from a
previous column a cell spans into the current column, but we did not check that
a colspan originates from the column itself.

There was already a stub for the necessary function in nsTableFrame.h. So I
wrote the implementation. And the code in nsCellMap.cpp, as it was never used
before, was plain wrong (it was copied from the row code above).

Comment 19

[Bernd]

taking the bug, this is not only a Mac problem

Comment 20

[Samir Gehani]

adt: nsbeta1+/adt3

Comment 21

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

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

sr=dbaron if you remove the tab that you copied (and the original as well, if
you want).

Comment 22

[Bernd]

fix checked in into trunk

Comment 23

[Paul Wyskoczka]

a=adt Please land this fix on the 1.4 branch and add the fixed1.4 keyword.

Comment 24

[Brian Ryner (not reading)]

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

requesting drivers approval to land on 1.4 branch.

Comment 25

[Brian Ryner (not reading)]

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

I think this is too late for 1.4.

Comment 26

[Bernd]

this bug is fixed on the trunk and did not go into the branch, so marking it fixed