221140 - 'overflow: hidden' on table cells broken

Status: RESOLVED FIXED

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

Description

[Bernd]

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

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 2

[Bernd]

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.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 5

[Bernd]

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.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 7

[Bernd]

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

Comment 8

[Bernd]

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.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: fix regression

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I filed bug 221154 for the general problem.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 14

[Boris Zbarsky [:bzbarsky]]

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 15

[Bernd]

Comment on attachment 132594 [details] [diff] [review]
fix regression

I think the same should be done on table and row frames

Comment 16

[Bernd]

Attachment: testcase that mozilla renders different than other browsers

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 132594 [details] [diff] [review]
fix regression

Checked in this patch, 2003-10-13 14:58 -0700.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: additional patch

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

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 133228 [details] [diff] [review]
additional patch

yikes.	I realized I never landed this.

Comment 20

[Asa Dotzler [:asa]]

Comment on attachment 133228 [details] [diff] [review]
additional patch

a=asa (on behalf of drivers) for checkin to Mozilla 1.6

Comment 21

[Mats Palmgren (inactive)]

Attachment: Testcase #2

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

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 133228 [details] [diff] [review]
additional patch

Fix checked in to trunk, 2003-12-12 15:00 -0800.

Comment 23

[Aleksey Nogin]

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.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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