175455 - mouseover causing contents to move to the right

Status: RESOLVED FIXED

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

Description

[Theblueman]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b) Gecko/20021018
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b) Gecko/20021018

When you visit www2.station.sony.com/en/ and you mouseover the game titles
(light blue), the contents to the right of the mouse position shifts to the right.

Reproducible: Always

Steps to Reproduce:
1.go to website www2.station.sony.com/en/
2.move mouse over the different game catagories
3.

Actual Results:  
after moving over the different game catagories I can no longer see the contents
to the right of where the mouse was

Comment 1

[Andrew Schultz]

Attachment: testcase

mouseover "Classic".  each time, the table grows
linux trunk build 20021017

Comment 2

[Andrew Schultz]

regression between trunk builds 2002090312 and 2002090421 (same as bug 175310)

marking NEW
==> tables

Comment 3

[Russell Odom]

Confirming on W2K 2002101612 (1.2b). Will attach a minimal testcase.
Seems to be to do with the box for the <a href...> not shrinking again after
it's been stretched by the :hover CSS rule.

Dammit, midair collision! Might it be a Layout bug though, rather than
HTMLTables - as far as I can tell, it's the <a href...> that's growing, not the
table cell?

Comment 4

[Russell Odom]

Attachment: Another testcase

Well, I spent ages on it, and I'm not gonna drop it just cos someone got there
first :-) It's the :hover that triggers the stretching of the table cell, which
doesn't seem to shrink again when the mouse is moved off.

Comment 5

[Stefan [:stefanh]]

Confirming with the latest nightly 2002101808 on Mac OS 9.1

Bug also occurs with 2002091014 (1.2a)

Looks like the sidemovement is of the same size as "padding-left:"

Bug 175310 is WFM on both builds

Comment 6

[Bernd]

this may be casued by hte patch for  bug 154780 

Comment 7

[lohphat]

Alternate test URL: http://nexland.com/support/

Comment 8

[Amarendra Hanumanula]

*** Bug 181085 has been marked as a duplicate of this bug. ***

Comment 9

[Amarendra Hanumanula]

*** Bug 167425 has been marked as a duplicate of this bug. ***

Comment 10

[Andrew Schultz]

*** Bug 181085 has been marked as a duplicate of this bug. ***

Comment 11

[Bernd]

 the suspicous checkins are 
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla%2Flayout&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=09%2F03%2F2002+11%3A00%3A00&maxdate=09%2F04%2F2002+21%3A00%3A00&cvsroot=%2Fcvsroot

Comment 12

[karnaze (gone)]

I think one of those checkins just exposed a bug. If you run the testcase on an
older build (e.g. the m1.0 branch) you'll see that there is a missing reflow and
the bold text overwrites the right border. Removing the regression keyword.

Comment 13

[karnaze (gone)]

Attachment: patch to fix expanding table cell

The patch fixes the problem where the table cell grows in width after each
hover. There is still a problem where after the hover, the cell doesn't return
to a smaller width, but I think this is a problem with the block not returning
the correct mes or max width during the incremental reflow, and I'll file
another bug on that.

Comment 14

[Bernd]

Attachment: first testcase plus a <i></i> which will not be fixed by the patch

Comment 15

[Bernd]

Comment on attachment 107638 [details] [diff] [review]
patch to fix expanding table cell

I think the patch fixes only the testcases not the nexland.com/support url. As
shown in the slightly modified testcase it wallpapers only over the problem. I
believe the real problem behind is that the initial idea of basic table layout
strategy was to call it once at the begining after an unconstrained reflow. If
we now rebalance we do not have a unconstrained reflow before. In principle we
would need after a NeedStrategyInit a new unconstrained reflow to get a new
balance with correct desired and computed width's, bug 131975 is caused by this
problem too.

Comment 16

[Andrew Schultz]

*** Bug 182856 has been marked as a duplicate of this bug. ***

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 107638 [details] [diff] [review]
patch to fix expanding table cell

marking - as per bernd's comments

Comment 18

[karnaze (gone)]

Attachment: revised patch

