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

RESOLVED FIXED

Status

()

Core
Layout: Tables
P2
normal
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Brian Ryner (not reading), Assigned: Bernd)

Tracking

({testcase, topembed})

Trunk
PowerPC
All
testcase, topembed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt3], URL)

Attachments

(7 attachments)

(Reporter)

Description

14 years ago
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.
(Reporter)

Comment 1

14 years ago
Created attachment 124264 [details]
gif file #1 for testcase
(Reporter)

Comment 2

14 years ago
Created attachment 124265 [details]
gif file #2 for testcase
(Reporter)

Comment 3

14 years ago
Created attachment 124266 [details]
gif file #3 for testcase
(Reporter)

Comment 4

14 years ago
Created attachment 124267 [details]
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.
(Reporter)

Comment 5

14 years ago
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.
(Reporter)

Updated

14 years ago
Keywords: nsbeta1, topembed

Comment 6

14 years ago
What Mozilla version do you use. 
(Reporter)

Comment 7

14 years ago
This is with a current CVS build.
(Reporter)

Comment 8

14 years ago
Created attachment 124272 [details]
reflow log from the table sizing too wide
(Reporter)

Comment 9

14 years ago
Created attachment 124273 [details]
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

14 years ago
Take a look at bug 207005 which appears to have similar symptoms.
Summary: Table lays out too wide when image is not pulled from image cache → {inc}Table lays out too wide when image is not pulled from image cache

Updated

14 years ago
Keywords: testcase
Priority: -- → P2

Comment 11

14 years ago
Confirmed with 2003052708 winXP
(Reporter)

Comment 12

14 years ago
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?
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.
(Assignee)

Comment 14

14 years ago
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.
         
> Setting these flags is
> *expensive* as a rebalance causes a resize reflow for all table children.

Why?  That seems ridiculous.
(Assignee)

Comment 16

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

Comment 17

14 years ago
adt: need info.  Brian, is this a regression from Mach V?
Whiteboard: [need info]
(Assignee)

Comment 18

14 years ago
Created attachment 124623 [details] [diff] [review]
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).
(Assignee)

Comment 19

14 years ago
taking the bug, this is not only a Mac problem
Assignee: table → bernd_mozilla
OS: MacOS X → All
Whiteboard: [need info]
(Assignee)

Updated

14 years ago
Attachment #124623 - Flags: superreview?(dbaron)
Attachment #124623 - Flags: review?(jkeiser)

Updated

14 years ago
Attachment #124623 - Flags: review?(jkeiser) → review+

Comment 20

14 years ago
adt: nsbeta1+/adt3
Keywords: nsbeta1 → nsbeta1+
Whiteboard: [adt3]
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).
Attachment #124623 - Flags: superreview?(dbaron) → superreview+
(Assignee)

Comment 22

14 years ago
fix checked in into trunk

Comment 23

14 years ago
a=adt Please land this fix on the 1.4 branch and add the fixed1.4 keyword.
(Reporter)

Comment 24

14 years ago
Comment on attachment 124623 [details] [diff] [review]
patch

requesting drivers approval to land on 1.4 branch.
Attachment #124623 - Flags: approval1.4?
(Reporter)

Comment 25

14 years ago
Comment on attachment 124623 [details] [diff] [review]
patch

I think this is too late for 1.4.
Attachment #124623 - Flags: approval1.4?
(Assignee)

Comment 26

14 years ago
this bug is fixed on the trunk and did not go into the branch, so marking it fixed
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.