Closed Bug 350444 Opened 18 years ago Closed 18 years ago

[FIX]Vertical borders in table containing col elements are not shown

Categories

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

Product:
Core
Component:
Layout: Tables
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.9alpha1

People

(Reporter: daniel, Assigned: bzbarsky)

References

Details

(4 keywords)

Attachments

(6 files)

Simple Testcase
946 bytes, text/html
Details
Screenshot made with Firefox 1.5.0
5.40 KB, image/gif
Details
Same testcase as XHTML showing long-existing problem with rows
929 bytes, application/xhtml+xml
Details
Branch version of fix for both rows and cols
11.07 KB, patch
Details | Diff | Splinter Review
Branch version only fixing cols (so just the regression)
6.54 KB, patch
dveditz
: approval1.8.0.9+
dveditz
: approval1.8.1.1+
Details | Diff | Splinter Review
Trunk version fixing just cols
6.54 KB, patch
bernd_mozilla
: review+
roc
: superreview+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6

I have a simple table. When defining col elements for each column, vertical borders within the table are not shown anymore.

This must have been changed somewhere between Version 1.5 and 1.5.5, as it works in 1.5 but does not with the latest versions including 3.0a

Reproducible: Always
Attached file Simple Testcase — 
Note:
The bug seems to depend on the "rules" attribute. If not set, everything's fine. When removing the col elements, the borders are shown.
The Screenshot shows the rendering result of the testcase using Firefox 1.5.0. The vertical border in the first table is shown. (Which is, what I expect and Opera and IE shows too.)
Yes, using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b2) Gecko/20060827 BonEcho/2.0b2 also doesn't show any vertical borders inside the table.

Please try different builds between 1.5.0.0 and 1.5.0.5 to minimize the regression range. That would make it easier to discover.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Version: unspecified → Trunk
Works in
Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3

Does not work in any later release. So it seems to be broken between 1.5.0.3 and 1.5.0.4
Product: Firefox → Core
Component: General → Layout: Tables
I'm not seeing this with a current 1.8 branch SeaMonkey on the testcase, but I'm seeing this sometimes on bigger tables - not completely reproducable though, as sometimes it works in one browser window and doesn't in a different one. I have seen this for a long time though, AFAIK even before 1.8 branch was cut.
Component: Layout: Tables → General
Product: Core → Firefox
Product: Firefox → Core
QA Contact: general → general
I can reproduce this on a 2006-08-27 1.8.0.1 branch build.
On the 1.8.1 branch, this regressed between 2006-02-26 and 2006-04-05, I don't have any builds inbetween. Could someone look for the precise regression range on 1.8.1 branch? Thanks.
Component: General → Layout: Tables
Flags: blocking1.8.1?
Flags: blocking1.8.0.8?
Keywords: testcase
QA Contact: general → layout.tables
I bet on trunk this regressed from bug 326105 and on branch it regressed from bug 322348.

Certainly builds from before bug 326105 have a computed 1px left/right border on the <col> in that testcase, while builds since have a 0px border.

The reason bug 322348 affected this is that for that bug we're ending up with the nsTableFrame as our style context parent instead of ending up with the colgroup, and the reason for that is that the style context it has doesn't have the nsCSSAnonBoxes::tableCol pseudo.  Which makes sense, because it's not a pseudo column.  So the net effect is the same as that of bug 326105 in terms of how the style context gets parented.

But it seems that the mess that is rules painting somehow depends on inheriting from the colgroup or something?

Bernd, do you have time to look at this?

