Closed Bug 350444 Opened 15 years ago Closed 15 years ago
[FIX]Vertical borders in table containing col elements are not shown
946 bytes, text/html
5.40 KB, image/gif
929 bytes, application/xhtml+xml
11.07 KB, patch
|Details | Diff | Splinter Review|
6.54 KB, patch
|Details | Diff | Splinter Review|
6.54 KB, patch
|Details | Diff | Splinter Review|
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:220.127.116.11) Gecko/20060728 Firefox/18.104.22.168 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:22.214.171.124) Gecko/20060728 Firefox/126.96.36.199 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
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 188.8.131.52 and 184.108.40.206 to minimize the regression range. That would make it easier to discover.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: unspecified → Trunk
Works in Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:220.127.116.11) Gecko/20060426 Firefox/18.104.22.168 Does not work in any later release. So it seems to be broken between 22.214.171.124 and 126.96.36.199
Than one of these checkins should be responsible for that issue: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_0_BRANCH&branchtype=match&dir=&file=.*&filetype=regexp&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-04-30&maxdate=2006-06-02&cvsroot=%2Fcvsroot
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 found this trunk regression range, but both ranges have nothing in common: http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2006-02-10+21%3A00&maxdate=2006-02-11+06%3A00
I can reproduce this on a 2006-08-27 188.8.131.52 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
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. :(
>Bernd, do you have time to look at this? If you ask how can I refuse, but It will not be for 184.108.40.206
Yeah, this has been broken on the 1.8.0 branch long enough that fixing in 220.127.116.11 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.
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 18.104.22.168 differs from 22.214.171.124, 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....
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 126.96.36.199 build Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:188.8.131.52) Gecko/20060426 Firefox/184.108.40.206 The next release (220.127.116.11) 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 (18.104.22.168) 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? 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?
> "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
see comment 18, I will not do this for 22.214.171.124
Not a security exploit and the engineers are nervous about fixing this on a stability branch: removing from the blocker list.
Flags: blocking126.96.36.199? → blocking188.8.131.52-
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...
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
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.
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.
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: 15 years ago
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...
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
Vertical border are shown in each latest nightly build. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:184.108.40.206pre) Gecko/20061115 Firefox/220.127.116.11pre 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.
(In reply to comment #55) > While attachment 235720 [details]  is working fine attachment 244643 [details]  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.