Cell background invisible (worked in Mozilla 1.2)

RESOLVED FIXED in Future

Status

()

Core
Layout: Tables
P2
normal
RESOLVED FIXED
15 years ago
9 years ago

People

(Reporter: Marek Jerzy Minakowski, Assigned: Bernd)

Tracking

({testcase})

Trunk
Future
x86
Windows XP
testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(7 attachments, 7 obsolete attachments)

(Reporter)

Description

15 years ago
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.
> 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?
(Assignee)

Comment 2

15 years ago
no, I have no idea, without a testcase its difficult to judge.
Keywords: qawanted
Well, for starters, is this a problem only in XSLT-created pages, or in normal standards mode HTML too?
(Reporter)

Comment 4

15 years ago
Created attachment 117403 [details]
In HTML it works
(Reporter)

Comment 5

15 years ago
Created attachment 117404 [details]
This is XSL with identical code which doesn't show image
(Reporter)

Comment 6

15 years ago
Created attachment 117405 [details]
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)
(Reporter)

Comment 7

15 years ago
Created attachment 117406 [details]
XML to make XSL work

(previously I submitted wrong XML)
Attachment #117405 - Attachment is obsolete: true
Created attachment 117429 [details]
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....
Attachment #117403 - Attachment is obsolete: true
Attachment #117404 - Attachment is obsolete: true
Attachment #117406 - Attachment is obsolete: true
Confirming....
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 10

15 years ago
removing KW : qawanted, since reduced testcase has been provided and bug confirmed.
Keywords: qawanted → testcase
Priority: -- → P2

Updated

15 years ago
Target Milestone: --- → Future
(Assignee)

Comment 11

13 years ago
Created attachment 158041 [details]
revised testcase with mozimage
(Assignee)

Updated

13 years ago
Attachment #117429 - Attachment is obsolete: true
(Assignee)

Comment 12

13 years ago
Created attachment 158042 [details] [diff] [review]
patch
Assignee: core.layout.tables → bernd_mozilla
Status: NEW → ASSIGNED
(Assignee)

Updated

13 years ago
Blocks: 246676
(Assignee)

Updated

13 years ago
Blocks: 264451
(Assignee)

Comment 13

13 years ago
Created attachment 169804 [details] [diff] [review]
revised patch
Blocks: 284844
(Assignee)

Updated

13 years ago
Attachment #169804 - Flags: superreview?(roc)
Attachment #169804 - Flags: review?(roc)
Don't you want

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

to be done differently if divisor was set to eligibleRows?
(Assignee)

Comment 15

13 years ago
Created attachment 178046 [details] [diff] [review]
next rev.
Attachment #169804 - Attachment is obsolete: true
Attachment #178046 - Flags: superreview?(roc)
Attachment #178046 - Flags: review?(roc)
(Assignee)

Updated

13 years ago
Attachment #169804 - Flags: superreview?(roc)
Attachment #169804 - Flags: review?(roc)
+      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 on attachment 178046 [details] [diff] [review]
next rev.

getting it off the radar while waiting
Attachment #178046 - Flags: superreview?(roc)
Attachment #178046 - Flags: superreview-
Attachment #178046 - Flags: review?(roc)
Attachment #178046 - Flags: review-
(Assignee)

Comment 18

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

Comment 19

10 years ago
Created attachment 278222 [details] [diff] [review]
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?
Attachment #278222 - Flags: review?(roc)

Comment 20

10 years ago
Jesse, in bug 284844 you said you hit that assertion frequently. What sites did you find triggered it?
+            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 on attachment 278222 [details] [diff] [review]
Continuing with bernd's approach

clearing request while waiting for comments to be addressed
Attachment #278222 - Flags: review?(roc)
(Assignee)

Comment 23

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

(Assignee)

Comment 24

10 years ago
I am taking the bug, I think I have something reviewable today.
(Assignee)

Comment 25

10 years ago
Created attachment 292320 [details] [diff] [review]
patch next rev

Hi Robert, I am at it again ;-)
Attachment #292320 - Flags: superreview?(roc)
Attachment #292320 - Flags: review?(roc)
Comment on attachment 292320 [details] [diff] [review]
patch next rev

+  PRInt32 elligbleRows = 0;

spelling: eligible

Fix lastElligibleRow while you're at it.
Attachment #292320 - Flags: superreview?(roc)
Attachment #292320 - Flags: superreview+
Attachment #292320 - Flags: review?(roc)
Attachment #292320 - Flags: review+
(Assignee)

Comment 27

10 years ago
Created attachment 292356 [details] [diff] [review]
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.
Attachment #292356 - Flags: review?(roc)
(Assignee)

Comment 28

10 years ago
Created attachment 292357 [details] [diff] [review]
patch with spelling fixed.
Attachment #292356 - Attachment is obsolete: true
Attachment #292357 - Flags: review?(roc)
Attachment #292356 - Flags: review?(roc)
Attachment #292357 - Flags: superreview+
Attachment #292357 - Flags: review?(roc)
Attachment #292357 - Flags: review+
(Assignee)

Comment 29

10 years ago
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/)
Attachment #292357 - Flags: approval1.9?

Updated

10 years ago
Attachment #292357 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 30

10 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED

Comment 31

10 years ago
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

10 years ago
Created attachment 294115 [details] [diff] [review]
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
(Assignee)

Comment 33

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

Updated

9 years ago
Depends on: 444702
You need to log in before you can comment on or make changes to this bug.