Last Comment Bug 25707 - Conflict between text-indent in percent and margin-left in percent in table
: Conflict between text-indent in percent and margin-left in percent in table
Status: VERIFIED FIXED
: css1, helpwanted, testcase
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: P3 major with 2 votes (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
: Jan Carpenter
: Jet Villegas (:jet)
Mentors:
http://www.thock.com/3ilinux/products/
Depends on: 45631
Blocks:
  Show dependency treegraph
 
Reported: 2000-01-29 18:38 PST by dylang
Modified: 2014-04-26 03:22 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase: conflict between text-indent and margin inside table (200 bytes, text/plain)
2000-01-30 01:49 PST, Eric Mao
no flags Details
re-applied testcase with correct MIME (200 bytes, text/html)
2000-03-08 17:58 PST, Marc Attinasi
no flags Details
Simpler testcase (1.45 KB, text/html)
2000-07-16 21:24 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
Even simpler test case! (153 bytes, text/html)
2000-07-16 21:26 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
The REAL testcase (162 bytes, text/html)
2000-07-16 21:50 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
Example of the enormous difficulty of choosing correct desired width (418 bytes, text/html)
2000-07-17 09:47 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
margin-left and text-indent in 600 pixel table (576 bytes, text/html)
2000-07-17 14:33 PDT, Bradley Robinson
no flags Details
Patch to nsBlockReflowContext::DoReflowBlock, nsBlockReflowContext::PlaceBlock (5.22 KB, patch)
2000-07-17 18:04 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
Screenshot of working table at www.thock.com/Dylan (19.33 KB, image/png)
2000-07-17 18:12 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
V2 of patch to fix maxElementSize of elements with percentage margins and percentage-width content (5.55 KB, patch)
2000-07-25 11:27 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review

Description dylang 2000-01-29 18:38:47 PST
It looks like the CSS1 parser has caused major problems.  The entire client area 
seems to be filled with random bits of text and graphics.  It could be an array 
that wasn't properly terminated, or just a general memory leak or pointer 
confusion, but the entire window slowly randomises itself :-)
Comment 1 Asa Dotzler [:asa] 2000-01-29 19:01:06 PST
changed component to Style System.  Dylan, what build are you using?
Comment 2 Eric Mao 2000-01-30 01:49:47 PST
Created attachment 4721 [details]
testcase: conflict between text-indent and margin inside table
Comment 3 dylang 2000-01-30 09:24:50 PST
This is using Mozilla M13.
Under Windows, I get massive corruption of the browser window (<a 
href="http://www.thock.com/Dylan/M13_tasties.jpg">screenshot</a>).
Under Linux (2.2.15pre4/October Gnome/IceWM v1.0.1/XF86_SVGA 3.3.5), I get a 
bunch of text that does not wrap propely (long horizontal scrollbar).  This is 
using a prebuilt binary with fullcircle talkback.

M13 on Linux and Windows both misrender <a 
href="http://www.thock.com/3ilinux/">http://www.thock.com/3ilinux/</a> the same, 
though :-)
Comment 4 leger 2000-02-01 09:30:24 PST
Assigning all open "nobody@mozilla.org" bugs to "leger@netscape.com" to weed 
thru.
Comment 5 Richard Zach 2000-02-22 01:58:52 PST
Reassign to component owner.
Comment 6 Marc Attinasi 2000-03-03 12:05:34 PST
I'm not seeing the major corruption, however the right-margins are clearly wrong 
as the text goes on and on toward the right without wrapping, as Dylan 
mentioned.

I'll take this one and investigate further.
Comment 7 Marc Attinasi 2000-03-08 17:58:13 PST
Created attachment 6311 [details]
re-applied testcase with correct MIME
Comment 8 Marc Attinasi 2000-04-05 11:49:29 PDT
This looks like a table layout problem. When both margin-left and text-indent 
are specified in percentages the table is way to wide. If either one is 
specified in em or px units, then it is fine. 

Assigning to karnaze since the style looks correct.
Comment 9 karnaze (gone) 2000-04-28 18:14:03 PDT
Buster, it looks like the cell's block is returning a very large desired and max 
element size upon an unconstrained reflow. 
Comment 10 buster 2000-06-01 14:20:51 PDT
redistributing bugs across future milestones, sorry for the spam
Comment 11 buster 2000-06-04 21:28:11 PDT
This bug has been marked "future" because we have determined that it is not 
critical for netscape6.0. 
If you feel this is an error, or if it blocks your work in some way -- please 
attach your concern to the bug for reconsideration.
Comment 12 Ben Bucksch (:BenB) 2000-07-15 14:00:37 PDT
Cool screenshot.

