Closed Bug 197391 Opened 19 years ago Closed 14 years ago

Cell background invisible (worked in Mozilla 1.2)

Categories

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

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
Future

People

(Reporter: minak, Assigned: bernd_mozilla)

References

()

Details

(Keywords: testcase)

Attachments

(7 files, 7 obsolete files)

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?
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?
Attached file In HTML it works (obsolete) —
Attached file XML to make XSL work (obsolete) —
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)
Attached file XML to make XSL work (obsolete) —
(previously I submitted wrong XML)
Attachment #117405 - Attachment is obsolete: true
Attached file HTML-only testcase showing problem (obsolete) —
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
removing KW : qawanted, since reduced testcase has been provided and bug confirmed.
Keywords: qawantedtestcase
Priority: -- → P2
Target Milestone: --- → Future
Attachment #117429 - Attachment is obsolete: true
Attached patch patchSplinter Review
Assignee: core.layout.tables → bernd_mozilla
Status: NEW → ASSIGNED
Blocks: 246676
Blocks: 264451
Attached patch revised patch (obsolete) — Splinter Review
Blocks: 284844
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?
Attached patch next rev.Splinter Review
Attachment #169804 - Attachment is obsolete: true
Attachment #178046 - Flags: superreview?(roc)
Attachment #178046 - Flags: review?(roc)
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-
*** Bug 217677 has been marked as a duplicate of this bug. ***
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)
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)
Ben please attach the reftests as an archive to make sure that they do not get lost.

I am taking the bug, I think I have something reviewable today.
Attached patch patch next revSplinter Review
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+
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)
Attachment #292356 - Attachment is obsolete: true
Attachment #292357 - Flags: review?(roc)
Attachment #292356 - Flags: review?(roc)
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?
Attachment #292357 - Flags: approval1.9? → approval1.9+
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
Closed: 14 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
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!
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
I don't see the improvement, other than the performance penalty of calling GetRowCount for every row, which I avoided.
Depends on: 444702
You need to log in before you can comment on or make changes to this bug.