This patch deals with the problem where contents inside a table cell have
percentage widths which cause the cell to expand beyond the width that the cell
determined based on non-percentage constraints.  

It fixes both test cases as well as doing a better job of laying out regression
test cases in bugs 131020, 137388. I can't see the problems on nextland.com
without the patch, but if that page is not relatated to what this patch fixes,
and is still a problem, then we need to file a new bug.

This patch adds the mFlags.mTableDerivedComputedWidth bit to the reflow state.
The bit is set for a cell block and inherited from the parent otherwise. The
bit gets reset if the frame has a real non-percent style width. If the bit is
set, the computed width of the frame is restricted to available width (minus
border and padding).  


>>I think the patch fixes only the testcases not the nexland.com/support url.
As
>>shown in the slightly modified testcase it wallpapers only over the problem.
I

This patch adds the wallpaper, drywall, studs, insulation, and exterior stucco
to the wall, I think.

>>believe the real problem behind is that the initial idea of basic table
layout
>>strategy was to call it once at the begining after an unconstrained reflow.
If
>>we now rebalance we do not have a unconstrained reflow before. In principle
we
>>would need after a NeedStrategyInit a new unconstrained reflow to get a new

We could solve most of our layout problems by doing unconstrained reflows over
again, but that would kill performance, and in this case wouldn't even
completely solve the problem. Currently if we have a 100% thingy with some
border/padding inside a table cell, we make the cell wider than it should be.
The patch doesn't result an any extra reflows.

>>balance with correct desired and computed width's, bug 131975 is caused by
this
>>problem too.

I looked at bug 131975 and believe it to be unrelated. That bug looks like a
block problem where during an unconstrained reflow the block reports a desired
size which doesn't match the max size it returns on a subsequent incremental
reflow.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Shouldn't there be some "mFlags.mTableDerivedComputedWidth = PR_FALSE" in there?

Comment 20

[karnaze (gone)]

There are supposed to be 2, after each of the constructors that don't take a
parent and after the following line. I guess I forgot to add them when I moved
some code around.

+  mFlags.mSpecialHeightReflow = PR_FALSE;

Comment 21

[Bernd]

Attachment: next testcase

the patch should not alter the rendering of the testcase

Comment 22

[karnaze (gone)]

That shoud be fairly easy to fix. Do you have any other test cases up your sleeve?

Comment 23

[karnaze (gone)]

Attachment: revised patch to fix remaining issues

These patches also fix the regression test of bug 4576.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't really understand how this patch is supposed to fix the underlying
problem, which seems pretty deep. Isn't the underlying problem that the minimum
size of the cell depends on its content, but the content's size depends on the
actual size of the cell, so we have a circular chain of layout constraints?

Comment 25

[karnaze (gone)]

>>Isn't the underlying problem that the minimum
>>size of the cell depends on its content, but the content's size depends on the
>>actual size of the cell, so we have a circular chain of layout constraints?

The point of the patch is that this shouldn't be the underlying problem
(actually it is the desired size, not the minimum size that is relevant).

From comment #18

>>This patch deals with the problem where contents inside a table cell have
>>percentage widths which cause the cell to expand beyond the width that the cell
>>determined based on non-percentage constraints.  

Let me elaborate. 

The table cell determines its min, desired, and max widths during the pass 1
unconstrained reflow of its cell block which reflows the cell contents. During
this reflow, there is no computed width on the cell block and thus percentage
width based contents are not using it (of course they may be inside something
that has a computed width and thus have one, but it isn't from the table cell).
So, the min, desired, and max widths of the cell are not based on any percentage
based content where the percentage basis is the table cell.

During the 2nd pass (and incremental) the table determines the column widths and
then each cell reflows its cell block with a computed width based on the final
column widths. But if during this reflow there is someting inside the cell that
relies on the cell being the percentage basis, then it doesn't make sense to
allow that to cause the cell to be bigger than it wants to be, since the cell
determined how big it wants to be by not considering percentage based content
where the percentage basis is the table cell.

These kinds of problems arise because our tables (1) (for legacy and practical
reasons) allow percentage things inside cells to size based on the final size of
table columns, not necessarily the computed width of a table cell (even that
could change due to other constraints) and (2) for the most part don't allow
things to overflow outside of cells like opera. If we did what opera does, then
this patch wouldn't be necessary, but then we wouldn't be compatible with IE or
Nav4.x and it would require a big change to our system. Maybe at some point that
should be our standards mode behavior.

Comment 26

[karnaze (gone)]

I'm thinking of adding some similar code to handle the remaining problems we are
having with over constrained percentage heights inside table cells. I haven't
tracked down the bugs yet, but I remember one where we have dataloss. The
concept would be similar - don't allow a computed height (during the final pass)
to exceed how high the cell wants to be. However, there's one additional
complication in that even if this is enforced, there could be percentage things
stacked on top of each other and exceed the height the cell wants to be. Maybe
in this case the thing to do is reflow again and not give the cell a computed
height and thus not honor any of the percentages. 

Also, it seems that when authors specify a percentage width or height inside a
table cell, they really want it to be box-sizing:border-box but only tables and
form controls have that by default. I wonder if a series of style rules could be
written to do this. 

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

1) makes sense to me.

For 2) it seems to me the most logical thing is to allow table cells to
overflow. How incompatible would that really be in practice, since it could only
affect cells with percentage-width children where the children could not be
wrapped to extra lines?

If we want to avoid cell overflow then I think we need to think hard about how
exactly we're going to avoid it.

Regardless of this decision, it is definitely wrong for incremental reflow to
not have the same result as reflowing from scratch, and I think that's what the
original bug is about. In this context, with the code we currently have, it
seems like the incremental reflow needs to reflow with the cell width set to the
original final column width, not the actual cell width after overflow was taken
into account.

I don't really understand why this is an issue with cell heights. Do we support
constrained-height tables?

Thanks for explaining all this.

Comment 28

[karnaze (gone)]

>>For 2) it seems to me the most logical thing is to allow table cells to
>>overflow. How incompatible would that really be in practice, since it could only
>>affect cells with percentage-width children where the children could not be
>>wrapped to extra lines?

I don't think it is a really big task, maybe a few days, but please don't hold
up this bug for that, because as I have said, we still want the current behavior
in quirks mode.

>>If we want to avoid cell overflow then I think we need to think hard about how
>>exactly we're going to avoid it.

That's exactly what I have been doing with this patch.

>>Regardless of this decision, it is definitely wrong for incremental reflow to
>>not have the same result as reflowing from scratch, and I think that's what the
>>original bug is about. In this context, with the code we currently have, it
>>seems like the incremental reflow needs to reflow with the cell width set to the
>>original final column width, not the actual cell width after overflow was taken
>>into account.

I agree and don't think the patch gives different results for incremental reflow
versus regular reflow unless you have a test case that shows otherwise. As I
mentioned in comment #13, there is an existing problem with the patch (and
without the patch there is a problem also) where the incremental reflow causes
the cell to get bigger because of the change to bold (and yes, this is the
correct behavior, contrary to what the bug says, because the text is getting
longer), but it doesn't get smaller again after that. This is a block bug.

>>I don't really understand why this is an issue with cell heights. Do we support
>>constrained-height tables?

Yes, and we also support percentage things based on the final height of rows
even if the rows/cells don't have a style height.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So, the underlying problem is that when a cell grows to accomodate overflowing
content, and then is incrementally reflowed, incremental reflow uses the new
cell width as the available width for percentage-width children, instead of
using the old cell width. That is why incremental reflow gets a different result
from the original reflow.

You want to fix this by simply preventing overflow so the cell doesn't grow.

Is that a fair summary?

I think we should fix it by altering incremental reflow so it uses the old cell
width as the available width for percentage-width children, so the cell may
overflow, or not, but incremental reflow will do the right thing in either case.

Comment 30

[karnaze (gone)]

>>So, the underlying problem is that when a cell grows to accomodate overflowing
>>content, and then is incrementally reflowed, incremental reflow uses the new
>>cell width as the available width for percentage-width children, instead of
>>using the old cell width. That is why incremental reflow gets a different result
>>from the original reflow.

