Last Comment Bug 197391 - Cell background invisible (worked in Mozilla 1.2)
: Cell background invisible (worked in Mozilla 1.2)
Status: RESOLVED FIXED
: testcase
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: Trunk
: x86 Windows XP
: P2 normal (vote)
: Future
Assigned To: Bernd
: Madhur Bhatia
:
Mentors:
http://www.przodkowie.com/1kal.xml
: 217677 (view as bug list)
Depends on: 444702
Blocks: 246676 264451 284844
  Show dependency treegraph
 
Reported: 2003-03-14 07:14 PST by Marek Jerzy Minakowski
Modified: 2008-07-10 22:06 PDT (History)
7 users (show)
bernd_mozilla: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
In HTML it works (465 bytes, text/html)
2003-03-16 09:06 PST, Marek Jerzy Minakowski
no flags Details
This is XSL with identical code which doesn't show image (650 bytes, text/xml)
2003-03-16 09:08 PST, Marek Jerzy Minakowski
no flags Details
XML to make XSL work (456 bytes, text/xml)
2003-03-16 09:11 PST, Marek Jerzy Minakowski
no flags Details
XML to make XSL work (378 bytes, text/xml)
2003-03-16 09:33 PST, Marek Jerzy Minakowski
no flags Details
HTML-only testcase showing problem (278 bytes, text/html)
2003-03-16 17:33 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
revised testcase with mozimage (286 bytes, text/html)
2004-09-06 12:36 PDT, Bernd
no flags Details
patch (1.77 KB, patch)
2004-09-06 12:38 PDT, Bernd
no flags Details | Diff | Splinter Review
revised patch (3.08 KB, patch)
2004-12-29 02:23 PST, Bernd
no flags Details | Diff | Splinter Review
next rev. (3.35 KB, patch)
2005-03-20 09:06 PST, Bernd
roc: review-
roc: superreview-
Details | Diff | Splinter Review
Continuing with bernd's approach (3.38 KB, patch)
2007-08-25 12:12 PDT, Ben Karel [eschew]
no flags Details | Diff | Splinter Review
patch next rev (5.74 KB, patch)
2007-12-09 10:08 PST, Bernd
roc: review+
roc: superreview+
Details | Diff | Splinter Review
patch to fix the spelling and the all rows case (6.28 KB, patch)
2007-12-09 21:14 PST, Bernd
no flags Details | Diff | Splinter Review
patch with spelling fixed. (6.29 KB, patch)
2007-12-09 21:19 PST, Bernd
roc: review+
roc: superreview+
mtschrep: approval1.9+
Details | Diff | Splinter Review
Slight improvement. (1.39 KB, patch)
2007-12-20 15:07 PST, Jeremy Lea
no flags Details | Diff | Splinter Review

Description Marek Jerzy Minakowski 2003-03-14 07:14:25 PST
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 [:bz] (still a bit busy) 2003-03-14 10:33:01 PST
> 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 2003-03-14 12:06:58 PST
no, I have no idea, without a testcase its difficult to judge.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2003-03-14 13:12:00 PST
Well, for starters, is this a problem only in XSLT-created pages, or in normal standards mode HTML too?
Comment 4 Marek Jerzy Minakowski 2003-03-16 09:06:42 PST
Created attachment 117403 [details]
In HTML it works
Comment 5 Marek Jerzy Minakowski 2003-03-16 09:08:32 PST
Created attachment 117404 [details]
This is XSL with identical code which doesn't show image
Comment 6 Marek Jerzy Minakowski 2003-03-16 09:11:16 PST
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)
Comment 7 Marek Jerzy Minakowski 2003-03-16 09:33:59 PST
Created attachment 117406 [details]
XML to make XSL work

(previously I submitted wrong XML)
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2003-03-16 17:33:44 PST
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....
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2003-03-16 17:36:39 PST
Confirming....
Comment 10 Madhur Bhatia 2003-03-24 16:35:34 PST
removing KW : qawanted, since reduced testcase has been provided and bug confirmed.
Comment 11 Bernd 2004-09-06 12:36:08 PDT
Created attachment 158041 [details]
revised testcase with mozimage
Comment 12 Bernd 2004-09-06 12:38:41 PDT
Created attachment 158042 [details] [diff] [review]
patch
Comment 13 Bernd 2004-12-29 02:23:06 PST
Created attachment 169804 [details] [diff] [review]
revised patch
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-03-14 15:05:01 PST
Don't you want

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

to be done differently if divisor was set to eligibleRows?
Comment 15 Bernd 2005-03-20 09:06:21 PST
Created attachment 178046 [details] [diff] [review]
next rev.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-03-21 19:01:25 PST
+      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) 2005-03-29 18:37:13 PST
Comment on attachment 178046 [details] [diff] [review]
next rev.

getting it off the radar while waiting
Comment 18 Bernd 2005-07-12 04:19:12 PDT
*** Bug 217677 has been marked as a duplicate of this bug. ***
Comment 19 Ben Karel [eschew] 2007-08-25 12:12:46 PDT
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?
Comment 20 Ben Karel [eschew] 2007-08-25 12:14:15 PDT
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) 2007-08-26 17:37:21 PDT
+            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) 2007-09-17 16:19:15 PDT
Comment on attachment 278222 [details] [diff] [review]
Continuing with bernd's approach

clearing request while waiting for comments to be addressed
Comment 23 Bernd 2007-12-09 07:06:10 PST
Ben please attach the reftests as an archive to make sure that they do not get lost.

Comment 24 Bernd 2007-12-09 08:03:49 PST
I am taking the bug, I think I have something reviewable today.
Comment 25 Bernd 2007-12-09 10:08:00 PST
Created attachment 292320 [details] [diff] [review]
patch next rev

Hi Robert, I am at it again ;-)
Comment 26 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-12-09 14:26:21 PST
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 2007-12-09 21:14:41 PST
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.
Comment 28 Bernd 2007-12-09 21:19:36 PST
Created attachment 292357 [details] [diff] [review]
patch with spelling fixed.
Comment 29 Bernd 2007-12-11 21:41:00 PST
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 2007-12-15 11:49:45 PST
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] 2007-12-15 21:43:22 PST
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 2007-12-20 15:07:08 PST
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
Comment 33 Bernd 2007-12-24 03:17:44 PST
I don't see the improvement, other than the performance penalty of calling GetRowCount for every row, which I avoided.

Note You need to log in before you can comment on or make changes to this bug.