Do I understand it correctly, that we won't be CSS1-compliant with this bug?
Comment 13 Jerry Baker 2000-07-15 14:32:29 PDT
This bug means that valid HTML 4.0 and CSS1 will not work. There is no way that 
Netscape/Mozilla can go gold without *full* CSS1 HTML 4.0 compliance IMHO. It's 
the only way to stand out.
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2000-07-15 14:59:26 PDT
Full HTML4/CSS1 compliance can't mean "100% bug free". If it does, no-one will
ever ship a fully copmliant browser.
Comment 15 Jerry Baker 2000-07-15 15:04:25 PDT
Right, but that is no reason to avoid fixing bugs either. There is a long time 
before the final release. Bugs that break compliance should be ahead of 
everything else except crashers.
Comment 16 Ian Thomas ('thelem') 2000-07-15 17:08:37 PDT
I agree, if it breaks compliance then it should be fixed. Before Netscape 
PR3 - thats not exactly tomorrow.
Comment 17 timeless 2000-07-15 18:58:24 PDT
Buster, thanks for considering this bug.  Jan, you seem to be the correct QA 
this bug needs to be fixed.  Buster, I'm sorry about your workload, could you 
possibly give a hint about how someone would fix this?  Milestone reset by /.

Justification for loss of future: /., css1
relnote2: if we don't fully support css we need to say so.
nsbeta3: css1
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2000-07-16 21:24:25 PDT
Created attachment 11473 [details]
Simpler testcase
Comment 19 Robert O'Callahan (:roc) (email my personal email if necessary) 2000-07-16 21:26:31 PDT
Created attachment 11474 [details]
Even simpler test case!
Comment 20 Robert O'Callahan (:roc) (email my personal email if necessary) 2000-07-16 21:32:19 PDT
First observation: incremental reflow of "text-indent: n%" is broken. It's easy 
to fix. I have filed bug 45631 with a patch.
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2000-07-16 21:34:01 PDT
Sorry, ignore those testcases. I'm confused.
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2000-07-16 21:50:14 PDT
Created attachment 11475 [details]
The REAL testcase
Comment 23 Robert O'Callahan (:roc) (email my personal email if necessary) 2000-07-16 22:20:17 PDT
Some of my comments below may be wrong if you're flying without the 
incremental-text-indent patch. Table layout depends on incremental reflow; 
there's no point in attacking this bug until incremental reflow of text-indent 
is fixed.

If you open attachment 11475 [details] in Viewer, you will see that the layout is wrong. 
The table is unnecessarily wide. In fact, the table will always be as wide as 
the window. In fact, the table edge should be to the right of the text, i.e. the 
table should be sized so that the text should occupy 60% of the width and the 
identation the other 40%.

Note that NS 4.x bungles the layout in a different way: the indent is set to 40% 
of the window width, and then the table is laid out around that.

Currently what happens in Gecko is that the line is initially reflowed with 
unconstrained width. We calculate the text-indent as 40% of some very large 
number, which results in another very large number :-). Then we set the desired 
size to that plus the width of the text. The desired size is never updated 
again... The table sets its size, then the cell is reflowed to fit (which 
updates the text-indent amount, but does not change the desired size).

(I'm pretty sure that the insanity that results in combination with "margin" is 
a consequence of the unreasonable desired cell width. Even if it isn't, we 
should get text-indent in tables working before we tackle the real bug.)

The problem here is simply that reflowing with unconstrained width does not tell 
us the desired width of the block. I tried detecting the special case of taking 
the text-indent percentage of NS_UNCONSTRAINEDSIZE and returning 0 for the 
indent, but of course that just gives you a different wrong estimate of the 
desired width (although one that's a lot more reasonable).

Similar (although less spectacular) problems happen if you put a replaced-inline  
element into the cell, styled with a percentage width.

The solution is not going to be easy, but this should work: while reflowing a 
block with unconstrained width, accumulate in the block's reflow state an 
"additional percentage width required" factor. Things whose width is a 
percentage of the unconstrained-width-block are assigned zero width but add 
their percentages into the accumulator. Then we can calculate the desired width 
of the unconstrained-width-block by solving the equation
desired_width = measured_contents_width + desired_width*accumulated_percentages

Heh heh heh.
Comment 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2000-07-16 23:05:25 PDT
Oh dear. More bad news about "text-indent" is that it is one of the few 
inheritable percentage styles, and we need to inherit the computed value instead 
of the percentage. Currently we inherit the percentage. This breaks in 
situations like this:

<div style="text-indent: 10%; width: 100px;"><p>hellohello</p></div>

The indent should be 10% of the width of the DIV's containing block, but Gecko 
is making it 10px (10% of the width of the DIV) regardless of the width of the 
DIV's containing block.

Making that work right is also going to make fixing this bug harder.
Comment 25 Robert O'Callahan (:roc) (email my personal email if necessary) 2000-07-17 09:47:15 PDT
Created attachment 11481 [details]
Example of the enormous difficulty of choosing correct desired width
Comment 26 Robert O'Callahan (:roc) (email my personal email if necessary) 2000-07-17 10:33:46 PDT
I was overly optimistic last night. The previous example shows just how 
difficult it is to choose the desired cell width. At large widths, the table 
fits in two lines. At smaller widths (~250px), it wraps to three lines. At yet 
smaller widths (~180px), it fits in two lines again! And if you keep making it 
smaller, it goes back to three lines.

So, the question is, how should this table be laid out if the table width is 
unconstrained? Ideally I think we should choose the smallest width that 
minimizes the height, but that means we need to globally optimize a function 
with multiple local minima. That's fundamentally hard.

As a possible first step, when taking the precentage of an NS_UNCONSTRAINEDSIZE, 
always return some fixed very large value (independent of the %) so that if we 
add a lot of them up, we won't overflow, but we will always get a very large 
width. Then when we compute the desired size of a table cell, we can detect 
over-wide cells and invoke a more complex "desired size search" subroutine. This 
way we avoid penalizing the common case.

The big question is, exactly what should this subroutine do? We can try 
reflowing the cell into different widths and see how it fits, but how can we 
detect that it's fitting well?

A stopgap measure would be to force the table width to the width of its 
container if we see a strangely-wide child.
Comment 27 Robert O'Callahan (:roc) (email my personal email if necessary) 2000-07-17 10:43:05 PDT
As an authoring practice, using percentage widths inside table cells whose width 
is otherwise unconstrained is probably unwise. Those measurements depend on the 
width of the cell, but the width of the cell depends on those measurements. No 
wonder no browsers get it right.
Comment 28 dylang 2000-07-17 11:25:05 PDT
Follow the logic here.  Nested tables should inherit their sizes fairly easily. 
 The outer table has its percentage size translated into a hard pixel value.  
Then the next inner table has its size based on its percentage of the outer one. 
 And so on.  You could evaluate this recusesively.

Once you have a size for a table, it's simply a matter of dividing the space 
among the cells.  Then once you have a cell size and position within the table, 
you put the content into it.  This is where indentation and margins come in, and 
are relative to the size of the cell.

That's my reasoning behind it, but then I've never tried to program a layout 
engine (or browser for that matter) before :)
Comment 29 Robert O'Callahan (:roc) (email my personal email if necessary) 2000-07-17 11:28:42 PDT
That's great if the page author specified the table width. That already works 
fine in Mozilla. The problem is when the table width is not specified at all so 
the browser has to find a good width.
Comment 30 Bradley Robinson 2000-07-17 12:13:41 PDT
This bug has to do with a conflict between margin-left and text indent when in 
percentages within tables. Even when the width of the table is set, I don't 
think it works properly in Mozilla. Otherwise, this bug would be marked FIXED 
and you guys would be using a different bug # for the problem with 
unconstrained tables.

If there is a width:X% specified for a table, then it should be easy for it to 
get the amounts for margin-left:X% and text-indent:X% from the table's width, 
which is gotten from the page's width. If it isn't specified, maybe anything 
that is specified in percentages within these kinds of tables should be 
ignored. I don't think anybody would have something against doing this if the 
W3C forgot to mention that specifying things in percentages within tables that 
don't have specified widths or heights might not work correctly.
Comment 31 Jerry Baker 2000-07-17 12:18:01 PDT
Maybe I'm just dense, but eventually Mozilla has to layout a table that has no 
specified width right? Why not use that size to determine the %'s?
Comment 32 dylang 2000-07-17 14:30:21 PDT
"Otherwise, this bug would be marked FIXED and you guys would be using a 
different bug # for the problem with unconstrained tables."