And could we _please_ add this to our regression tests, especially the pre-release ones?  :(  We really shouldn't be breaking stuff like this in security updates.  :(
Flags: blocking1.9?
>Bernd, do you have time to look at this?

If you ask how can I refuse, but It will not be for 1.8.0.7
Assignee: nobody → bernd_mozilla
Yeah, this has been broken on the 1.8.0 branch long enough that fixing in 1.8.0.8 should be ok.
> And could we _please_ add this to our regression tests, especially the
> pre-release ones?  :(  We really shouldn't be breaking stuff like this in
> security updates.  :(

Agreed - but add what specifically to the regression tests?   


not going to block on this, will likely take a patch once a safe one has been created and well-tested.
Flags: blocking1.8.1? → blocking1.8.1-
> Agreed - but add what specifically to the regression tests?   

The "simple testcase" from this bug.
>But it seems that the mess that is rules painting somehow depends on inheriting
>from the colgroup or something?

I am not sure what it is, but we have that hall of shame nomination for 
http://lxr.mozilla.org/seamonkey/source/layout/style/nsHTMLStyleSheet.cpp#167

This would require to get bug 43178 fixed.
I don't really think we want to do bug 43178 on the branch (though I do think we should do it on trunk)...  I wonder whether we could safely break ReParentStyleContext on branch for just table cols...
> safely break ReParentStyleContext
Thats beyond my capabilities. I don't have a good style system understanding.
Well.  I think it wouldn't be too hard to break it like that.  The question is whether we could do it _safely_.  That is, withour reintroducing the issues that bug 326105 and bug 322348 were trying to solve.
(In reply to comment #15)
> > Agreed - but add what specifically to the regression tests?   
> 
> The "simple testcase" from this bug.
> 

So that would catch this specific case - which is great - but I'm wondering if there was some other better way to catch this class of regressions.
how can we programmatically determine if this test case passes or fails?  Is there something that can be detected by code running in content that will indicate pass/fail status?

What about code running in chrome?

If not, then please generate two pages that should be identical.  If they are not identical, then the test should be marked as failing.

One way to do this would be to put the first table in one file and the third table in another file, and change the text to be identical.
Blocks: test-suites
I really wish we actually had roc's "visual" regression tests.  Then we could actually run the layout regression tests and compare results between point releases.  If 1.8.0.6 differs from 1.8.0.5, we need to investigate, for example.
Getting the test framework in bug 344591 working would also be helpful here, wouldn't it?
(In reply to comment #23)
> Getting the test framework in bug 344591 working would also be helpful here,
> wouldn't it?
> 

that's on my short list of things to do real soon, and the motivation for my "generate two pages" request.  wanna help?

But if this particular bug can be determined from dom-accessible data structures, so much the better.
> But if this particular bug can be determined from dom-accessible data
> structures,

Yeah, I think you can detect this with computed style....
With the given HTML the first and third table should be identical in width. While this is true on 1.5.0.3 it is not in newer versions. The missing border line causes a decrease of one pixel (if no explicit size is given by css or a html attribute).

So a simple javascript could check that:

onload = function() {
  var tables = document.getElementsByTagName("table");
  alert(tables[0].offsetWidth == tables[2].offsetWidth);
};

If I would have more details on what is necessary for the automated test I could add that function to the testcase.
However this test would be pretty limited to only this bug and doesn't really check for the existence of a border but only for the correct width. So far I couldn't find a way to determine the border using getComputedStyle. Any hint is welcome.
Do getComputedStyle() on the <col> and look at the computed border-left-width and border-right-width?
No, always returns 0px even on the third example where the borders are visible.

See http://www.birgin.de/test/bugzilla/col_bug.html
I really don't know which element holds the border-width value when using the "rules" attribute.
Checking the border-width of all tr and td elements returns 0px for each table with a "rules" attribute set too.
> No, always returns 0px even on the third example where the borders are visible.

That example doesn't have any <col> elements.

I suggest trying your testcase in an old build that doesn't show this bug and seeing what the computed style for the <col>s in that first table is.  For example, with a 2006-01-01 trunk build I see 1px computed styles in the first table in your testcase, as well as in the second one.
(In reply to comment #30)
> That example doesn't have any <col> elements.

Yes, but td elements. And the td element's border width is 0px as well.


> I suggest trying your testcase in an old build that doesn't show this bug and
> seeing what the computed style for the <col>s in that first table is.  For
> example, with a 2006-01-01 trunk build I see 1px computed styles in the first
> table in your testcase, as well as in the second one.

Yes. The same for the official 1.5.0.3 build 
Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3

The next release (1.5.0.4) shows 0px for each value.
> Yes, but td elements. And the td element's border width is 0px as well.

Yes.  The borders are painted by the anonymous columns, not the table cells...  Thats why we get this bug.

> The next release (1.5.0.4) shows 0px for each value.

Yep.   That's this bug.
Does that mean the test should fail if the return value is 0px and should succeed if it is 1px?
If so what is needed to create a usefull test file? Is it possible to use a HTML document and JavaScript? Just a function with a boolean return value?
> Does that mean the test should fail if the return value is 0px and should
> succeed if it is 1px?

With that testcase and for now, yes.  If we ever fix bug 43178, that will change.

> If so what is needed to create a usefull test file?

davel?
(In reply to comment #34)
> > If so what is needed to create a usefull test file?
> 
> davel?

"davel" does not scale as a creator of useful test files :-)

Write a test case using jsunit (www.jsunit.net), and attach it to this bug.  Here's an shell for such a test case:

<!DOCTYPE html>
<title>Testcase bug 350444</title>
<script language="javascript" src="app/jsUnitCore.js"></script>
<script>

function testBug350444() {
  assertEquals(1,document.getElementById('td id').blah.blah.border-width)
</script>

<table>
...
</table>

You probably want to clean up the above shell so that it is a legal html doc, though :-)
> "davel" does not scale as a creator of useful test files :-)

I was just saying that "davel" might be able to answer the question, as he did.  ;)
Restoring lost blocking flag
Flags: blocking1.8.0.9?
see comment 18, I will not do this for 1.8.0.9
Not a security exploit and the engineers are nervous about fixing this on a stability branch: removing from the blocker list.
Flags: blocking1.8.0.9? → blocking1.8.0.9-
Flags: in-testsuite?
The basic problem is that ProcessTableRulesAttribute ASS-umes things about style context parentage based on whether something is "a group" or not...
The only difference between branch and trunk patches is s/IsContentOfType/IsNodeOfType/, so I figure might as well get this some trunk bake time...
Attachment #244647 - Flags: superreview?(roc)
Attachment #244647 - Flags: review?(bernd_mozilla)
Renominating for branches.  This is a nasty-ish regression from a crash fix we took on those branches, and the patch I just attached is quite safe...
Assignee: bernd_mozilla → bzbarsky
Blocks: 322348
Flags: blocking1.8.1?
Flags: blocking1.8.1-
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9-
OS: Windows XP → All
Priority: -- → P2
Hardware: PC → All
Summary: Vertical borders in table containing col elements are not shown → [FIX]Vertical borders in table containing col elements are not shown
Target Milestone: --- → mozilla1.9alpha
Comment on attachment 244647 [details] [diff] [review]
Trunk version fixing just cols

Man and I hoped soooo that the only time I will see this callbacks again will be when they are removed.
Attachment #244647 - Flags: review?(bernd_mozilla) → review+
Yeah, I hear you.  Let's just remove them on trunk, dammit!
this should be pretty safe for branch, the removal is big surgery but fantasai is on it.
Flags: blocking1.8.1? → blocking1.8.1.1?
Which patch do you want for branches? none of the "branch" versions appear to have reviews. Please ask for approval on the patch(es) you want to land.
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9+
I want the branch version of the trunk patch that has review requests.  As comment 43 says, the two are identical except for a trivial name change on trunk.  Once the trunk patch has been reviewed, I will request branch approval.
Attachment #244647 - Flags: superreview?(roc) → superreview+
Fixed on trunk.
Status: NEW → RESOLVED
Closed: 18 years ago
Flags: blocking1.9?
Resolution: --- → FIXED
Comment on attachment 244646 [details] [diff] [review]
Branch version only fixing cols (so just the regression)

This is a pretty safe fix for a rendering regression one of our security fixes introduced...
Attachment #244646 - Flags: approval1.8.1.1?
Attachment #244646 - Flags: approval1.8.0.9?
Good work! Thanks for the quick response and all the efforts.
Comment on attachment 244646 [details] [diff] [review]
Branch version only fixing cols (so just the regression)

approved for 1.8/1.8.0 branches, a=dveditz for drivers
Attachment #244646 - Flags: approval1.8.1.1?
Attachment #244646 - Flags: approval1.8.1.1+
Attachment #244646 - Flags: approval1.8.0.9?
Attachment #244646 - Flags: approval1.8.0.9+
Fixed for 1.8.0.9 and 1.8.1.1.
Keywords: fixed1.8.0.9, fixed1.8.1.1
Vertical border are shown in each latest nightly build.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.9pre) Gecko/20061115 Firefox/1.5.0.9pre
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061115 BonEcho/2.0
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061115 Minefield/3.0a1

But one bug sliped through. We only talked about vertical borders. While checking if the fix is working I noticed that also horizontal borders aren't visible. Dunno why I havn't seen that before.

While attachment 235720 [details] is working fine attachment 244643 [details] shows the issue. I'll file a new bug for this later.
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.0.9 → verified1.8.0.9
(In reply to comment #55)
> While attachment 235720 [details] [edit] is working fine attachment 244643 [details] [edit] shows the issue. I'll
> file a new bug for this later.

This issue occurs only for XHTML documents. If you save attachment 244643 [details] and rename it to test.html all is working fine. Anyone know if there is already a bug which covers this issue?
> But one bug sliped through.

It didn't slip through.  It's just never worked, unlike vertical borders.  If you notice, the first patch I posted in this bug fixed both, but I decided to fix the regression and only the regression on branch, especially since the rows problem only appears in XHTML.  On trunk, all this code is going away by 1.9 anyway, with any luck.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: