the table code tests for if ((NS_STYLE_OVERFLOW_HIDDEN == disp->mOverflow) || {http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/nsTableCellFrame.cpp#479 But it gets now NS_STYLE_OVERFLOW_SCROLLBARS_NONE

The solution is not to test for it in the table code. The solution is to do the correct frame construction that causes a scroll frame to be built.

I would have expected that the patch in bug 69355 has been tested against http://lxr.mozilla.org/seamonkey/search?string=overflow%3Ahidden before checkin in order to prevent unnecessary regressions.

I would have expected that the table code be reasonably designed and somewhat robust. But I guess I'm silly.

The basic problem here is that 'scroll' and 'auto' have never worked on table cells, and the working group decided to make 'hidden' work like 'scroll' and 'auto', so now 'hidden' doesn't work either.

I guess you know the code and its quality well enough to know that you need to test it. Btw the patch for bug 173277 that you sr'ed also contains NS_STYLE_OVERFLOW_HIDDEN. I am not certain that http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsFrame.cpp#4344 is still correct, that could also influence block layout.

I did test it. It just didn't occur to me to test table cells. And frankly, I think I would have landed it anyway even if I had known about this regression (which probably requires more changes to fix than were in the original patch), since I think making our basic overflow:hidden behavior compatible with other browsers is more important than making it work on table cells.

Overflow:hidden only works on row groups. It fails for tables, tr and td.

As a module owner, if you think it is more important to be comaptible to other browsers than to fix our implementation first, please do whatever you want.

It's not supposed to work on them: http://www.w3.org/TR/CSS21/visufx.html#overflow

I'll make this bug about the regression and file another one about the general problem (which I have no intention of fixing).

I filed bug 221154 for the general problem.

The code cited in comment 5 is currently correct. It will, given this workaround, cause a larger overflow area to be reported than necessary when 'overflow: hidden' is used on table cells, because the clipped content inside the table will count as overflow. However, I think that's an acceptable loss until bug 221154 is fixed.

Comment on attachment 132594 [details] [diff] [review] fix regression sr=bzbarsky... We should really try to reduce the code duplication (and mis-duplication) in frame construction, though.... :(

Comment on attachment 132594 [details] [diff] [review] fix regression I think the same should be done on table and row frames

Comment on attachment 132594 [details] [diff] [review] fix regression Checked in this patch, 2003-10-13 14:58 -0700.

The extraneous changes (InvalidateDamage) are adding |const| that would have prevented bustage that happened a few weeks ago.

Comment on attachment 133228 [details] [diff] [review] additional patch yikes. I realized I never landed this.

Comment on attachment 133228 [details] [diff] [review] additional patch a=asa (on behalf of drivers) for checkin to Mozilla 1.6

The additional patch does not seem to fix this testcase. Opera renders a green block with a black border and no red.

Comment on attachment 133228 [details] [diff] [review] additional patch Fix checked in to trunk, 2003-12-12 15:00 -0800.

Comment on attachment 133228 [details] [diff] [review] additional patch This have added a warning on brad TBox: layout/html/table/src/nsTableRowFrame.cpp:565 Unused variable `const nsStyleDisplay*disp' 563 PRBool clip = overflow == NS_STYLE_OVERFLOW_HIDDEN || 564 overflow == NS_STYLE_OVERFLOW_SCROLLBARS_NONE; 565 const nsStyleDisplay* disp = GetStyleDisplay(); 566 if (clip) { 567 aRenderingContext.PushState(); The line 565 should just be removed.

Marking this bug fixed. Filed comment 21 as bug 228709.