Tables ignore mComputedWidth and mComputedHeight

VERIFIED INVALID

Status

()

Core
Layout: Tables
P3
normal
VERIFIED INVALID
19 years ago
10 months ago

People

(Reporter: David Hyatt, Assigned: karnaze (gone))

Tracking

Trunk
x86
Other
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

19 years ago
When an outer table is flowed at some computed width and height, it does not
simply pass these values through to the inner table.  Instead it tries to pull
values off the table tag etc. etc. and thus ends up blowing away the computed
width and height passed in.

For the case where the computed width and height are not unconstrained, the
outer table should not be replacing these values.  It should be relying on them.

This is preventing tree widgets and tables from working inside boxes, and
according to Troy is a bug in the table code.
(Assignee)

Updated

19 years ago
Assignee: karnaze → hyatt
(Assignee)

Comment 1

19 years ago
David, please supply a test case, preferably one without apprunner, xul, etc.
(Reporter)

Comment 2

19 years ago
Hmmm.  I'm not sure how to supply a test case in HTML.  I can give you a XUL
test case.

Comment 3

19 years ago
I think an XML test case would be acceptable as well. Not a XUL test case,
though. :-)
(Reporter)

Updated

19 years ago
Assignee: hyatt → karnaze
(Reporter)

Comment 4

19 years ago
Here is a XUL test case (I needed to use XUL to get support for inline style,
because I think this makes it easier for you to see what's going on visually).

======

<?xml version="1.0"?> <!-- -*- Mode: SGML; indent-tabs-mode: nil; -*- -->

<?xml-stylesheet href="chrome://global/skin/xul.css" type="text/css"?>

<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
  xmlns:html="http://www.w3.org/TR/REC-html40" align="vertical"
  style="width: 300px">
<box style="height:300px; border: 3px solid blue" align="vertical">
<html:table flex="1" style="height:100px; border: thin solid red;">
     <html:tr><html:td>Row 1 of 3</html:td></html:tr>
	 <html:tr><html:td>Row 2 of 3</html:td></html:tr>
	 <html:tr><html:td>Row 3 of 3</html:td></html:tr>
</html:table>
</box>
</window>

======

What you'll see: a 300 by 300 blue box that contains a much smaller
(red-bordered) table.  The table was actually being flowed with a computed width
and height of 300 by 300.

Notice that the table doesn't stretch itself horizontally, despite being told
that it's computed width is wider.  Notice more importantly that the table
doesn't stretch itself vertically.  It instead sized itself to 100 pixels,
matching the style rule on the table.  This is wrong, since the parent box is
the one that chose to ignore that style rule and flow the child at a larger
computed height.

My hope is that this is easy to fix.  I believe it's just that the inner table
frame is looking at style rules on the outer table frame or something, when it
really should just be getting a pass-through (minus the table caption of
course).
(Reporter)

Comment 5

19 years ago
Err that should have read "its computed width is wider"... the grammar police
will be coming for me soon.
(Reporter)

Comment 6

19 years ago
I believe this situation really is unique to XUL, since in HTML, the parent
container would typically have used and honored the style request to be 100
pixels tall.  Boxes are different in that they can stretch content, even when a
style rule specifying a size is present.  The box in this test case looks at the
100 pixel height request and then sees that the flex attribute is present.
Because it's there, the box knows that the table should be stretched to be 300
pixels instead, and so it passes that in as the computed height.

Somewhere along the line, though, the table is looking at the 100 pixel height
request and using that instead.

So your mission, should you choose to accept it, is to get the red bordered box
to stretch so that it's as big as the blue bordered box.

Comment 7

19 years ago
Maybe you can use min-height and max-height to help get what you want? Something
like "let the computed height be shrink wrap, but make sure the height is at
least xxx and no more than yyy"
(Reporter)

Comment 8

19 years ago
I'd like to avoid special-casing the box code when dealing with trees and
tables.  IMO what the box code is doing is quite reasonable, and I suspect this
bug is pretty easy to fix (i.e., that the style values are being pulled off by
the table, when it has no business doing so).

Comment 9

19 years ago
It's true that looking at it now it has no business doing that, but keep in mind
that the table code was written long before we added in things like
nsHTMLReflowState

I wasn't proposing min-height and max-height as alternatives to cleaning up the
table code, I was simply mentioning them as options in controlling the computed
width and height

Comment 10

19 years ago
And I might add hyatt, I implemented min-width, max-width, min-height,
max-height support _just_ for the xpfe (and xul) folks. So, I would appreciate
it if you would use it, somewhere, anywhere... :-)
(Reporter)

