Closed Bug 431403 Opened 18 years ago Closed 3 years ago

distribution of height from rowspan cells should prefer rows without fixed heights

Categories

(Core :: Layout: Tables, defect)

Product:
Core
Component:
Layout: Tables
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
RESOLVED INVALID

People

(Reporter: ihatespam.jf, Unassigned)

References

(Depends on 1 open bug,
URL
)

Details

(Keywords: regression, testcase)

Attachments

(6 files, 2 obsolete files)

testcase2
1.33 KB, text/html
Details
unminimized testcase, w/ no linebreaks, zipped up
9.94 KB, application/zip
Details
screenshot of unminimized testcase in trunk (broken)
6.03 KB, image/png
Details
screenshot of unminimized testcase in branch (works)
6.13 KB, image/png
Details
my minimized testcase
276 bytes, text/html
Details
screenshot of martijn's "testcase2" in 2023: IE8, Epiphany 42.4 (WebKit), Firefox Nightly 115, Chromium dev 115
92.60 KB, image/png
Details
User-Agent: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.8.1.14) Gecko/20080416 Fedora/2.0.0.14-1.fc7 Firefox/2.0.0.14 Build Identifier: Firefox3 preRC1 firefox-3.0pre.en-US.linux-i686 The image at http://www.gmp-vfx.com/MOZILLA/firefox-3.0RC1pre.png shows how tables are not correctly scaling background and other inages placed in cells correctly. This image from firefox-2.0.0.14 shows the image rendering properly: http://www.gmp-vfx.com/MOZILLA/firefox-2.0.0.14.png Reproducible: Always Steps to Reproduce: 1. Click on the links above 2. Bob's your uncle. Actual Results: Improper rendering of table Expected Results: Expected results can be seen in the image from forefox-2.0.0.14 as noted above. The code which is responsible for these overlays is done using the popular prototype.js scripts. Here's the php we use to draw the popup tips: echo "<script type='text/javascript'>\n"; echo "new Tip(document.getElementById('".$ObjectID."'), "; //echo "new Tip(".$ObjectID.", "; // WRAPPER TABLE FOR TOOLTIP BACKGROUND IMAGES echo "'<table border=\"0\" cellpadding=\"0\" cellspacing=\"0\">"; echo "<tr>"; echo "<td><img src=\"../".$ImgPath."backgrounds/tooltip_corner_ul.png\" /></td>"; echo "<td background=\"../".$ImgPath."backgrounds/tooltip_top.png\"></td>"; echo "<td><img src=\"../".$ImgPath."backgrounds/tooltip_corner_ur.png\" /></td>"; echo "</tr>"; echo "<tr>"; echo "<td background=\"../".$ImgPath."backgrounds/tooltip_side_l.png\"></td>"; echo "<td background=\"../".$ImgPath."backgrounds/tooltip_body.png\" rowspan=\"3\" valign=\"top\">"; //START INNER TOOLTIP TEXT DISPLAY TABLE echo "<table class=\"prqTipTable\">"; echo "<tr>"; //Spacer echo "<td class=\"prqTipHeader\" width=\"10px\">&nbsp</td>"; //Caption echo "<td class=\"prqTipHeader\" colspan=\"3\">"; echo $Caption; echo "</td>"; //Spacer echo "<td class=\"prqTipHeader\" width=\"10px\">&nbsp</td>"; echo "</tr>"; // Row Spacer echo "<tr height=\"10px\"><td class=\"prqTipData\"></td></tr>"; //Prerequisites for($p=0; $p<count($OpenPreqs); $p++){ echo "<tr>"; //Spacer echo "<td class=\"prqTipData\" width=\"10px\">&nbsp</td>"; //Asset if not same echo "<td class=\"prqTipData\" >"; if($OpenPreqs[$p]['ASSET_ID'] != $AssetID){ echo "<B>"; echo $OpenPreqs[$p]['ASSET_NAME']; echo "</B>&nbsp;&nbsp;"; } echo "</td>"; //Step echo "<td class=\"prqTipData\" >"; echo $OpenPreqs[$p]['DISPLAY_STEP']; echo "&nbsp;&nbsp;</td>"; //Due echo "<td class=\"prqTipData\" >"; if($OpenPreqs[$p]['TXT_DUE'] == NULL){ echo "<font color=#FF0000>"; echo translate("F_popupBuildPrereqs", 2, "Unscheduled"); echo "</font>"; } else if(isLate($OpenPreqs[$p]['TXT_DUE'])){ echo "<font color=#FF0000>"; echo translate("F_popupBuildPrereqs", 1, "Due")."&nbsp;"; echo format_date($DateFormatOx, $OpenPreqs[$p]['TXT_DUE']); echo "</font>"; } else { echo translate("F_popupBuildPrereqs", 1, "Due")."&nbsp;"; echo format_date($DateFormatOx, $OpenPreqs[$p]['TXT_DUE']); } echo "&nbsp;&nbsp;</td>"; //Assigned to echo "<td class=\"prqTipData\" >"; echo "<b>Assigned to:</b>&nbsp;"; if($OpenPreqs[$p]['ASSIGN_TO'] == NULL){ echo "<font color=#FF0000>"; echo "-".translate("F_popupBuildPrereqs", 3, "Unassigned")."-"; echo "</font>"; } else { echo $OpenPreqs[$p]['FNAME']."&nbsp;".$OpenPreqs[$p]['LNAME']; } echo "&nbsp;&nbsp;</td>"; echo "</tr>"; }
Severity: normal → major
Flags: blocking-firefox3?
Version: unspecified → Trunk
Looking at this one more time it sees that perhaps the images are rendering correctly but the cells are not formatted to the correct height. In any event we have behavior different from FF2.0.0.x and should probably get a good once over to see if there's a bug here. The details on the prototype framework are here: http://www.prototypejs.org/
Component: General → Layout: Tables
Flags: blocking-firefox3?
Product: Firefox → Core
QA Contact: general → layout.tables
--> Core::Layout:Tables
Flags: blocking1.9?
Is there a URL where we can see the bug?
It's on a proprietary system with customer data in it. I can expose it to you if I create a login. is there a way to take the discussion on this offline so I can set that up with some pretense of security? J
If you do File -> Save Page As, choose "Web page, complete", and then sanitize the data in the page that's saved, do you see the bug in what is saved? If so, you could zip it up and attach it. (Or, even better, simplify it to the smallest piece of HTML+CSS that shows the bug.)
David, As we're in production at the moment it's actually easier to just grant you access if you have an alternate contact e-mail. I can simply send the link and login without exposing client data, however I'm not too terribly interested in posting a link to our internal database server to the whole world. J
Is this seen only on Linux, or is it present on Win32 and OSX as well?
(Not blocking on unconfirmed bugs - please attach a testcase and renom!)
Flags: blocking1.9? → blocking1.9-
Mike, We have no time to go digging out an isolated test case. In fact our schedule here in the office just got worse. I offered to grant access to our internal server but no one came back with an e-mail address. We only use linux here so testing on other platforms will be up to people who deploy other OS's. If someone wants to provide an e-mail address for testing on our server we can make our server available to them and e-mail them access information.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050906 Minefield/3.0pre Confirming bug. Reporter has sent me a test account and i can verify the images from the attachment. Martijn, do you have time to put together a simplified testcase outlining the problem? I'll forward you the account information offline.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached file unminimized testcase, zipped up (obsolete) —
I don't have time now to minimized further, until after the weekend. This is what I have thus far, if someone wants to minimize further, that would be great.
Attached file testcase1 (obsolete) —
One thing that has changed on trunk is that the text wraps in the table in this case, while it doesn't do that on branch, nor in IE. This is probably something that is alreaday known (maybe deliberate?). But this might not at all be related to the changed position of the background images on trunk. Actually, with the unminimized testcase, it seems that current trunk builds are behaving similar as IE.
Attached file testcase2 —
I'm not really sure this touches the issue of what this bug is about, but it shows a different in behavior on trunk and branch. All 4 tables are the same, except: - Table 2 has no height set on the yellow table cell. - Table 3 has padding: 0.01px set on the blue table cell. - Table 4 has a non breaking space inside the blue table cell. Trunk seems to behave a bit more like what IE is doing.
Not that we should ever really use IE as an example of how to handle tables as their table behavior is essentially a blueprint for "how not to behave." At least from our standpoint as developers. J
(In reply to comment #8) > (Not blocking on unconfirmed bugs - please attach a testcase and renom!) > Renom'd for blocking. This sounds like an important bug to the reporter's business site. testcase has been added.
Flags: blocking1.9- → blocking1.9?
Comment on attachment 320698 [details] testcase1 Ah, I just found out that this testcase is what bug 377040 is about (which is a WONTFIX), so no need to discuss this in this bug.
Attachment #320698 - Attachment is obsolete: true
Is the issue you're complaining about the left edge of the popup in the screenshots, or something in the table behind the popup? If the latter, is it the result of breaking the line inside the right column inside the popup?
David, It's the left edge of the popup. J
DBaron/Roc what is you assessment of whether this should force an RC2 or can it wait for a 3.0.1?
Mike, I am just mailing Martjin and Roc some code and an explanation of how our guys are drawing this. It may shed some light. J
Here's the unminimized testcase again, with some of the extra text removed so that the linebreak issue from testcase1 doesn't come into play.
Attachment #320243 - Attachment is obsolete: true
Attached file my minimized testcase —
This is basically the same as Martijn's testcase #2. We're assigning extra space to the row with the height="10" cell, which causes images to be separated instead of sticking together.
My gut feeling is that we shouldn't take this for RC2. My understanding is that height distribution in this situation is underspecified and fragile.
Would the fix for this be regression-prone and touch a highly exposed area?
Do we know when this regressed?
This regressed between 2006-12-07 and 2006-12-08, so a regression from bug 300030 (reflow branch landing).
Blocks: reflow-refactor
(In reply to comment #29) > This regressed between 2006-12-07 and 2006-12-08, so a regression from bug > 300030 (reflow branch landing). > Given this and comment 27 I don't think we want to touch this code RC2 or no.
Minusing per comment 30
Flags: blocking1.9? → blocking1.9-
we hit ancient code http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/tables/nsTableRowGroupFrame.cpp&rev=3.409&mark=728,735#709 the first row has a height of zero so all the excess space will go into the second row which has the 10px height. There are little reftests that test the row height distribution. So changes here are indeed regression prone.
What's the word on this for RC2 or RC3?
(In reply to comment #33) > What's the word on this for RC2 or RC3? > This will not be changed for FF3.
Mike, Would you like to propose some code revisions to make this work on our side then? It would seem we have written compliant code and now what wee have working in FF2 is going to break when FF3 goes public. J
Flags: blocking1.9.0.1?
Keywords: regression, testcase
bernd, dbaron and dholbert need to figure out what, if anything, should be done here.
Well it would be really nice to know how were supposed to fix this on our side since you plan to release this to the world on Tuesday and our compliant code is not going to render correctly for god knows how many customers. Since we have exactly 2 working days to re-write the code on our side it would be great to know what you guys thing needs to be done to make this work.
Summary: RC1pre is not tiling/scaling background images in table cells properly. → distribution of height from rowspan cells should prefer rows without fixed heights
(In reply to comment #37) > Well it would be really nice to know how were supposed to fix this on our side > since you plan to release this to the world on Tuesday and our compliant code > is not going to render correctly for god knows how many customers. Based on the minimized testcases, it looks like you have something like: -------------------------- | cell | cell with no | | with |explicit height| | height |---------------- | 200 | cell with | | | height 26 | -------------------------- and the bug is that when we distribute the 200 among the rows its spans, we don't distribute all of it to the cell with no explicit height. The simple workaround should be (assuming the table in question has no cellpadding or border) to give the cell with no explicit height a height of 174. (I'm not sure if the numbers in the testcases are the exact ones from your site, but I think that should give you an idea.)
That does not work because the size of the cell with height is variable depending on the result of the database query which populates the cell's text data. This can be 3 lines or 300 lines. Because the data is ever changing, each time the table is drawn (as a popup as you may recall) we would need to recalculate the size of the remaining cell. Depending on the system fonts and other user controlled scaling factors this could result in cell sizes that are all over the map. So in this case your solution does not work.
Then replace the two cells on the right with a single cell containing a nested table, give the nested table height=100%, and give its first row height=100%.
because of the nature of the graphic elements it can't be done that way. The cells contain semi transparent graphics for the corners of the UI and in the case of the text and variable height cells there are background images. I understand this bug is inconvenient, but we have some pretty smart guys here that have looked at trying to fix the code and retain the same design and functionality and they have not found a way to do so. If we have to re-write all of this we may as well just rewrite it all IE compliant and tell users we no longer support FF.
---------------------------------------------------------------------------------- | cell with fixed size | cell with fixed size | cell with fixed size | ---------------------------------------------------------------------------------- | cell with fixed size | Large text field | background img | -----------------------| background img | height cell | | variable height cell | variable height | variable height| | fixed width | fixed width | fixed width | | background img | | | ---------------------------------------------------------------------------------- | cell with fixed size | cell with fixed size | cell with fixed size | ---------------------------------------------------------------------------------- Here is a diagram of our situation. There are images which are the background for each cell, some are simple img tags where there is no other content in the cell, other cells which contain text or are variable height and we require scaling of the large image to fit within the scaled cell we use background images. Clearly we can combine the top row of 3 cells and also combine the bottom row with 3 cells in this example, however this table is just one case where we draw the popup hooked to the up and right of the user's cursor. There are other cases where we hook down and right or down and left depending on the data and the arrangement of the page. We reuse the corner graphic elements in each case. We're still open to suggestions so long as it allows us to retain our design as changing the design involves management and toher issues we really prefer not to get into.
Sorry.. here's proper formatting to ake life easier... ------------------------------------------------------------------------- | cell with fixed size | cell with fixed size | cell with fixed size | ------------------------------------------------------------------------- | cell with fixed size | Large text field | background img | -----------------------| background img | height cell | | variable height cell | variable height | variable height| | fixed width | fixed width | fixed width | | background img | | | ------------------------------------------------------------------------- | cell with fixed size | cell with fixed size | cell with fixed size | -------------------------------------------------------------------------
The cells I was suggesting combining would be the middle two cells in the left column in the diagram in comment 43. I don't see why it wouldn't be possible to put those two cells inside a nested table (height=100% width=100%) inside a single cell, so that you have this: ------------------------------------------------------------------------- | cell with fixed size | cell with fixed size | cell with fixed size | ------------------------------------------------------------------------- |----------------------| | | ||cell with fixed size|| Large text field | background img | |----------------------| background img | height cell | ||variable height cell|| variable height | variable height| ||fixed width || fixed width | fixed width | ||background img || | | |----------------------| | | ------------------------------------------------------------------------- | cell with fixed size | cell with fixed size | cell with fixed size | ------------------------------------------------------------------------- (except that the nested table is actually the same size as the cell containing it) At this point, the bug is so full of discussions about workarounds that the information needed to actually fix it is going to get lost, which will make it much less likely to be fixed.
Dave, that may work. We'll test that tomorrow. Thanks, J
Flags: wanted1.9.0.x?
Flags: blocking1.9.0.1?
Flags: blocking1.9.0.1-
Flags: wanted1.9.1? → wanted1.9.1+
Regression-prone changes are no fun in maintenance releases. If there's a safe fix, please re-nom for wanted1.9.0.x.
Flags: wanted1.9.0.x? → wanted1.9.0.x-
re comment 38 http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableRowGroupFrame.cpp&rev=3.411&mark=721-748#721 shows that we look only at style heights on the rows and not on a fixed height defined by the cells The fix would be as rowFrame->GetFixedHeight() will return what we need. However I am pretty reluctant to make a change to this code without a decent table height spec. OK, we have done for 10 years without it, but as soon as pct heights are involved I can not predict what should happen. I started http://developer.mozilla.org/User:Bernd_mozilla/Table_Heights to get my thoughts and questions organized.
s/fix would be as/ fix would be easy as/
QA Whiteboard: qa-not-actionable

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Severity: major → --

I ran across this when retriaging old bugs that were lacking a severity field.

The first visible attachment here, martijn's "testcase2", renders differently in all browsers (and nobody looks anything like IE, which I included since per comment 13 it sounds like that was originally relevant.

More importantly, the attached "unminimized testcase" looks identical between Firefox and Chrome on my machine -- both of us look like attachment 320754 [details], with two "bites" taken out of the left side.
So it seems like we're interoperable there, even though this particular site seemed to depend on a different behavior.

Also, roc's "my minimized testcase" is now interoperable among Firefox, Chrome, and Epiphany (WebKit). So we're interoperable on that too.

So I don't think there's anything much to track here at this point. There's a little bit of interesting non-interoperability in testcase2 as shown in this screenshot, but it seems that's distinct from the actual behavior that the site was depending on here, and it's likely just one of many already-known table interop differences.

Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INVALID

(closing as "INVALID" since our trunk behavior from comment 22 seems to have ended up being the interoperable behavior that Chromium at least converged on, and WebKit approximately matches as well.)

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

Attachment

General

Creator:
Created:
Updated:
Size: