Closed Bug 175455 Opened 21 years ago Closed 21 years ago
mouseover causing contents to move to the right
397 bytes, text/html
671 bytes, text/html
423 bytes, text/html
510 bytes, text/html
7.31 KB, patch
|Details | Diff | Splinter Review|
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
mouseover "Classic". each time, the table grows linux trunk build 20021017
regression between trunk builds 2002090312 and 2002090421 (same as bug 175310) marking NEW ==> tables
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?
OS: Linux → All
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.
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
this may be casued by hte patch for bug 154780
Alternate test URL: http://nexland.com/support/
*** Bug 181085 has been marked as a duplicate of this bug. ***
*** Bug 167425 has been marked as a duplicate of this bug. ***
*** Bug 181085 has been marked as a duplicate of this bug. ***
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
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.
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla1.4alpha
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.
Target Milestone: mozilla1.4alpha → mozilla1.3beta
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.
Attachment #107638 - Flags: review?(bernd.mielke) → review-
*** Bug 182856 has been marked as a duplicate of this bug. ***
Comment on attachment 107638 [details] [diff] [review] patch to fix expanding table cell marking - as per bernd's comments
Attachment #107638 - Flags: superreview?(roc+moz) → superreview-
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.
Shouldn't there be some "mFlags.mTableDerivedComputedWidth = PR_FALSE" in there?
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;
the patch should not alter the rendering of the testcase
That shoud be fairly easy to fix. Do you have any other test cases up your sleeve?
These patches also fix the regression test of bug 4576.
Attachment #108060 - Attachment is obsolete: true
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?
>>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.
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.
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.
>>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.
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.
>>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 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 :-) .
Attachment #108304 - Flags: review?(bernd.mielke) → review+
Comment on attachment 108304 [details] [diff] [review] revised patch to fix remaining issues OK. sr=roc+moz
Attachment #108304 - Flags: superreview?(roc+moz) → superreview+
*** Bug 186071 has been marked as a duplicate of this bug. ***
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.
(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.)
fixed on trunk.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
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>
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.
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.
I think this patch was a workaround for one of the block bugs I fixed recently and should be backed out.
You need to log in before you can comment on or make changes to this bug.