Comment 11

19 years ago
Troy, we use it all over the place.  You are loved. :)
(Reporter)

Updated

19 years ago
Status: NEW → RESOLVED
Last Resolved: 19 years ago
Resolution: --- → FIXED
(Reporter)

Comment 12

19 years ago
I checked in a fix.  It looks cool to me.  If it causes any problems, I'll be
ready to back it out pronto.
(Assignee)

Updated

19 years ago
Status: RESOLVED → REOPENED
(Assignee)

Comment 13

19 years ago
David, I am going to back out your changes since they broke a lot of the
regression tests (located in layout/html/tests/table and invoked by "rtest
baseline" before and "rtest verify" after). I'll keep your changes in as
comments and will take the bug.
(Assignee)

Updated

19 years ago
Resolution: FIXED → ---
(Reporter)

Updated

19 years ago
Status: REOPENED → NEW
(Reporter)

Comment 14

19 years ago
Ok, all yours.
(Assignee)

Updated

19 years ago
Status: NEW → RESOLVED
Last Resolved: 19 years ago19 years ago
Resolution: --- → FIXED
(Assignee)

Comment 15

19 years ago
David, this is fixed except for a minor thing you need to do. The box frame
(unlike various html elements) is setting the computed width/height for the
outer table frame as the content dimensions. For tables (and form control
elements, I think) the computed values should be the box dimensions (including
border and padding), not content dimensions. Fiddle with the table's border and
padding in your example to see what I mean.

Updated

19 years ago
Depends on: 12012

Comment 16

19 years ago
Hmm.. In everything I have encountered so far including form elements the
computed size for a block or inline-replaced (like for elements) is the content
area and does not include border or padding. It is up to the frame to return
the computed size + it's border and padding. Take a look at the implementation
of nsLeafFrame....
(Assignee)

Comment 17

19 years ago
For Nav4.6 and IE5 compatibility, computed widths for form elements and tables
include border and padding (unlike every other html element). When you say
<table border width=100>, you get a table that is 100 pixels wide and that
includes border and padding. When you say <div style="width:100px; border:1px
solid black;"> you get a div whos content area is 100 pixels wide and doesn't
include border and padding. You won't find this distinction in the CSS2 spec,
but Peter has been active on the working group and has implemented a property
which tells which edges to use to define the computed width. I'm not sure what
the property is or how to use it yet, but it sounds like it should be used for
this bug.

You are right that in all cases, it is up to a frame's Reflow to return its
desired size which includes border and padding. The only issue in question is
how to interpret the computed values set on the ReflowState, and that is the
point I am trying to make.

Comment 18

19 years ago
The CSS3 proprty in quesiton is called: 'box-sizing'. It determines which edge
of a box the width & height properties apply to: content, padding or border.
To make this work right, the HTML attribute mapping code
(nsHTMLTableElement::MapAttributesInto) for 'width' on tables should *also* set
the box-sizing property to 'border-box'.

Comment 19

19 years ago
So it sounds like the table needs to look at that property to see how it should
interpet the computed size. So this bug should probably be reopened until that
is fixed. Otherwise the the table will return the wrong size and will cause
boxes to do very bad things. Boxes expect their children to return the computed
size + their border and padding. If they don't it assumes they could not be
sized and gives them a min size of what was returned.. The end effect will be
that tables will just continously grow.

Updated

18 years ago
Whiteboard: 11/16: Requested verification by reporter

Comment 20

18 years ago
David H: Could you please verify this is fixed. I'm not familiar with
'box-style' or table 'flex'. If you agreed it's fixed, please verify it so.
Thanks

Updated

18 years ago
Whiteboard: 11/16: Requested verification by reporter → 12/3: Requested verification by reporter

Comment 21

18 years ago
David: Could you please verify this bug fixed if you agree? Thanks
(Reporter)

Comment 22

18 years ago
Nope. This is not fixed.  It should be reopened.

Updated

18 years ago
Status: RESOLVED → REOPENED
Whiteboard: 12/3: Requested verification by reporter

Comment 23

18 years ago
Reopened per reporter's comments

Updated

18 years ago
Resolution: FIXED → ---

Comment 24

18 years ago
Adding rickg to cc list....need to investigate for PDT+ or -
(Assignee)

Comment 25

18 years ago
I talked to Hyatt and he doesn't think this is PDT+.

Updated

18 years ago
Whiteboard: [PDT-]

Comment 26

18 years ago
Putting on PDT- radar, per karnaze comment.
(Assignee)

Updated

18 years ago
Status: REOPENED → ASSIGNED
Target Milestone: M13
(Assignee)

Updated

18 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 19 years ago18 years ago
Resolution: --- → FIXED
(Assignee)

Comment 27

18 years ago
Fixed with latest checkin.

Updated

18 years ago
Whiteboard: [PDT-] → [PDT-] 1/7: Pending verification from reporter

Comment 28

18 years ago
David: Could I ask you to look at this again and see if it's fixed? Karnaze
marked it fixed as of 1/2. Thanks
(Assignee)

Comment 29

18 years ago
Hyatt, I noticed some changes you checked in to nsTreeOuterFrame that leads me
to believe you don't think this got fixed. My changes fixed the example given.

I forgot to mention that tables now have a default box-sizing property of
border-box (compatable with Nav and IE) and respect box-sizing. If it is desired
to use content-box, then that should be specified in the style.
(Reporter)

Comment 30

18 years ago
Tables still ignore mComputedWidth and mComputedHeight on an incremental reflow
targeted at the table.  I added a hacked method called "FixBadReflowState" to
tables (that only trees implement), so that trees would at least work.  You can
grep for this method to see where the problem is.

Comment 31

18 years ago
Should this bug be reopened?
(Reporter)

Comment 32

18 years ago
yes, but if you do reopen it, remove the DOGFOOD and BLOCKER designations.  It's
not that serious anymore.  It's almost totally fixed.

Updated

18 years ago
Status: RESOLVED → REOPENED
Summary: [DOGFOOD/BLOCKER] Tables ignore mComputedWidth and mComputedHeight → Tables ignore mComputedWidth and mComputedHeight

Updated

18 years ago
Resolution: FIXED → ---

Comment 33

18 years ago
Removing DOGFOOD/BLOCKER designations and reopening bug per David's comments.
(Assignee)

Updated

18 years ago
Status: REOPENED → ASSIGNED
Target Milestone: M13 → M14
(Assignee)

Comment 34

18 years ago
Moving to M14.

Updated

18 years ago
Blocks: 24206

Updated

18 years ago
Whiteboard: [PDT-] 1/7: Pending verification from reporter → [PDT-]

Comment 35

18 years ago
Removing PDT annotation now that dogfood nomination has been removed.
Whiteboard: [PDT-]
(Assignee)

Comment 36

18 years ago
Moving to M16.
Target Milestone: M14 → M16
(Assignee)

Comment 37

18 years ago
Tables have stopped using FixBadReflowState and now they handle mComputed values 
better. I forgot to remove the method, but I will at some point.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → FIXED

Updated

18 years ago
No longer blocks: 24206

Comment 38

18 years ago
Verifying bug fixed. If reporter does not agree, please reopen.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 39

16 years ago
Reopening to mark invalid.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 40

16 years ago
I have removed the code that fixes this bug to fix topembed bug 106158 and clean 
up the table code, since evaughan assures me that a box can no longer contain a 
table. The outer table is a combination of the inner table and caption and 
always has auto width and height. It was only made to honor artifical mComputed 
value to accomodate boxes when trees used tables. 
Status: REOPENED → RESOLVED
Last Resolved: 18 years ago16 years ago
Resolution: --- → INVALID

Updated

16 years ago
QA Contact: chrisd → amar

Comment 41

16 years ago
 Marking verified. Invalid
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.