>>You want to fix this by simply preventing overflow so the cell doesn't grow.

>>Is that a fair summary?

Yes. But realize that this is the correct quirks mode behavior and there is no
resulting data loss.  

>>I think we should fix it by altering incremental reflow so it uses the old cell
>>width as the available width for percentage-width children, so the cell may
>>overflow, or not, but incremental reflow will do the right thing in either case.

I was going to do it this way initially but don't think it is the right
approach, because it will result in something that is neither quirks mode nor
the standards mode (I guess you could call it that) that I would like to see. In
addition, it leads to all kinds of complexities. For example, if you store that
original cell width, it is hard to tell during an incremental reflow when that
original cell width should change in size (did one of those funny percent width
thingys cause it to change size or did some real content cause it to change?).
With the patch, all reflows including incremental reflows do the right thing
consistent with quirks mode behavior.

I think the right approach is what I have been proposing. That is, (1) for
quirks mode (which is really what tables support for the most part currently,
and which most people expect from tables, since they don't expect to see things
sticking out of table cells, like opera provides) use this patch and don't allow
these percentage based things to influence a table cell's final width at all
(this is consistent with the rest of layout and the spec), and (2) for standards
mode, don't allow a percentage on a descendant to influence the cell's width
(i.e just like quirks mode) and either show the overflow or not depending on
what the overflow property is. In standards mode it can be implemented by (1)
never supplying one of these artifical computed widths when the cell doesn't
have a real style width and (2) only honoring the cell's real style width when
it has one.

Comment 31

[Bernd]

Comment on attachment 108304 [details] [diff] [review]
revised patch to fix remaining issues

I got enough lessons in reflow, thanks chris. I believe this cuts the  chicken
vs egg problem and hmmm I know the owner of bug who promised to investigate the
overflow mechanism in tables. I think that solution has also seen some testing
:-) .

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 108304 [details] [diff] [review]
revised patch to fix remaining issues

OK. sr=roc+moz

Comment 33

[Andrew Schultz]

*** Bug 186071 has been marked as a duplicate of this bug. ***

Comment 34

[David Baron :dbaron:]

For the record, I agree with roc's proposal in comment 29.  In particular, the I
think the width used for percentage-width children should be derived from the
maximum width and max element size of the cells in the table, which should not
depend on any of the final assigned widths.

Comment 35

[David Baron :dbaron:]

(Also note that we'd get the behavior I suggest "for free" if we separate the
computation of max-width and min-width (i.e., what we call max-element-size)
from Reflow.)

Comment 36

[karnaze (gone)]

fixed on trunk.

Comment 37

[karnaze (gone)]

David, I think the last sentence in comment 34 implies that the div's below
would not take up all of the horizontal space in the cells (Mozilla, IE, Opera
do take up that space).

<table width=200 border>
 <tr>
  <td>foo</td>
  <td>bar</td>
 <tr>
  <td>
   <div style="width:100%; border:1px solid red">foo</div></td>
  <td>
   <div style="width:100%; border:1px solid red">bar</div></td>
 </tr>
</table>

Comment 38

[David Baron :dbaron:]

No, it doesn't.  The cell sizes are computed based on min-width / max-width
calculations.  Then the contents of the cells are laid out based on those sizes.

Comment 39

[Bernd]

Just for the record, I don't agree with dbarons view in comment #34 and I am
scared that going the way david proposes will cause a lot of pages  with tables
to regress. I believe the table concept extends the block concept and trying to
reduce the table reflow to a block mechanism will not work. The heart of this
extension is the MEW and a two stage reflow process. I can't follow the
argumentation of comment #38. As far as I understand it, the table would use the
table size as a basis for the computation. On the first reflow the width of the
div will not be computed as it is an unconstrained reflow. On the second reflow
the space will be distributed between cells as the table has a specified width
so the percentage width of the divs depends on the specified widths as Chris told.

Comment 40

[David Baron :dbaron:]

I think this patch was a workaround for one of the block bugs I fixed recently
and should be backed out.

Comment 41

[David Baron :dbaron:]

(The block bugs I'm referring to are bug 217369, bug 39683, and bug 215857.)