197391 - Cell background invisible (worked in Mozilla 1.2)

Status: RESOLVED FIXED

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

Description

[Marek Jerzy Minakowski]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3) Gecko/20030312
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3) Gecko/20030312

In the right-upper corner there is a map with some localities marked by
asterisks. It is made with map as cell bacground and asterisk positioned via
CSS. In Mozilla 1.3 the map is invisible but it worked in Moz 1.2 and it works
with MS IE 6.x.

What is important, the page layout is in XSLT (client-side).

See http://www.przodkowie.com/1kal.xml

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> What is important, the page layout is in XSLT (client-side).

Which means it is now in standards mode....

Looks like the table is 200px tall, but the <tbody> inside it is 0px tall...
bernd, any ideas why?

Comment 2

[Bernd]

no, I have no idea, without a testcase its difficult to judge.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Well, for starters, is this a problem only in XSLT-created pages, or in normal standards mode HTML too?

Comment 4

[Marek Jerzy Minakowski]

Attachment: In HTML it works

Comment 5

[Marek Jerzy Minakowski]

Attachment: This is XSL with identical code which doesn't show image

Comment 6

[Marek Jerzy Minakowski]

Attachment: XML to make XSL work

I have uploaded 3 files: 1test.html which works OK and 1test.xml & 1test.xsl
with idendital HTML code which doesn't work (image in cell background is not
shown in Mozilla 1.3)

Comment 7

[Marek Jerzy Minakowski]

Attachment: XML to make XSL work

(previously I submitted wrong XML)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: HTML-only testcase showing problem

The issue seems to be the combination of 0 cellpadding (any other value works),
HTML attr height and width, and an empty cell....

Comment 9

[Boris Zbarsky [:bzbarsky]]

Confirming....

Comment 10

[Madhur Bhatia]

removing KW : qawanted, since reduced testcase has been provided and bug confirmed.

Comment 11

[Bernd]

Attachment: revised testcase with mozimage

Comment 12

[Bernd]

Attachment: patch

Comment 13

[Bernd]

Attachment: revised patch

Comment 14

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

Don't you want

 float percent = rowRect.height / ((float)divisor);

to be done differently if divisor was set to eligibleRows?

Comment 15

[Bernd]

Attachment: next rev.

Comment 16

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

+      divisor = pctBasis ? pctBasis : GetRowCount();    

What should the numerator be in these cases? You seem to be still using
rowRect.height even when the divisor is GetRowCount().

Maybe you could put a comment somewhere indicating what ratios are used under
what conditions.

Comment 17

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

Comment on attachment 178046 [details] [diff] [review]
next rev.

getting it off the radar while waiting

Comment 18

[Bernd]

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

Comment 19

[Ben Karel [eschew]]

Attachment: Continuing with bernd's approach

I renamed |divisor| to |totalHeight|, because that's really what it is.

Also, I have reftests. I think they ought to go in their own directory: layout/reftests/table-height/, but I don't have commit access and so can't add new directories. Should I stick them in reftests/table-width/, or reftests/bugs/, or attach in a tarball?

Comment 20

[Ben Karel [eschew]]

Jesse, in bug 284844 you said you hit that assertion frequently. What sites did you find triggered it?

Comment 21

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

+            percent = rowRect.height / ((float)totalHeight);
+            percent = 1 / ((float)unStyledRowCount);

Constructor cast

+          // ... unless the row is unstyled and has no height, in which case it splits
+          // the available space with the other unstyled rows
+          if (rowRect.height > 0 && totalHeight > 0)

I'm confused about what's going on here. Shouldn't we start using unStyledRowCount (which should be "unstyledrowCount" --- "un" isn't a word) only when totalHeight == 0? As it is, it seems some rows will get percentages ("percent" is also a bad name, it should be "fraction" or something) based on their height, and others get 1/unstyledRowCount, so these won't add up to 1 in general.

Comment 22

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

Comment on attachment 278222 [details] [diff] [review]
Continuing with bernd's approach

clearing request while waiting for comments to be addressed

Comment 23

[Bernd]

Ben please attach the reftests as an archive to make sure that they do not get lost.

Comment 24

[Bernd]

I am taking the bug, I think I have something reviewable today.

Comment 25

[Bernd]

Attachment: patch next rev

Hi Robert, I am at it again ;-)

Comment 26

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

Comment on attachment 292320 [details] [diff] [review]
patch next rev

+  PRInt32 elligbleRows = 0;

spelling: eligible

Fix lastElligibleRow while you're at it.

Comment 27

[Bernd]

Attachment: patch to fix the spelling and the all rows case

Robert, I did a mistake in the patch, now I commented the thing to death. Could you please look again at the second part where the ratio is computed. Thanks.

Comment 28

[Bernd]

Attachment: patch with spelling fixed.

Comment 29

[Bernd]

Comment on attachment 292357 [details] [diff] [review]
patch with spelling fixed.

I would like to get this into 1.9. It  will reduce the amount of asserts (it #4 in assert frequency http://bclary.com/blog/2007/10/29/assert-this-top-site-assertions-october-27-2007/)

Comment 30

[Bernd]

fix checked in.


Ben, please attach the test cases or send them to me. It would save me a lot of time and we can't afford loosing test cases.  Sorry for jumping so on your patch but I am very time constrained and this one was just embarrassing to have it still lingering around.

Comment 31

[Ben Karel [eschew]]

Bernd, I put a bunch of testcases online: http://eschew.org/projects/mozilla/testcases/284844/ .
They're not reftests, sorry. Still, I hope they're better than nothing. Apologies for not linking sooner, I didn't see your request last week. And I was neglecting the bug, so thanks for fixing it!

Comment 32

[Jeremy Lea]

Attachment: Slight improvement.

Hi,

I've attached a patch which I think is a slight improvement to the logic here.  I'm not sure it's right.

  -Jeremy

Comment 33

[Bernd]

I don't see the improvement, other than the performance penalty of calling GetRowCount for every row, which I avoided.