So is there a bug open for unconstrained tables, or are unconstrained tables 
symptoms of the indent/margin percentage conflict?
Comment 33 Bradley Robinson 2000-07-17 14:33:14 PDT
Created attachment 11502 [details]
margin-left and text-indent in 600 pixel table
Comment 34 Robert O'Callahan (:roc) (email my personal email if necessary) 2000-07-17 14:36:09 PDT
The deal with margins is this: the text indent is set to a bogusly large value, 
as I explained earlier. Therefore the P element's contents are bogusly wide. The 
P element is "shrink wrapping" its contents here, so its margins are calculated 
so that the left margin will be 1% of the final P width, i.e. the left margin is 
set to bogusly wide too. The sting is that when we look to see what the minimum 
cell width should be (the "maxElementSize"), we scan the contained P element to 
see what its "maxElementSize" is; we find that it's the width of the string plus 
the margins on the element! So we incorrectly decide that the element needs to 
be incredibly wide.

I think this means we should change nsBlockReflowContext::PlaceBlock so that it 
recomputes the margin values based on the maxElementSize (i.e. the minimum width 
required for the block) instead of the actual width.
Comment 35 Robert O'Callahan (:roc) (email my personal email if necessary) 2000-07-17 14:44:37 PDT
There are a number of different bugs falling over each other here:
1) percentage text-indent doesn't inherit in the right way (bug 45631)
2) percentage text-indent doesn't incrementally reflow correctly (bug 45631)
3) table cells containing percentage text-indent and percentage width 
inline-replaced elements don't size correctly
4) the minimum width of shrink-wrapped blocks with percentage margins isn't set 
correctly

Bugs 2), 3) and 4) are all affecting this bug report. Bug 4) is probably causing 
the worst of the trouble. Thanks for pointing that out.

Bugs 1) and 2) really need to be solved together; they're tricky, but solvable. 
Bug 3) is a nightmare and probably deserves its own Bugzilla bug. Bug 4) is 
definitely solvable and should probably stay here.
Comment 36 Robert O'Callahan (:roc) (email my personal email if necessary) 2000-07-17 14:50:32 PDT
Bradley: the problem is that you can't just use the table width to compute the 
margin and text-indents, for two reasons. First, if the minimum width of the 
columns is wider than the specified table size, Mozilla will make the table 
wider to fit. I don't know if that's a good decision, but it seems to have been 
done deliberately. Also, if there's more than one column you have to figure out 
how to distribute the space and that's not easy.

Jerry: Apart from the margin bug, the problem is all about how to choose the 
table width. If that's done correctly, everything else should work. But it seems 
terribly difficult to do "correctly", if indeed there is any correct way to do 
it.
Comment 37 Robert O'Callahan (:roc) (email my personal email if necessary) 2000-07-17 18:04:35 PDT
Created attachment 11511 [details] [diff] [review]
Patch to nsBlockReflowContext::DoReflowBlock, nsBlockReflowContext::PlaceBlock
Comment 38 Robert O'Callahan (:roc) (email my personal email if necessary) 2000-07-17 18:10:46 PDT
The patch fixes the margin issues. The tables in this bug's testcases and on 
other sites seem to work OK. text-indent in tables still doesn't really work, 
but at least things don't get out of control.
Comment 39 Robert O'Callahan (:roc) (email my personal email if necessary) 2000-07-17 18:12:14 PDT
Created attachment 11512 [details]
Screenshot of working table at www.thock.com/Dylan
Comment 40 Robert O'Callahan (:roc) (email my personal email if necessary) 2000-07-18 12:59:28 PDT
I have another idea for getting a reasonable desired size for table cells 
containing percentage-width elements.

First, set things up so that when we reflow a table cell with unconstrained 
width to find its desired width, we can specify as a parameter the actual 
(very large) width that percentage-width children will base themselves on (the 
"putative width"). Right now they (or at least some of them) take a percentage 
of the value of NS_UNCONSTRAINEDWIDTH, which is just dumb.

Now, it seems to me that the desired width of a table cell in terms of this 
putative width should be a linear function, i.e. the desired width will normally 
consist of some fixed width "x" plus some fraction "f" of the putative width. I 
have a hard time thinking of content which could violate this rule (assuming the 
putative width is very large).

If so, then we can do the following: first reflow the cell with unconstrained 
width and a very large putative width W. If the resulting desired width D is 
not unreasonably large, then we're done. Otherwise reflow the cell again with 
unconstrained width and putative width W/2, obtaining desired width E. Then, by 
the assumption,
D = x + fW and E = x + fW/2
Solving the equations, we get
f = 2(D - E)/W and x = 2E - D
We'd check 0 < f < 1 and 0 < x and bail out (or do something else special) if 
not. Otherwise we can go ahead and compute the true desired width "T" by solving 
the equation
T = x + fT
i.e. set T = x/(1 - f)

Should be fast, robust and give good results.
Comment 41 Jason Eager 2000-07-24 15:05:28 PDT
I'll start looking at this.
Comment 42 Robert O'Callahan (:roc) (email my personal email if necessary) 2000-07-24 15:13:29 PDT
What do you plan to look at?

My patch fixes the issues with percentage margins in the presence of 
percentage-width content making the minimum cell width unreasonably large. That 
patch should be enough to consider this bug "fixed".

The desired cell size calculation is still broken in the presence of 
percentage-width content, but it turns out that all the ideas I posted here are 
flawed and will give bad results. I have other ideas but they're complex and 
slow and the problem is so hard, it's really not worth fixing.
Comment 43 Jason Eager 2000-07-24 15:24:29 PDT
So has your patch been committed? Should we open another bug to work on desired
cell size
computation?

I'm just looking for other things to work on in Mozilla, and I was pointed to
this bug.

Should we add "Fix in hand" to the status whiteboard?
Comment 44 Robert O'Callahan (:roc) (email my personal email if necessary) 2000-07-24 15:44:50 PDT
The patch hasn't been committed. I'm actually going to submit a new patch, which 
is slightly lower risk. If you can try it out, verify that it fixes the bug and 
doesn't cause any new problems, and report so here, that would be very helpful. 
Then I can seek review and approval and check it in myself.

Filing a new bug about choosing correct desired cell widths in the presence of 
percentage-width content would be a fine thing to do, but please don't burn time 
on it. I don't think anyone really cares about it.
Comment 45 Matthew Paul Thomas 2000-07-25 06:25:05 PDT
If anyone files that cell-width bug, please CC me on it. Thanks.
Comment 46 Robert O'Callahan (:roc) (email my personal email if necessary) 2000-07-25 11:27:20 PDT
Created attachment 11896 [details] [diff] [review]
V2 of patch to fix maxElementSize of elements with percentage margins and percentage-width content
Comment 47 Robert O'Callahan (:roc) (email my personal email if necessary) 2000-07-25 11:33:24 PDT
I think that patch is checkin-worthy. However, let's wait until nsbeta2 has 
branched before we seek approval. This is minor stuff.

I'm stealing the bug from buster. I hope he doesn't mind :-).
Comment 48 Robert O'Callahan (:roc) (email my personal email if necessary) 2000-08-02 09:08:06 PDT
OK, can I have review and module owner/mozilla.org approval please? It's time to 
get this in. The patch is
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=11896
Comment 49 Kevin McCluskey (gone) 2000-08-02 11:12:34 PDT
CC'ing attinasi@netscape.com
Comment 50 Chris Waterson 2000-08-02 12:19:56 PDT
a=waterson
Comment 51 Hixie (not reading bugmail) 2000-08-09 22:43:23 PDT
Do we still need r=? Marc, assuming this falls into your module, could you
review the patch please? Thanks...
Comment 52 Marc Attinasi 2000-08-10 09:47:49 PDT
It would be better if Chris W. could review this, he is much more familiar with 
this are than I am. Chris, can you review the patch 
(http://bugzilla.mozilla.org/showattachment.cgi?attach_id=11896)?
Comment 53 Chris Waterson 2000-08-10 12:10:16 PDT
r=waterson, too!

roc, can you add an appropriate regression test to 
mozilla/layout/html/tests/table/bugs, too?
Comment 54 Robert O'Callahan (:roc) (email my personal email if necessary) 2000-08-11 13:29:54 PDT
Fix and testcase checked in.
Comment 55 dylang 2000-08-19 14:32:06 PDT
I have double checked several times with recent builds and, to my great joy, 
this indeed works fine (and fixes problems with rendering other things I was 
doing with CSS and HTML).  I'm glad that something originally marked as "future" 
was just a simple patch away from proper HTML/CSS conformance :)
Comment 56 Jerry Baker 2002-05-27 14:28:44 PDT
Mass removing self from CC list.
Comment 57 Jerry Baker 2002-05-27 14:53:16 PDT
Now I feel sumb because I have to add back. Sorry for the spam.

Note You need to log in before you can comment on or make changes to this bug.