Closed Bug 4510 Opened 26 years ago Closed 21 years ago

table, row, column, *-group backgrounds not working correctly (<col>, <colgroup>, table, style, background)

Categories

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

Product:
Core
Component:
Layout: Tables
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: dbaron, Assigned: fantasai.bugs)

References

(
URL
)

Details

(Keywords: css2, testcase, Whiteboard: [awd:tbl])

Attachments

(4 files, 17 obsolete files)

temporary fix
(deleted), patch
karnaze
: review+
attinasi
: superreview+
Details | Diff | Splinter Review
screenshot
(deleted), image/png
Details
screenshot of painting inside the cellspacing
(deleted), image/png
Details
final patch (?)
(deleted), patch
dbaron
: superreview+
Details | Diff | Splinter Review
You have a number of serious problems with backgrounds on table elements (CSS2, section 17.5.1: http://www.w3.org/TR/REC-CSS2/tables.html#table-layers ). My three tests: http://www.fas.harvard.edu/~dbaron/csstest/sec170501 http://www.fas.harvard.edu/~dbaron/csstest/sec170501a http://www.fas.harvard.edu/~dbaron/csstest/sec170501b show the following problems: 1) Backgrounds on table-column and table-column-group elements don't work. 2) Transparency on table-cell elements causes the table-row and table-row-group backgrounds to be ignored. This is because of you current inherit hackery in ua.css. I assume if the tables were created in XML, the table-row and table-row-group backgrounds wouldn't work either. 3) Backgrounds are not ignored (as they should be) on empty table-cell elements. If you want table backgrounds to work correctly, you need to paint them in the 6 layers described in the spec (table, table-column-group, table-column, table-row-group (or header or footer), table-row, table-cell) and not use the inherit stuff in ua.css.
Status: NEW → ASSIGNED
Target Milestone: M5
chrisd, is this Linux specific?
OS: Linux → All
Hardware: PC → All
It's not Linux specific. It's the same on Linux and Windows. I just reported it on Linux. Changing OS and Platform to All.
Target Milestone: M5 → M6
Moving to M6.
Moving to M8
ua.css, this bug, and and many other table style bugs will be easily fixed after Peter adds a new style rule (in the code) which gets applied after the html style sheet but before the css style sheets.
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
Fixed with latest checkin. Only works correctly in Standard Mode.
Status: RESOLVED → VERIFIED
Using 6/14 Apprunner, colors display an indicated on test cases. Verifying bug fixed.
Background attribute of <tr> does not inherit to <td>. (Build 2000062020 Windows 2000) <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"> <HTML lang=ja> <HEAD> <STYLE TYPE="text/css"><!-- tr {background: #80ffff; color: #000000} td {background: #000000; color: #ffffff} --></STYLE> </HEAD> <BODY background=#ffffff> <table> <tr><td>a</td><td></td><td>c</td></tr> <tr><td>a</td><td>b</td><td>c</td></tr> </table> </BODY> </HTML> For HTML above, background color becomes #fffff of body. This should be color of <tr> #80ffff. >These "empty" cells are transparent, letting lower layers shine through. (CSS2 17.5.1)
I'm reopening this bug, since I think this should not be only for standard mode. Are there any pages that depend on table backgrounds not working? We don't want quirks mode to be blatant emulation of Nav4 -- page authors hate Nav4. We only want to emulate the quirks that pages depend on. Also clearing M7 TM and removing peterl from cc: list, but adding Ian.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Target Milestone: M7 → ---
I think this should be fixed for beta3. Assuming we decide to do it, it should be very easy to do.
Keywords: nsbeta3
Status: REOPENED → ASSIGNED
Keywords: testcase
One bug report == one issue is one of the golden rules of Bugzilla because it enables independent tracking and prioritization of each issue. David, could you please split this into separate bug reports, one per issue, because not all of these issues are of equal priority. For example, we don't support styles at all on columns or column groups (FUTUREd), so supporting backgrounds on them should be FUTUREd as well. But you can make your case that the other bugs should be of higher priority. Please justify to PDT user/developer impact, any blocking effect on adoption within content, etc.
virtual attinasi
Whiteboard: [nsbeta3-]
Target Milestone: --- → Future
*** Bug 48013 has been marked as a duplicate of this bug. ***
see also bug 47563
See comment on bug 55264 for yet another reason why this should be fixed in quirks mode too. Regarding ekrock's comment above -- it *is* one issue. It's just that when the fix was checked in, it was for standard mode only (for no apparent reason).
Keywords: css2
Nominating for mozilla 0.9 to get on people's radar. This is a trivial fix, and I don't see any reason not to make it. We're better off avoiding quirks/standard mode differences that we don't need.
Keywords: mozilla0.9
NavQuirks behavior should be kept for <tr bgcolor="x">, though, since all versions of MSIE also render it that way. (And Opera, too.) Do you want CSS2 behavior for /CSS/-applied backgrounds on <tr>?
QA contact update
QA Contact: chrisd → amar
Keywords: nsbeta3
Keywords: mozilla0.9 → mozilla0.9.2, mozilla1.0
Whiteboard: [nsbeta3-]
Target Milestone: Future → ---
nomination of mozilla0.9 conflicts w/ target milestone of future. clobbering both and nsbeta3-. Can we please try to get closure on this bug?
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=20181 [bug 46268] should fix this for everything except table rows (a necessary quirk).
what needs to be done on this one?! Hope to see this one fixed soon.
copy from 46268: Just a short quote from the CSS2 spec: 17.6.1 The separated borders model In this model, each cell has an individual border. The 'border-spacing' property specifies the distance between the borders of adjacent cells. *This space is filled with the background of the table element*. Rows, columns, row groups, and column groups cannot have borders (i.e., user agents must ignore the border properties for those elements). I think what IE is doing is just right, they inherit the color into the table cell. I think we should do the same. This would require from fantasai's patch only the nsTableFrame.cpp part in order to paint the cellspacing with the table backgrounds color. I have to admit that the CSS2 spec is not conflict free in this matter and as one can see by the David's posting http://lists.w3.org/Archives/Public/www-style/1999Jul/0083.html and the answers, the W3C is not able create a clear specification over two years now. So it is hard to believe that this will change very soon. The honest interpretation of the conflict is that for the collapsed border model as default the space between cell's should be covered by table- , row- , col- etc background, while for the separated border model it deviates from the default model and only the table background is used for the space between the cells.
The WG is looking at this. I hope to have an answer for you on Monday.
Whiteboard: (WG bug 85)
*** Bug 87232 has been marked as a duplicate of this bug. ***
*** Bug 90247 has been marked as a duplicate of this bug. ***
We partly removed the table background quirks and now we have the regression bugs, before going any further in this direction we should carefully analyze our painting bugs because the majority of pages use still the quirk mode and switching to the less tested strict mode can create a bunch of regressions.
Depends on: 91034
Depends on: 68998
Whiteboard: (WG bug 85) → [awd:tbl](WG bug 85)
Target Milestone: --- → mozilla1.0
Blocks: 55623
*** Bug 84986 has been marked as a duplicate of this bug. ***
CSS2 Errata [http://www.w3.org/Style/css2-updates/REC-CSS2-19980512-errata.html] | | [2001-08-27] At the end of the section add the following paragraph: | | Note that if the table has 'border-collapse: separate', the background of | the area given by the 'border-spacing' property is always the background | of the table element. See 17.6.1"
*** Bug 113067 has been marked as a duplicate of this bug. ***
*** Bug 119442 has been marked as a duplicate of this bug. ***
*** Bug 119442 has been marked as a duplicate of this bug. ***
Keywords: mozilla0.9.9
We could easily make the strict mode finally compliant to the spec (errata), aka not drawing the cellspacing background, by switching it to the quirks mode rendering. Oh man I love that, sorry David and Hixie if you cant share my joy.
I second a switch to Quirks rendering for Standard mode. It's almost identical to IE and Opera. It won't break pages. Right now, our rendering is incorrect per CSS2, and I think it's pointless to keep it that way. As for making our rendering standards-compliant, the CSS2 spec is too ambiguous about how one would handle, for example TR {background: url(starburst.gif) no-repeat;} IMO, Quirks is good enough for 1.0, it's easy to change, and it's better than what we have now. At the very least, we shouldn't introduce yet another incorrect rendering for page authors to handle.
Attached patch temporary fix (deleted) — — Splinter Review
Comment on attachment 69072 [details] [diff] [review] temporary fix r=karnaze. fantasia, please run enough visual tests to convince yourself the patch is ok.
Attachment #69072 - Flags: review+
I looked at the testcase in bug 46268, all three of dbaron's table background tests, and a testcase with image backgrounds. We render incorrectly per spec, of course, but aside from the column backgrounds, we're consistent with IE5.
Comment on attachment 69072 [details] [diff] [review] temporary fix sr=attinasi (egg-shields up)
Attachment #69072 - Flags: superreview+
Marking nsbeta1+
Keywords: nsbeta1+
Blocks: 116212
fix checked in
->fantasai
Assignee: karnaze → fantasai
Status: ASSIGNED → NEW
fixed
Status: NEW → RESOLVED
Closed: 25 years ago → 23 years ago
Resolution: --- → FIXED
This bug isn't actually fixed yet. Reopening and moving to future.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla1.0 → Future
Status: REOPENED → ASSIGNED
*** Bug 116212 has been marked as a duplicate of this bug. ***
*** Bug 134261 has been marked as a duplicate of this bug. ***
*** Bug 137707 has been marked as a duplicate of this bug. ***
*** Bug 137779 has been marked as a duplicate of this bug. ***
Even if you don't solve this bug for every case, could you at least be as standards compliant as IE5 and paint background colors for colgroup and col like IE5 does? I'm trying to classify data using foreground colors for rows and background colors for columns. BTW: There's a lot of discussion here about CSS2 being ambiguous. But I think this bug would be a bug for HTML4/CSS1 as well as HTML4/CSS2. I think that would make Netscape 4.79's behaviour a bug and why carry that into Mozilla 1? I don't think CSS2 is very ambiguous regarding table backgrounds.
> Even if you don't solve this bug for every case, could you at least be as > standards compliant as IE5 and paint background colors for colgroup and col > like IE5 does? Then we would be condoning and spreading IE5's incorrect behavior. IMO, IE5 is not any more standards compliant than we are here. Even if you interpreted according to the scant information in CSS1, background is applied to the element it is specified on, not on a collection of associated elements as IE5 does. > I think this bug would be a bug for HTML4/CSS1 as well as HTML4/CSS2. CSS1 does not bother to specify anything about table rendering; intra-table elements' behavior is effectively outside the scope of that specification.
It's not a CSS problem, it's an HTML4 problem. First, I am not implying that IE is a better browser overall. I appreciate the efforts of mozilla.org to supply a W3C standards-based browser and I'm just trying to help the process. >CSS1 does not bother to specify anything about table rendering; No, but HTML4 DOES specify how <col/> and <colgroup> should behave. Strictly speaking, Navigator4 and Mozilla1RC1 do not conform to HTML4 (and therefore XHTML 1.0 I think) in regards to <col/> or <colgroup>. Neither one correctly displays the sample table in the HTML4.01 spec, and there isn't a single STYLE attribute in that sample. My opinions: As I see it there are three separate issues which may all be one bug or 3 bugs: 1: Mozilla doesn't attempt to paint the background of <COL> or <COLGROUP> 2: Mozilla doesn't properly apply HTML4 attributes to <COL> or <COLGROUP> 3: Mozilla doesn't inherit HTML4 attributes (including STYLE) from <COL> or <COLGROUP> to <TH> or <TD> Items 2 and 3 are clearly addressed in the W3C HTML4.01 specification. Item 1 is clearly addressed in the W3C CSS2 specification. >IMO, IE5 is not any more standards compliant than we are here. Yes, it is in this case. IE5 attempts to do all 3 things I listed above. It correctly shows the sample table from the HTML 4.01 spec. It may not do them perfectly but IMO half a glass is better than an empty glass. >Even if you interpreted according to the scant information in CSS1, >background is applied to the element it is specified on, >not on a collection of associated elements as IE5 does. OK, so IE5 doesn't paint the <col/> background directly. It allows <td> to inherit the background properties from <col/>. Yes, this is inconsistent with CSS2. And the background property is prohibited from being inherited in HTML4.01. But I suggest that since IE5 doesn't apply the background property directly to <col/> it is reasonable to allow it to be inherited by an element that it can apply to. Therefore I think it is one reasonable implementation of HTML4/CSS1. If you disagree, so be it. But tell me how to apply a background or a foreground style to a <col>? >CSS1 does not bother to specify anything about table rendering; >intra-table elements' behavior is effectively outside the scope >of that specification. Correct. It is established first in HTML 4.01 and refined in CSS2, and Mozilla is out of compliance with both of them in regards to <col/>. IE seems to at least comply with HTML4.01 in this instance. Good Luck!
*** Bug 140084 has been marked as a duplicate of this bug. ***
Ok, I discovered bug 915 which covers issues #2 and #3 for HTML attributs. Sorry about venting that here. Issue #1 remains part of this bug.
http://lists.w3.org/Archives/Public/www-style/2002May/0077.html > Item 1 is clearly addressed in the W3C CSS2 specification. It is not clearly adressed in the W3C CSS2 specification, and that is exactly the reason this bug is Futured--until it /is/ clearly addressed.
Nice, clear description of the problems. The collapsed border table model is much simpler, is the default behavior of many browsers, and is (I guess) more commonly used than the separated borders model. Therefore it seems to me that fixing the column backgrounds would be simple enough and important enough to do for the collapsed borders model regardless of the status of the separated borders model. The background-color property doesn't have any of the the patterning issues that you have illustrated in your description. Therefore it seems that implementing the background-color property for both table models would be simple enough and important enough to do regardless of the status of the other background properties. Regarding spanning cells, I think that if cell R1C2 spans across R2C2 then cell R2C2 does not exist. The row background for R2 should be transparent for where R2C2 would be. The row or column background for R1C2 would fill the entire R1C2 cell. BTW: I don't think I would often use backgrounds on both columns and rows in the same table. I think most people would be satisfied if column backgrounds worked as long as there were no row backgrounds to conflict with. You said: "In describing the effects of the background properties, however, the spec is very ambiguous, making it impossible to write a defensibly correct implementation." No wonder moz 1.0 has taken so long. ;) Just kidding. I would say: "In describing the effects of the background properties, however, the spec is somewhat ambiguous, making it possible to write multiple defensibly correct implementations." In short, something is better than nothing. Again, Thank You!
The separated borders model is the default in all browsers I know of, and I don't know of any "real" pages that use the collapsed borders model. There is still a major ambiguity with 'background-color' -- whether or not to break at the cell spacing.
charles allen wrote: > In short, something is better than nothing. from a conversation I had with Chris Karnaze - : we're more likely to have pages break if we enable those backgrounds than if we disable them (since IE also applies color and we don't) : Imagine a table with a colored column background. In IE, the background and the color that goes with it both get applied. : In Mozilla, currently, neither get applied. : So there's no contrast problems. : If, however, we apply the background and not the color, we might wind up with navy text on a forest green background or something like that. In my mind, we should either be compatible with other browsers or undisputably correct in our deviations. Anything we change now would be neither. > I would say: "In describing the effects Yes, I probably should have written it that way. dbaron wrote: > There is still a major ambiguity with 'background-color' -- whether or not to > break at the cell spacing. I thought the Errata cleared that up?
quote: : we're more likely to have pages break if we enable those backgrounds than if we disable them (since IE also applies color and we don't) : Imagine a table with a colored column background. In IE, the background and the color that goes with it both get applied. : In Mozilla, currently, neither get applied. : So there's no contrast problems. Not always true. I have a table where I'm assigning background colors to columns, and foreground colors to rows. This is clearly legal in CSS2. In IE, both get assigned and I have no problem. In Mozilla, only the foreground colors are assigned. If I did not carefully choose my table background color, I would have contrast problems in Mozilla, but not IE. Why not draw background colors for columns the same way quirks mode draws them for rows? This would be extending quirks mode to columns rather than inventing a new method. AFIK this is also the same way IE draws column background colors so you are still not inventing a new method. This would make you more compatible with other browsers, AND more compatible with CSS2 at the same time! A win/win if you ask me. BTW, you are correct that most browsers use separate border model. I didn't believe you, so I proved it to myself. :) However CSS2 specifies the collapsed borders model should be the default. Therefore I would expect it to be default in a CSS2 spec browser. Again, the collapsed borders model is much simpler and I think would be easier to fix. You should fix it and maybe make it the default. Then I would care less about the broken separated borders model.
> However CSS2 specifies the collapsed borders model should be the default. See http://www.w3.org/Style/css2-updates/REC-CSS2-19980512-errata.html#s-17-6
*** Bug 149008 has been marked as a duplicate of this bug. ***
Blocks: 98694
*** Bug 172824 has been marked as a duplicate of this bug. ***
*** Bug 175075 has been marked as a duplicate of this bug. ***
Test suite: http://fantasai.tripod.com/www-style/2002/table-backgrounds/tests/ It's a work in progress; only part A has been written.
URL: http://fantasai.tripod.com/www-style/...
Attached patch preliminary work (obsolete) (deleted) — — Splinter Review
So you can see where I'm going with this. Mozilla with the patch passes Part A of the test suite. I still have to run it through as yet unwritten tests for exact image position wrt borders, empty cells, etc. Also, the code needs polish. Of course, I'm not submitting for review yet, but if anyone notices problems with the overall algorithm, it would help to know sooner rather than later. Many thanks to bz for answering silly questions to which any amateur programmer would know the answers. He's saved me hours of frustration.
*** Bug 179086 has been marked as a duplicate of this bug. ***
*** Bug 180404 has been marked as a duplicate of this bug. ***
Attached patch patch (obsolete) (deleted) — — Splinter Review
Not up for checkin just yet. Got some conceptual issues with borders. But the code is more-or-less in its final form.
Attachment #105030 - Attachment is obsolete: true
My first impression of the patch is somehow ambivalent. Probably I am only dumb. @@ -140,8 +140,9 @@ const nsRect& aBorderArea, const nsStyleBorder& aBorder, const nsStylePadding& aPadding, - nscoord aDX, - nscoord aDY, + nscoord aDX = 0, + nscoord aDY = 0, + const nsRect* aClipRect = nsnull, PRBool aUsePrintSettings=PR_FALSE); why we need this aDX = 0 , aDY = 0 ? +class ColumnBGData; + +struct ColGroupBGData { .... do we really need these classes, I would love to see them going away. A malloc inside a paint routine seems to me like a potential performance hit. If we really need those can't we compute them during the reflow? If we can compute them during reflow, why dont we change the existing frame structures. ColumnBGData::~ColumnBGData() +{ + ColGroupBGData* lastColGroup = nsnull; + for (PRInt32 i = 0; i < mNumCols; i++) { + if (mCols[i].mColGroup != lastColGroup) { + lastColGroup = mCols[i].mColGroup; + delete mCols[i].mColGroup; + } + mCols[i].mColGroup = nsnull; + } + delete [] mCols; +} isnt + delete [] mCols; enough? if (bgData.Ok() && bgData.ColGroup(colIndex)->Ok()) { + nsCSSRendering::PaintBackgroundWithSC(aPresContext, aRenderingContext, + bgData.ColGroup(colIndex)->mFrame, aDirtyRect, + bgData.ColGroup(colIndex)->mRect, *bgData.ColGroup(colIndex)->mBackground, + zeroBorder, zeroPadding, 0, 0, &cellRect, PR_FALSE); + } + //Paint row group background + if (rgBG) { + nsCSSRendering::PaintBackgroundWithSC(aPresContext, aRenderingContext, + this, aDirtyRect, rgRect, *rgBG, zeroBorder, zeroPadding, 0, 0, + &cellRect, PR_FALSE); + } + //Paint column background + if (bgData.Ok() && bgData.Col(colIndex)->Ok()) { + nsCSSRendering::PaintBackgroundWithSC(aPresContext, aRenderingContext, + bgData.Col(colIndex)->mFrame, aDirtyRect, bgData.Col(colIndex)->mRect, + *bgData.Col(colIndex)->mBackground, zeroBorder, zeroPadding, 0, 0, + &cellRect, PR_FALSE); + } why do we paint first the colgroup then the row group then the col, I thought the spec requires colgroup col rowgroup + nscoord cellSpacingX = table->GetCellSpacingX(); - x = borderPadding.left; + x = borderPadding.left + cellSpacingX; y = borderPadding.top; availSize.width = aAvailWidth; if (NS_UNCONSTRAINEDSIZE != availSize.width) { - availSize.width -= borderPadding.left + borderPadding.right; + availSize.width -= borderPadding.left + borderPadding.right + (2 * cellSpacingX); This is pure horror for me, this must be wrong. The patch alters the nsTableReflowState. Before we reflow all the children. Probably there should be some comments on those critical places why it does not affect reflow.
> My first impression of the patch is somehow ambivalent. Probably I am only > dumb. Probably you're not asking the right questions. What are you ambivalent toward? What bothers you about it? What would you have prefered? > why we need this aDX = 0 , aDY = 0 ? The arguments don't seem to be used anywhere in the function, and IIRC every function call I've run across (I had to run a search modify all calls passing in the aUsePrintSettings arg) passes in zero. So, I think we should eliminate the arguments. I'm defaulting them to zero for now so most calls won't have to pass those zeros in, and I'll file a bug on taking them out later. > do we really need these classes, I would love to see them going away. A malloc > inside a paint routine seems to me like a potential performance hit. I originally put them in to avoid a performance hit. This is also why all the painting is done in nsTableRowGroupFrame::Paint rather than in the frames' respective classes; there is no need to iterate over all the cells in the table once for the column groups, once more for the columns, another time for the row groups, yet another time for the rows, and once again for the cells themselves. Alternatively the cells could paint everything by calling into parents, but I don't think this is any better than what I'm doing right now. (That method would involve more complicated mRect adjustments.) The paint function is iterating over all the cells in the row group, which in most cases means all of the cells in the table. Instead of recalculating all the column style and frame info for every cell, I'm calculating everything once and storing the info in an array. The storage doesn't cost very much in terms of memory because its created and destroyed in one function call, existing for a very short amount of time. However, it's possible that the time cost in allocating and deallocating the memory might outweigh the savings from storing the calculated values. I don't know what the performance cost is for that. If you think that it's better to call the table frame's GetColFrame and get the background, mRect, and the colgroup frame (and it's background and mRect) for each cell, I can change it. > If we really need those can't we compute them during the reflow? We could, but they don't seem important enough to take up space when we're not painting. > isnt + delete [] mCols; enough? No, because each ColBGData holds a pointer to its parent ColGroupBGData. Suppose I have a colgroup with two columns. If I delete the first ColBGData, it will delete its ColGroupBGData. When I delete the second ColBGData, it will call delete on its pointer to that same ColGroupBGData. Except it's already been deleted, and the space might have been allocated to something else. > why do we paint first the colgroup then the row group then the col, I thought > the spec requires colgroup col rowgroup Yes, you're right. I'll fix that, along with the MOZ_COUNT_CTOR and GetStyleData stuff you mentioned on IRC. > This is pure horror for me, this must be wrong. The patch alters the > nsTableReflowState. Before we reflow all the children. nsTableReflowState is already altered before the children are reflowed by subtracting the border... > Probably there should be some comments on those critical places why it does not > affect reflow. Actually, it does affect reflow. But the end result should be the same as before. CellspacingX is subtracted out of the width, but it's also no longer added to x before reflowing the first cell. I will run the layout regression tests, and that should point out any major miscalculation errors. btw, if you could answer the few questions I've commented in, that would be helpful.
*** Bug 190455 has been marked as a duplicate of this bug. ***
*** Bug 190942 has been marked as a duplicate of this bug. ***
Depends on: 195291
*** Bug 196889 has been marked as a duplicate of this bug. ***
Removing mozilla0.9.9+, mozilla1.0, and nsbeta+ keywords as they are all obsolete by many moons.
Keywords: mozilla0.9.9+, mozilla1.0, nsbeta1+
Please never again remove keywords you don't know about!
Keywords: mozilla0.9.9+, nsbeta1+
If the keywords do not have the meaning that the "Keywords" page has, then perhaps someone needs to update that page eh???
Could you possibly explain for the other 30-some people who are watching this bug, what does mozilla0.9.9+ mean if it does not mean, "drivers@mozilla.org would like to see these bugs checked in during the [0.9.9] milestone freeze or branch period"? And, what is the meaning of an nsbeta1+ keyword placed there many versions ago? Thank you.
There was a "temporary fix" for this bug that was checked in a while ago to make us, if not correct, at least compatible with the other incorrect rendering engines out there. I believe the keywords apply to that. This bug was then marked FIXED, but I reopened it because it isn't really fixed.
roc, views are seriously broken on tables. I did the best I could, given that they don't paint properly. Relevant testcases are: http://fantasai.tripod.com/www-style/2002/table-backgrounds/tests/layers-opacity.html http://fantasai.tripod.com/www-style/2002/table-backgrounds/tests/fixed-bg.html That's not a background issue, though; content is also missing.
Target Milestone: Future → mozilla1.5alpha
Attached patch patch (obsolete) (deleted) — — Splinter Review
Attachment #106551 - Attachment is obsolete: true
Attached file nsTableUnderlay.h (obsolete) (deleted) —
Attached file nsTableUnderlay.cpp (obsolete) (deleted) —
ok, Bernd. Fire away! (There are a few things I wasn't sure of; these are marked with XXX.)
Attached file nsTableUnderlay.h (Now with More Comments) (obsolete) (deleted) —
Attachment #125789 - Attachment is obsolete: true
Attached file nsTableUnderlay.cpp (tweaked) (obsolete) (deleted) —
Attachment #125790 - Attachment is obsolete: true
Ok, so. Explanations: > what will the patch do and what not The patch will fix every standards-mode table background issue I know about except fixed backgrounds on columns/colgroups. Those are broken... Actually, now that I think about it, I might be able to make that work, too. I'll give it a try. The backgrounds are painting--just not properly. So, fixed issues include: - positioning and tiling of background images - painting area (no cellspacing) - layering - views unwittingly giving cells wrong paint flags (bug 191567) > which frames will change their size, Rows, row groups, columns, and colgroups will no longer include outer cellspacing in their dimensions. > why did you implemented it the way you have choosen. I implemented it this way because it seemed more efficient to cache calculated data and make one pass through the table than to have each cell calculate its own set of background data for its row/rowgroup/column/colgroup. Implementing it as a separate class makes it a) easier to understand b) easy to deal with views c) easy to reduce background painting to a single pass once the responsibility for painting borders is moved to the cells > what tests has the already patch seen The patch passes all the tests at http://fantasai.tripod.com/www-style/2002/table-backgrounds/tests with one problem: As aforementioned, views don't paint tables properly so some cells are incompletely painted in the fixed background and opacity tests. I also ran them in quirks mode; I haven't broken anything. Although, I could also fix the way <table> backgrounds leak in quirks border-collapse... > risks Risk of pages breaking is pretty low; the tests I ran are quite comprehensive. Risk of crashes/leaks/other-stupid-mistakes is.. pretty high. I'm a very inexperienced programmer; I wouldn't even rank myself as high as amateur. In addition, I build on Linux, but that's all I use it for, so the patched build hasn't seen extensive use. I'm going to give Bugzilla a break and put up the code at http://fantasai.tripod.com/Mozilla/bugs/4510 I'll attach the final versions for archiving when we're done. dbaron -- you might want to check, at some point, to make sure my interpretation is ok. After this goes in would probably be easiest, since then you can grab a build and look at the test cases.
> Actually, now that I think about it, I might be able to make [fixed bg on cols] > work, too. Looks like I can do that without much trouble. However, it means I have to cheat a little; I'd have to ignore the bounds of the dirtyRect and paint the whole column/columngroup. Do you want me to do this or should I add col, colgroup { background-attachment: scroll !important; } to html.css instead?
Target Milestone: mozilla1.5alpha → mozilla1.5beta
Blocks: 203166
After reading the patch a first time ( 3hours) I believe the code will do the job, however I am still concerned about the performance hit, why do we compute for the whole table and not for the damage area, couldnt we do something similar (call) as below http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/nsTableFrame.cpp#6632 (oh, I thought I converted that into a function a long time ago). and even reuse the damageArea afterwards for BC painting. I will run the paint code after my vacation...
> why do we compute for the whole table and not for the damage area, couldnt we... Added coordinate checks to the loop. If you want, I can make the intersect check in PaintCell into a separate inline function and call the rest of the paint code conditionally based on its return value.
The patch doesnt compile. While the GetView deCOM is easy, GetContinuousLeftBCBorderWidth is used in nsTableUnderlay.cpp but not defined in nsTableFrame.h I think the patch needs to be updated to the tip, sorry for beeing so lame that the patch started to bitrot.
> The patch doesnt compile. Not surprising. It's based on the May 28th source. > I think the patch needs to be updated to the tip I'm trying. :) Mozilla's compiling right now. If I'm lucky, I'll get a working Windows binary and I can start applying the patch. If not.. I guess I'll try to imitate the Windows-Linux build environment I had on my old computer. Pulling the source is working now that I've reinstalled cygwin, and given that, I should be able to get it to work. > sorry for being so lame that the patch started to bitrot. It's ok. I just hope I'll be able to finish this before I leave this weekend...
Attached patch Patch (untested) (obsolete) (deleted) — — Splinter Review
Attachment #125788 - Attachment is obsolete: true
Attached file nsTableUnderlay.h (untested) (obsolete) (deleted) —
Attachment #125925 - Attachment is obsolete: true
Attached file nsTableUnderlay.cpp (untested) (obsolete) (deleted) —
Attachment #125928 - Attachment is obsolete: true
It compiles under mingw. More than that, I cannot say; I wasn't able to get a working binary to test with under either Windows or Linux. If you can send me a drop-in replacement of Gecko that I can use with a nightly, I'll test it...
*** Bug 214877 has been marked as a duplicate of this bug. ***
Apologies for duplicating this bug, I hadn't found it from a few searches. However, could someone say what is happening or going to happen? I got a bit lost when people started using the code ;) My basic question is: Will front end web developers be able to style with col? Thanks :) I do try and persuade everyone to use Moz, which is becoming easier when they realise they don't have to get all those ad-programs installed whilst browsing.
> Will front end web developers be able to style with col? Yes, but see http://www.w3.org/TR/REC-CSS2/tables.html#q4 This bug will fix background rendering on columns in Standards layout mode.
> This bug will fix background rendering on columns in Standards layout mode. That's great, I now have a related question about horizontal alignment, that you might refer me to another bug for.. Moz seems to ignore horizontal alignment set by the width attribute of col, and any CSS alignment class set on the col. E.g: http://www.ukwindsurfing.com/results/2003/rankings_rb.asp The CSS section refered to (http://www.w3.org/TR/REC-CSS2/tables.html#q4) doesn't cover aligment - fair enough. But it doesn't give any control available for controling columns without using classes set on each cell. In the example above, the names are left aligned, and points are supposed to be right aligned. The alignment attribute of col is in the XHTML DTD (http://www.w3.org/TR/xhtml1/dtds.html#dtdentry_xhtml1-strict.dtd_col), has this been intentionally left out, or would this be a separate bug? Cheers, I hope I'm being useful, I've been using (& relying on) Moz since version 1, but I'm new to the world of bugzilla. :) -Alastair
Comment 98 is about bug 915, not this bug. It's generally harmful to add unrelated comments to bugs, especially long comments, since they make the real issues in the bug harder to find.
Turning this over to Bernd for review/checkin/etc. The two new files should be MPLed, of course. And please look over comment 85.
Comment on attachment 129063 [details] [diff] [review] Patch (untested) *pokes Bernd* Bernd, will _you_ set the new target milestone? Obviously my guesses are way off.
Attachment #129063 - Flags: superreview?(bz-vacation)
Attachment #129063 - Flags: review?(bernd_mozilla)
Attached image screenshot (deleted) —
the patch fails when a rowspan in the border collapsed table is scrolled into the visible area. furthermore the patch does not compile as - PRInt32 numCols = GetColCount(); the numCols are used later in an assertion and the Makefile.in part of the patch is missing. There are some whitespace errors in the patch http://www.johnkeiser.com/cgi-bin/jst-review-cgi.pl?patch_file=&patch_url=http%3A%2F%2Fbugzilla.mozilla.org%2Fattachment.cgi%3Fid%3D129063%26action%3Dview&patch_text=&reason_type=%21&reason_type=A&reason_type=B&reason_type=E&reason_type=F&reason_type=L&reason_type=N&reason_type=O&reason_type=P&reason_type=Q&reason_type=R&reason_type=S&reason_type=W&reason_type=X&reason_type=%7B
I've not digested all the changes yet, so a lot of these comments are non-substantive. I've also read only part of the patch. So take these comments with a boulder of salt... Please use more context and use the -p option to diff (-p -u8 is a good way to diff....). That makes patches a lot more readable. >Index: mozilla/content/shared/src/nsStyleStruct.cpp >+#ifdef DEBUG >+ if (mBorderStyle[NS_SIDE_TOP] & BORDER_COLOR_SPECIAL) { >+ NS_WARNING("Clearing special border because BORDER_COLOR_DEFINED is not set"); >+ } >+#endif Are there good reasons not to make that an assertion? Are there ever cases where it could happen that are technically correct? Same for the other sides. >Index: mozilla/layout/html/style/src/nsCSSRendering.h >- PRBool aUsePrintSettings=PR_FALSE); >+ PRBool aUsePrintSettings=PR_FALSE, Toss in spaces around the '=' sign, please? >Index: mozilla/layout/html/document/src/quirk.css >+/* make sure backgrounds are inherited in tables -- see bug 4510*/ >+td, th, tr { >+ background: inherit; >+} Do we really want this quirk? What purpose does it serve? In particular, what would "tr" inherit from (that only comes into play on quirks-mode pages that are setting backgrounds on <tbody> elements; I suspect we want to follow IE there, and I doubt that IE has that quirk, but I could be wrong (no way I can test)). >Index: mozilla/layout/html/table/src/nsTableFrame.h >+ /** Get left continuous border width >+ */ >+ nscoord GetContinuousLeftBCBorderWidth(float aPixelsToTwips, >+ PRBool aGetInner) const; >+ What does that mean, exactly? A clearer explanation is in order here. That is, what is a "left continuous border"? What does aGetInner do? >- unsigned : 19; // unused >+ unsigned mLeftContBCBorder:8; >+ unsigned : 11; // unused I think I'd be happier if this struct were using PRUint32, so you knew you actually _did_ have 32 bits to work with, but that can be a separate patch.... >Index: mozilla/layout/html/table/src/nsTableFrame.cpp >- x = borderPadding.left; >+ x = borderPadding.left + cellSpacingX; > y = borderPadding.top; Why the asymmetry here? Comment it, if there is a good reason. >- availSize.width -= borderPadding.left + borderPadding.right; >+ availSize.width -= borderPadding.left + borderPadding.right >+ + (2 * cellSpacingX); Comment why you have to subtract off the cellspacing? Same for the Y direction? Or perhaps comment where availSize is declared and say what size it actually reflects?
> the patch fails when a rowspan in the border collapsed > table is scrolled into the visible area. Made some changes. See if that works. > the numCols are used later in an assertion Removed change. I think the compiler was complaining that the variable wasn't used -- the assertion doesn't appear in optimized builds, I assume. So therefore, the variable and function call aren't, either. Should I put an #ifdef DEBUG on it? > and the Makefile.in part of the patch is missing Ok, fixed. > -p -u8 is a good way to diff It shall be done. *updates diff4510 script to do that* > Are there good reasons not to make that an assertion? Are > there ever cases where it could happen that are technically > correct? I can't think of any. I guess you want me to switch it over? > Toss in spaces around the '=' sign, please? Done. > Do we really want this quirk? What purpose does it serve? I've wondered about that myself. I think it's only useful for pages that use "background: transparent" on cells to make the table background shine through. > In particular, what would &quot;tr&quot; inherit from... table row group, of course. > I doubt that IE has that quirk, but I could be wrong IE does indeed have that quirk. (I think we're to be the only layout engine that actually layers table backgrounds--although I wouldn't be too surprised if Tasman's team did something like that.) > >+ /** Get left continuous border width > What does that mean, exactly? A clearer explanation is in > order here. Yes, you're quite right about that. I need to spec out the interaction of table backgrounds with table borders. > That is, what is a &quot;left continuous border&quot;? What does aGetInner do? Commented in. Implementation moved inline into nsTableFrame.h > I think I'd be happier if this struct were using PRUint32 Changed. > Why the asymmetry here? Comment it, if there is a good reason. What asymmetry? Between x and y? The cellspacing gets added to the y coordinate during reflow; it's a running count and cellspacing gets added before the child height in the loops. Comment added, anyway. > Comment why you have to subtract off the cellspacing? availSize.width reflects the width available for the children. In tables this means subracting out the cellspacing on the sides as well as the border. (Before this change, cellspacing in the X direction was treated as a shift in the table cell position within the row.) (Updated patch is at http://fantasai.tripod.com/Mozilla/bugs/4510 ; No guarantee that it'll compile.)
Blocks: 216167
> No guarantee that it'll compile. isnt that an euphimism for just the opposite. :-) the patch at http://fantasai.tripod.com/Mozilla/bugs/4510/patch.txt does not compile as a) #define GET_TWIPS_TO_PIXELS(presContext,var) \ float var; \ (presContext)->GetScaledPixelsToTwips(&var); \ + var = 1.0f / var; the additional line is not nessary. (while you are on it, please remove the unessary blank lines from nsTableFrame.h) In content/shared the assertion requires an argument what to test for. I would rewrite that section with out the debug defines and loop over the four sides. > for (PRUint8 Side = NS_SIDE_TOP; Side <= NS_SIDE_LEFT; Side++) { > if (!(mBorderStyle[Side] & BORDER_COLOR_DEFINED)) { > NS_ASSERTION(!(mBorderStyle[Side] & BORDER_COLOR_SPECIAL), > "Clearing special border because BORDER_COLOR_DEFINED is not set"); > SetBorderToForeground(Side); > } While this are peanuts, the patch draws into the cellspacing in separate mode when the table is not completely inside the window (see screenshot)
> painting inside the cellspacing I've never seen /that/ before. Is it a regression from the previous patch? > unnecessary blank lines Stupid text editor. I'll fix that, don't worry. :)
this is not a regression it has been like this with attachment 129063 [details] [diff] [review], the trick is the small window
*** Bug 218541 has been marked as a duplicate of this bug. ***
Resummarizing to what I think this bug currently covers. If I'm wrong, please resummarize again.
Summary: table element backgrounds not working correctly → table row, column, *-group backgrounds not shown
*** Bug 219404 has been marked as a duplicate of this bug. ***
We're fixing a bit more than that here. Things covered include: - paint table column/colgroup backgrounds - layer backgrounds properly - set up image positioning & repeating bounds correctly - cleaning up <table> background boundaries in Quirks (might as well) I really ought to finish this off. If I don't post a (hopefully finalized) patch by Monday, someone should send me a nice flame or something for being so annoyingly slow... ~_~
Summary: table row, column, *-group backgrounds not shown → table, row, column, *-group backgrounds not working correctly
Patch updated. And it does compile. However, I don't know if it /works/ since I still get linking errors...
*** Bug 221020 has been marked as a duplicate of this bug. ***
Re comment 104: > I can't think of any. I guess you want me to switch it over? Yes, please. > IE does indeed have that quirk OK. Then it's ok to keep it as a quirk, I guess... If you have an updated patch and are having trouble getting it to link, feel free to mail it to me and I'll look..
I've put the files up at http://fantasai.tripod.com/Mozilla/bugs/4510/ I'll attach them for flagging once Bernd gives his ok, but attaching three files every time I adjust whitespace would probably strain the CC list overmuch...
I cant apply the patch: patch: **** malformed patch at line 790: +nsTableRowGroupFrame::GetContinuousBCB orderWidth(float aPixelsToTwips, could you please create a patch with only diff -u, do you manually edit the patch files ?
I'm patching with diff -p -u8. No, I didn't edit the patch by hand. I just reran cvs diff and uploaded the result. I checked it, too, by running it in reverse and then normally. Is it still causing problems?
QA Contact: amar → ian
Whiteboard: [awd:tbl](WG bug 85) → [awd:tbl]
*** Bug 224283 has been marked as a duplicate of this bug. ***
patch updated; fixed nsStyleStruct assertions. The border-collapse assertions still go off when I build without the patch, so I don't think I'm causing the problem. Also, it's in a function I don't call..
Flags: blocking1.6b+
Flags: blocking1.6b+
I tested the patch at http://fantasai.tripod.com/Mozilla/bugs/4510/ . It looks good to me. And it is so much better what mozilla does currently. Thanks a lot. Attach it to the bug. You should ask bz, roc and dbaron as the module owner to look carefully at this. I think, I looked pretty hostile at the patch but this can't be a substitute for a review.
Attached patch patch (obsolete) (deleted) — — Splinter Review
Attachment #129063 - Attachment is obsolete: true
Attached file nsTableUnderlay.h (obsolete) (deleted) —
Attachment #129064 - Attachment is obsolete: true
Attached file nsTableUnderlay.cpp (obsolete) (deleted) —
Attachment #129065 - Attachment is obsolete: true
Attachment #135353 - Flags: superreview?(bz-vacation)
Attachment #135353 - Flags: review?(dbaron)
Attachment #129063 - Flags: superreview?(bz-vacation)
Attachment #129063 - Flags: review?(bernd_mozilla)
*** Bug 225780 has been marked as a duplicate of this bug. ***
Summary: table, row, column, *-group backgrounds not working correctly → table, row, column, *-group backgrounds not working correctly (<col>, <colgroup>, table, style, background)
So... is this going to get reviewed in time for 1.6beta or should I set the target to January (1.7alpha)?
fantasai, I definitely won't get to it in time for beta (in the next 40 hours, basically). I'm sorry I dropped the ball on this so badly. :( On the other hand, I feel that this is alpha material in any case. And I will make sure that I've done a review well before 1.7a opens so you can land as soon as it does...
> I'm sorry I dropped the ball on this so badly. :( Eh, don't worry too much about it. I doubt dbaron or roc would've found time, either. And I've taken out several months myself, what with not being able to build for so long... > On the other hand, I feel that this is alpha material in any case. Yeah, I'd prefer to have it checked in during an alpha cycle, too. :)
Blocks: 226426
Blocks: 227031
Blocks: 149378
Target Milestone: mozilla1.5beta → ---
Comment on attachment 135354 [details] nsTableUnderlay.h Why "nsTableUnderlay"? Wouldn't "nsTableBackgroundPainter" be a better reflection of what's going on? You need a license comment here. >#define BC_BORDER_TOP_HALF_COORD(p2t,px) NSToCoordRound((px - px / 2) * p2t ) You need parens around "px" in that expression, both places (consider what happens if I pass "2-1" as "px". Same for all the other macros here. > * User is expected to loop through elements to be > * painted <em>in layout order</em> (row groups must > * be ordered properly), using Load/Unload functions Perhaps you should clearly document which functions need to be called in which order to paint a cell or whatever objects this paints? An example would probably not hurt either.... > * TableBackgroundPainter will handle position > * translations Meaning into the child's coordinate space? If so, I think it would be better to say that clearly. > TableBackgroundPainter(nsIPresContext* aPresContext, > nsIRenderingContext& aRenderingContext, Why isn't that a pointer? References to interfaces like that make me queasy. > * @param aDeflate - adjustment for frame's rect That's not helpful. Explain why one would want to pass this and what it does. > * @param aSkipSelf - pass this cell; i.e. paint only underlying layers Why would one use this? I would rather you avoided params with default values; people will have a tendency to assume the default value is the correct one. It's better to make them explicit so they have to think about how they are calling the function. > struct TableBackgroundData { > /** Data is valid */ > PRBool Ok() const { return (PRBool)mBackground; } Maybe call this IsOK() ? > /** Destructor; must call Clear first, however */ Why? If this class always has a frame, it can get the prescontext off the frame. So there is no need to pass a prescontext to it, and hence no need for Clear() that I see. Unless mFrame can be null? > /** Calculate and set data values to represent aFrame */ > void Set(nsIPresContext* aPresContext, > nsIRenderingContext& aRenderingContext, > nsIFrame* aFrame); Does aFrame have to be non-null? Is aPresContext really needed here? Could you use a pointer to the nsIRenderingContext instead? > nsIRenderingContext& mRenderingContext; And here.
Comment on attachment 135354 [details] nsTableUnderlay.h > PRBool Ok() const { return (PRBool)mBackground; } Also, that's not cool in general -- casting a pointer to PRBool does not do what you really want it to. Use "return !!mBackground;" instead, I would say.
> PRBool Ok() const { return (PRBool)mBackground; } is uncool as bz said, because it generated random non-zero results on 32-bit systems, and where pointers may be 64 bits but int is 32, it may even generate 0 (PR_FALSE) for a non-null pointer. The !!p trick looks odd enough to people that I've always gone for an explicit null comparison, e.g.: > PRBool Ok() const { return mBackground != nsnull; } Note no need for (PRBool) casting. /be
Comment on attachment 135355 [details] nsTableUnderlay.cpp Need a license. >TableBackgroundPainter::TableBackgroundData::Clear(nsIPresContext* aPresContext) Like I said, just get the prescontext off mFrame and be done with it. > mBorder = nsnull; > mBackground = nsnull; > mFrame = nsnull; > mBorderIsSynth = PR_FALSE; Do you really need to reset all these? > //XXXfr should aRenderingContext be removed from IVFP's param list? Probably. File a bug on that, cc me and roc and dbaron. We probably want to deCOMtaminate that method at the same time too. >inline PRBool >TableBackgroundPainter::TableBackgroundData::ShouldSetBCBorder() Any reason not to just inline this into the header? >TableBackgroundPainter::TableBackgroundData::SetBCBorder(nsMargin& aBorder, > NS_PRECONDITION(aPainter, "null frame"); Change the text to make sense? > nsStyleBorder* styleBorder = new (aPainter->mPresContext) nsStyleBorder; > if (!styleBorder) return NS_ERROR_OUT_OF_MEMORY; > *styleBorder = aPainter->mZeroBorder; Wouldn't nsStyleBorder* styleBorder = new (aPainter->mPresContext) nsStyleBorder(aPainter->mZeroBorder); if (!styleBorder) return NS_ERROR_OUT_OF_MEMORY; be better? >TableBackgroundPainter::TableBackgroundPainter(nsIPresContext* aPresContext, This could use a constructor counter. >TableBackgroundPainter::Init(nsTableFrame* aTableFrame, > NS_ASSERTION(colGroupList.FirstChild(), "table should have at least one colgroup"); Are you sure? What if I'm in XHTML and I just have an <html:table> with nothing inside? Will this assert trigger? What will mNumCols be? Perhaps it would be better to bail early or skip stuff if mNumCols is 0, then assert if mNumCols > 0 and there is no first child? > for (nsTableColGroupFrame* cgFrame = (nsTableColGroupFrame*)colGroupList.FirstChild(); > cgFrame; cgFrame = (nsTableColGroupFrame*)cgFrame->GetNextSibling()) { It's not immediately clear to me what this code is attempting to do. A comment would help a lot here. I assume that you're storing the colgroup info for each col or something? In any case, comments about you're doing this here (and comments in the header when you declare these data structs explaining what they are used for) would be most helpful. > nsresult rv = cgData->SetBCBorder(border, this); > if (NS_FAILED(rv)) return rv; Doesn't that return leak cgData? > mCols[colIndex].mCol.mRect.MoveBy(cgData->mRect.x, cgData->mRect.y); So mCols[colIndex].mCol.mRect is the rect of the col in the coord system of the table? If so, why? Also, this needs to be documented somewhere (eg the header, either where you declare the ColData type or where you declare mCols or whatever you deem appropriate). In fact, it would be good to document the coord systems for all the various members that end up having an nsRect as part of them. > mCols[colIndex].mColGroup = cgData; So the idea is that cgData is allocated in Init() and deallocated in the destructor of this class? That is, the mCols elements have pointers to it, but do not own it? If so, document that clearly here and perhaps where mCols is declared.... > nsresult rv = mCols[colIndex].mCol.SetBCBorder(border, this); > if (NS_FAILED(rv)) return rv; Document that this does NOT leak cgData, since there is now a pointer to it in mCols[colIndex]. I have to admit that I'm not sure quite what's going on with the BC left border (in particular, why you reset to lastLeftBorder every time in the inner loop. Please document that. >TableBackgroundPainter::PaintTable(nsTableFrame* aTableFrame, More BC magic... why exactly do we only sometimes need to set the bcborder? The ShouldSetBCBorder impl should probably document why it uses the criterion it uses. >TableBackgroundPainter::TranslateColumns(nscoord aX, > nscoord aY) This method needs some documentation in the header. > if (eOrigin_TableRowGroup != mOrigin) { It helps to comment which coord space you are transforming to before doing the coord transformations... you're transforming into the row group coord space here, right? > mRenderingContext.Translate(mRowGroup.mRect.x, mRowGroup.mRect.y); > mDirtyRect.MoveBy(-mRowGroup.mRect.x, -mRowGroup.mRect.y); > if (mCols) { > TranslateColumns(-mRowGroup.mRect.x, -mRowGroup.mRect.y); When you translate back in UnloadRowGroup(), you use mRowGroup.mFrame->GetRect() as the rect.. are you guaranteed that the x,y coords of that are the same as the x,y coords of mRowGroup.mRect at this point? If so, assert that here, please. >TableBackgroundPainter::LoadRow(nsTableRowFrame* aFrame) > //else: No translation necessary since we're using row group's coord system Erk. First I hear of that, and I read the header. That needs to be VERY clearly documented somewhere..... I'm not very knowledgeable about BC code, so I can't tell whether what you're doing here with BC is right... if bernd is OK with it, I'm OK with it. >TableBackgroundPainter::PaintCell(nsTableCellFrame* aCell, > nsRect cellRect; > cellRect = aCell->GetRect(); Why two lines? Just put that on one line, please. > //Paint cell background in border-collapse unless told not to. Why only in border-collapse? Please document that somewhere.... I'm wondering about the Pass* functions.... if I decide to call PassRow(), I need to make sure I don't LoadRow() that row, right? Because it looks like if I load a row and then call PassRow() on it, the row's background will be painted. Could we add some debug-only code that asserts that LoadRow() is not called while another row is already loaded, and similarly for row groups, etc?
By the way, I just realized that you have some places where you call Clear() when you don't want to call the destructor. So it's OK to keep Clear(), but the destructor can call it, which would allow callers who plan to just destroy to not bother with having to call Clear().
Comment on attachment 135354 [details] nsTableUnderlay.h One more thing. The various members here need documenting.... It's not clear what role mOrigin plays, what mCols is used for, what mRow and mRowGroup are used for, what mZeroBorder and mZeroPadding are supposed to do, etc.
Comment on attachment 135353 [details] [diff] [review] patch >Index: mozilla/layout/html/table/src/nsTableFrame.h >+ /** Get width of table + colgroup + col collapse: elements that >+ * continue along the length of the whole left side. >+ * @param aPixelsToTwips - conversion factor >+ * @param aGetInner - get only inner half of border width >+ */ It's not clear to me what this does, still... This is getting the width of _which_ border? The left one? The left border of what element? Why "continuous"? This would benefit greatly from an example, I think. >+ PRUint32 mLeftContBCBorder:8; Comment that this, like other BC code, rather arbitrarily limits the widths of BC borders? That said, why is this 8 bits? I thought BC borders were currently limited to 6 bits? I've gotten up to the beginning of nsTableFrame::Paint() for now; more later.
> Why isn't that a pointer? References to interfaces like that make me queasy. ... > Could you use a pointer to the nsIRenderingContext instead? Because everything else refers to it that way and I'm copying them. Would you rather have a pointer? > Why? If this class always has a frame, it can get the prescontext off the frame. I didn't think of that.. If it's in the frame, then why is it being passed around the paint functions? > Unless mFrame can be null? hm.. Don't think it would have a generated borderstyle if it did. > Any reason not to just inline this into the header? I wanted to keep all the implementation logic in one file. (It's not just returning a value; it's making a judgement.) >>+ /** Get width of table + colgroup + col collapse: elements that >>+ * continue along the length of the whole left side. >>+ * @param aPixelsToTwips - conversion factor >>+ * @param aGetInner - get only inner half of border width >>+ */ > >It's not clear to me what this does, still... This is getting the width of >_which_ border? The left one? The left border of what element? Why >"continuous"? The left table border (nsTABLEFrame::GetContinuousLEFTBCBorder)--which is also the leftmost column's left border, which is also the leftmost colgroup's left border. It is taking the width of the "table + colgroup + col collapse" -- i.e. the border's width before it collapses with the rowgroups/rows/cells. Continuous because the table/colgroup/col elements necessarily extend across the entire length of the table from top to bottom. (Rowgroup/row/cell borders do not: there can be multiple rowgroup/row/cell borders along this edge of the table.) > I thought BC borders were currently limited to 6 bits? http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/nsTableCellFrame.h#483 Thanks for all the comments, bz. I'll deal with the rest of them as I fix the problems they point out. :)
> Because everything else refers to it that way and I'm copying them. In that case, I guess keep it as-is (if you're getting passed nsIRenderingContext& yourself in the paint code; I've not looked at that yet). > The left table border (nsTABLEFrame::GetContinuousLEFTBCBorder Put all this in the relevant comments in the patch, please. ;)
I almost forgot... The reason the prescontext is passed around and all that is that there didn't use to be an accessor for it off the frame. But there is one now, and we're slowly moving to eliminating prescontexts from all sorts of function signatures.
> Why "nsTableUnderlay"? Wouldn't "nsTableBackgroundPainter" be a better > reflection of what's going on? *thinks back* I put that there because the header includes border-related things as well. Suppose you explain why bug 149378 is blocked by this. Should I change it to nsTablePainter? (In terms of things in the distant-and-likely-nonexistant-future, I've also contemplated a redesign of BC handling that would find the caching features of the painter most useful. ^^)
> Suppose you explain why bug 149378 is blocked by this. Because the measurements there should be redone once this patch goes in, since it will significantly change the control flow of table painting. > Should I change it to nsTablePainter? That may be a better description of what it does, yes.
Comment on attachment 135353 [details] [diff] [review] patch >Index: mozilla/layout/html/table/src/nsTableFrame.cpp > nsTableFrame::Paint(nsIPresContext* aPresContext, >+ if (eCompatibility_NavQuirks != mode) { You need a comment right before this check explaining why the check is made (that is, what the behavior difference should be between the modes, and why this behavior difference is desired). It looks like we're just doing our old painting in quirks mode and CSS2 painting in standards mode; is there a good reason for this quirk? >+ rv = painter.PaintTable(this, (nsTableRowGroupFrame*)rowGroups.ElementAt(0), >+ (nsTableRowGroupFrame*)rowGroups.ElementAt(numRowGroups-1)); What if numRowGroups is 0? Eg. an XHTML <table> with no <tbody> and no rows... > + if (rg->GetView()) { GetView() is slow. This is why we have HasView(). > + if (row->GetView()) { Same. >+ else if (aDirtyRect.YMost() > rgRect.y + rect.y) { Why that test instead of an Intersects() test (with an appropriately translated rect)? Won't this trigger painting of rows/cells that come completely before the dirty rect? Also, why ">" instead of ">="? >+ rv = painter.PaintCell(cell, (PRBool)cell->GetView()); HasView() > + OrderRowGroups(rowGroups, numRowGroups); > + rv = painter.PaintTable(this, (nsTableRowGroupFrame*)rowGroups.ElementAt(0), > + (nsTableRowGroupFrame*)rowGroups.ElementAt(numRowGroups-1), Again, what if numRowGroups == 0? >+nsTableFrame::SetColumnDimensions(nsIPresContext* aPresContext, Comment clearly in here why the cellSpacingY affects all these heights, please. Also comment why cellspacing affects origins. >+ if (colGroupWidth) { >+ colGroupWidth -= cellSpacingX; >+ } Comment that you are doing this to cancel out the extra cellSpacingX we counted for the last column? >@@ -5740,16 +5816,17 @@ nsTableFrame::CalcBCBorders(nsIPresConte >+ CalcDominateBorder(this, cgFrame, colFrame, nsnull, nsnull, >+ nsnull, PR_TRUE, NS_SIDE_RIGHT, PR_TRUE, t2p, //XXXfr why true? You added this code... why did you use true? Add a clearer XXX comment if needed. >@@ -5878,16 +5992,28 @@ nsTableFrame::CalcBCBorders(nsIPresConte >+ nsnull, PR_TRUE, NS_SIDE_RIGHT, PR_TRUE, t2p, //XXXfr why true? Same. >+ nsnull, PR_TRUE, NS_SIDE_RIGHT, PR_TRUE, t2p, //XXXfr why true? Same. I got up to the "@@ -6087,26 +6228,46 @@ nsTableFrame::CalcBCBorders(nsIPresConte" part. More later.
Comment on attachment 135353 [details] [diff] [review] patch I'm not sure I can usefully review your CalcDominateBorder calls, btw... if you could have bernd look them over, that would be great. >@@ -6087,26 +6228,46 @@ nsTableFrame::CalcBCBorders(nsIPresConte >+ if (!gotRowBorder && 1 == info.rowSpan) { >+ //get continuous row/row group border This could use a nice long comment explaining _why_ you're doing this, not just what you're doing. It's not obvious to me what "gotRowBorder" means at this point, what info.rowSpan really means, and why you care about this particular condition. >Index: mozilla/layout/html/table/src/nsTableRowGroupFrame.h >+ // border widths in pixels in the collapsing border model >+ unsigned mRightContBorderWidth:8; >+ unsigned mBottomContBorderWidth:8; >+ unsigned mLeftContBorderWidth:8; Any reason not to just make these PRUint8? Also, I assume we have very few frames of this type around? Otherwise adding members to them is not so great... If this is something that will be used only rarely, perhaps it could be stored as a property of the frame. If it's something that needs to be accessed often, I guess we need to byte the bullet and just store this? >+inline nscoord >+nsTableRowGroupFrame::GetContinuousBCBorderWidth(float aPixelsToTwips, Is there a good reason to inline this? It seems like a nontrivial bit of code, especially if it's called often. >Index: mozilla/layout/html/table/src/nsTableRowGroupFrame.cpp >+ if (NS_FRAME_PAINT_LAYER_BACKGROUND == aWhichLayer && >+ !(aFlags & (NS_PAINT_FLAG_TABLE_BG_PAINT | NS_PAINT_FLAG_TABLE_CELL_BG_PASS))) { This could use a comment explaining what exactly this check means (as far as I can tell, "Paint() called directly because we have a view" or something like that. >+ if (row->GetView()) { HasView() >+ else if (aDirtyRect.YMost() > rect.y){ Again, why this check as opposed to an intersects check? >+ rv = painter.PaintCell(cell, (PRBool)cell->GetView()); HasView() This whole chunk of code duplicates code in the inner parts of the loops in nsTableFrame. Is there really no way to push this code down into the painter? It could decide whether to run the outer part of the loop based on what the origin is or something... In fact, here you would just call PaintRowGroup() on the painter, and the loop over the rows logic (as well as checking for views) could live in the PaintRowGroup() method, no? I suppose you're trying to have the painter present a more flexible interface, but do we really envision using the extra flexibility? Would there ever be a case when one _would_ want to paint kids with views in this sort of situation? >+void nsTableRowGroupFrame::SetContinuousBCBorderWidth(PRUint8 aForSide, Will this never get called with NS_SIDE_RIGHT? If so, add a debug-only case with an NS_ERROR to that effect. Also, as written this will trigger unhandled value warnings; you may want to add an empty "default" to this switch (and add a return to the third case!). >Index: mozilla/layout/html/table/src/nsTableRowFrame.h >+ unsigned mRightContBorderWidth:8; >+ unsigned mTopContBorderWidth:8; >+ unsigned mLeftContBorderWidth:8; This is increasing the size of nsTableRowFrame, and I _know_ we have lots of those.... is there really no way not to store this state? Perhaps we should have a BC-only class for this like we do for table cells? >+inline void >+nsTableRowFrame::GetContinuousBCBorderWidth(float aPixelsToTwips, >+inline nscoord nsTableRowFrame::GetOuterTopContBCBorderWidth(float aPixelsToTwips) Again, why inline the first of these? Is there any reason these are two separate functions for table rows but combined into a single function for table row groups? It would be better to be consistent, imo. >Index: mozilla/layout/html/table/src/nsTableRowFrame.cpp >+ if (NS_FRAME_PAINT_LAYER_BACKGROUND == aWhichLayer && >+ !(aFlags & (NS_PAINT_FLAG_TABLE_BG_PAINT | NS_PAINT_FLAG_TABLE_CELL_BG_PASS))) { Same comment as for nsTableRowGroupFrame >+ rv = painter.PaintCell(cell, (PRBool)cell->GetView()); HasView(). Once again, this would be better in the painter, imo... >+void nsTableRowFrame::SetContinuousBCBorderWidth(PRUint8 aForSide, Same comments as for table row groups. >Index: mozilla/layout/html/table/src/nsTableColGroupFrame.h >+ PRUint32 mTopContBorderWidth:8; >+ PRUint32 mBottomContBorderWidth:8; PRUint8. >Index: mozilla/layout/html/table/src/nsTableColGroupFrame.cpp >+void nsTableColGroupFrame::SetContinuousBCBorderWidth(PRUint8 aForSide, Similar comments as for row groups. >Index: mozilla/layout/html/table/src/nsTableColFrame.h >+ // border width in pixels >+ PRUint32 mLeftBorderWidth: 8; >+ PRUint32 mRightBorderWidth: 8; >+ PRUint32 mTopContBorderWidth:8; >+ PRUint32 mRightContBorderWidth:8; >+ PRUint32 mBottomContBorderWidth:8; PRUint8. I guess we don't have as many colframes as rowframes in really big tables, so this may be OK.... >+inline nscoord >+nsTableColFrame::GetContinuousBCBorderWidth(float aPixelsToTwips, >+ nsMargin& aBorder) Again, I don't think this should be inlined. >Index: mozilla/layout/html/table/src/nsTableColFrame.cpp >+void nsTableColFrame::SetContinuousBCBorderWidth(PRUint8 aForSide, >+ nscoord aPixelValue) Same comments as for row groups, etc. >Index: mozilla/layout/html/table/src/nsTableCellFrame.cpp >- nsILookAndFeel* look = nsnull; Hey, if you're touching this code anyway, make the thing an nsCOMPtr, eh? >@@ -417,17 +418,17 @@ nsTableCellFrame::Paint(nsIPresContext* >- PRBool paintChildren = PR_TRUE; >+ PRBool paintChildren = PR_TRUE; Why make changes like that? Just muddles up the CVS history.... >@@ -435,17 +436,17 @@ nsTableCellFrame::Paint(nsIPresContext* >- >+ Same. >@@ -1363,18 +1364,19 @@ nsBCTableCellFrame::PaintUnderlay(nsIPre >+ if (aVisibleBackground && >+ (!(aFlags & NS_PAINT_FLAG_TABLE_BG_PAINT) || >+ (aFlags & NS_PAINT_FLAG_TABLE_CELL_BG_PASS))) { Again, need a comment as to what that conditional actually means... >@@ -1389,11 +1391,11 @@ nsBCTableCellFrame::PaintUnderlay(nsIPre >+ aFlags &= ~ (NS_PAINT_FLAG_TABLE_CELL_BG_PASS | NS_PAINT_FLAG_TABLE_BG_PAINT); Why do table cells need to reset this flag? It's not clear what that will do, just by looking at this code. Comment, please.
bernd, if you have thoughts of any kind on using a BC-only table row class that stores the extra border state, please comment... how well is that working out for table cells?
Boris, thanks for your careful review, it shows that comment 121 was correct. BC table cell frames work. I never touched them (Maybe thats the reason why they work). I like the idea to hide the bc burden from ordinary rows in the separate border case. I will review the CalcDominateBorder calls. Btw the empty table background case is bug 227123, and mozilla currently does the right thing, so please no regressions....
s/CalcDominateBorder/CalcDominantBorder/g ? /be
As a separate patch, sure. This one is already pretty big....
I would like to propose to spin of the border collapse part into a separate patch. It will otherwise jeopardize the 1.7a timetable for the whole patch. The border collapse review issues are not new (http://bugzilla.mozilla.org/show_bug.cgi?id=191731#c7) and when bz (comment 142) and dbaron ( http://bugzilla.mozilla.org/show_bug.cgi?id=225266#c8 ) try to avoid to review this code it sends out a clear message. And it is really hard to argue when looking at an undocumented function with 14 arguments like CalcDominantBorder (http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/nsTableFrame.cpp#5146) that it might cause comments like //XXXfr why true? To summarize I would prefer if this function can be reorganized before any further calls are added. I filed bug 229883 for this. @@ -927,30 +931,31 @@ public: + PRUint32 mLeftContBCBorder:8; + PRUint32 : 11; // unused } mBits; Why are mLeftContBCBorder not a member of the bcProperty like the other bc widths on a table? this would also eliminate the my objection against @@ -1101,16 +1106,25 @@ inline void nsTableFrame::SetBorderColla +inline nscoord +nsTableFrame::GetContinuousLeftBCBorderWidth(float aPixelsToTwips, + PRBool aGetInner) const why isn't there a Set method. @@ -5782,27 +5859,53 @@ nsTableFrame::CalcBCBorders(nsIPresConte + mBits.mLeftContBCBorder = ownerWidth; this should never happen, mBits should not accessed so directly. Why are the widths for the table not reset together with the other table widths? if (!tableBorderReset[NS_SIDE_TOP]) { 5738 propData->mTopBorderWidth = 0; 5739 tableBorderReset[NS_SIDE_TOP] = PR_TRUE; 5740 } A continuous bc border is that the thinnest continuous line? If yes + //get row continuous borders + CalcDominateBorder(this, info.cg, info.leftCol, info.rg, rowFrame, nsnull, PR_TRUE, NS_SIDE_LEFT, + PR_FALSE, t2p, owner, ownerBStyle, ownerWidth, ownerColor); + rowFrame->SetContinuousBCBorderWidth(NS_SIDE_LEFT, ownerWidth); Why are cell frames here excluded from the computation? Imagine that they have border style hidden. I would expect more some minimum mechanism. My impression is that the exclusion of frames at the CalcDominantBorder calls will create problems once a hidden style is defined on any of those excluded frames.
> To summarize I would prefer if this function can be reorganized before any > further calls are added. I filed bug 229883 for this. If this is done (and it would be fine with me), I would say the calls should still be added per this patch, just ifdefed out. That way lxr searches will show it when bug 229883 is being fixed (and they can be fixed to do the right thing then).
> I would like to propose to spin of the border collapse part into a separate > patch. It will otherwise jeopardize the 1.7a timetable for the whole patch. You want me to extricate the border collapse parts so that we check in one behavior (ignoring borders) and during the next release cycle (1.8a) change it to another (accounting for borders)?? Right now, my main concern wrt release timetables is whether I'll be able to handle all these comments in time for the freeze... :) (I won't have time to work on it this week.) > A continuous bc border is that the thinnest continuous line? No, it is the result of collapsing all the continuous borders on that edge. For vertical lines, this is table, colgroup, column. For horizontal lines, this is table, rowgroup, row. This is done so that backgrounds will line up along the edge. Cell borders are not continuous on a line: they are segments along it. Therefore they don't get counted. For example, setting a thick border on the leftmost cell should not shift the row background over; this way a striped background set on <tr> will line up across rows even if the cells are assigned arbitrary border widths. I really ought to write this up.
Blocks: 230133
For the record, the tests mentioned on this bug so far are currently at: http://fantasai.tripod.com/www-style/2002/table-backgrounds/tests/ http://dbaron.org/css/test/sec170501 http://dbaron.org/css/test/sec170501a http://dbaron.org/css/test/sec170501b It would be great if we could get some CSS2.1-test-case-guideline-compliant versions of those tests for the CSS2.1 test suite. :-) http://www.w3.org/Style/CSS/Test/guidelines.html
Blocks: 232101
Blocks: 232108
Blocks: 191567
Attached file nsTablePainter.h (not quite done) (obsolete) (deleted) —
Attached file (not quite done) (obsolete) (deleted) —
Latest patch will be up at http://fantasai.tripod.com/Mozilla/bugs/4510/patch.txt I'm not quite done, but nsTablePainter.* are, minus some comments, mostly ready for review. I'm still going over the nsTableFrame portions of the patch. I'm extremely busy tomorrow, though, so I thought I'd post these for now in case you (bz, bernd) wanted to look it over sooner rather than later. Responses to some of the comments: > Is there any reason these are two separate functions for table rows but > combined into a single function for table row groups? It would be better > to be consistent, imo. Return value is void now, so n/a. > Comment clearly in here why the cellSpacingY affects all these heights, please. > Also comment why cellspacing affects origins. That should be documented in CSS2.1 >+ CalcDominateBorder(this, cgFrame, colFrame, nsnull, nsnull, >+ nsnull, PR_TRUE, NS_SIDE_RIGHT, PR_TRUE, t2p, //XXXfr why true? > > You added this code... why did you use true? I just guessed based on what I saw elsewhere. I don't know what it does, which is why I ask. > Is there a good reason to inline this? It seems like a nontrivial bit of > code, especially if it's called often. It's only called once. > This is increasing the size of nsTableRowFrame, and I _know_ we have lots > of those.... As discussed, I'll file a bug on moving that into a frame property. > - PRBool paintChildren = PR_TRUE; > + PRBool paintChildren = PR_TRUE; > > Why make changes like that? Just muddles up the CVS history.... neater code for the next person to read?
Attachment #135354 - Attachment is obsolete: true
Attachment #135355 - Attachment is obsolete: true
> That should be documented in CSS2.1 Then point to the relevant spec section here.... That at least tells people where to look for the reasons for that code.
Blocks: 233532
@@ -5731,27 +5750,53 @@ nsTableFrame::CalcBCBorders(nsIPresConte . . . +CalcDominantBorder(this, cgFrame, colFrame, nsnull, nsnull, + nsnull, PR_TRUE, NS_SIDE_RIGHT, PR_TRUE, t2p, //XXXfr why true? so what does this code do IMHO, it looks for the table, the colgroupframe and colframe It queries for all of them right border. If the borders are the result of the rules attribute then they should be ignored. Furthermore the owner should be the adjacent frame. Afterwards only the owner width is used. If the request is going for the last PR_TRUE then the answer is it doesnt matter, you need the owner only when you store in the cellmap. If this request is for the first PR_TRUE then it really matters. So we are at the top row and we query for the right side of these frames, only for the rightmost edge we are certain that we hit an outer edge of the table for all others we know that we are inside the table. So in once case this should be PR_TRUE and in the majority of cases PR_FALSE. In the light of bug 43178 I think my comment 147 makes sense because it would free you from fixing this.
Attached patch patch (obsolete) (deleted) — — Splinter Review
newest nsTablePainter is up at http://fantasai.tripod.com/Mozilla/bugs/4510 I'll attach it once bz says its good enough :)
Attachment #135353 - Attachment is obsolete: true
Attachment #140946 - Attachment is obsolete: true
Attachment #140947 - Attachment is obsolete: true
Comment on attachment 141121 [details] [diff] [review] patch forgot to mention: the only difference between the nsTablePainter files previously attached and the ones I just put up is the comments and a couple lines I swapped in PaintCell (moved the get colIndex part below the empty-cells short-circuit).
Attachment #141121 - Flags: superreview?(bzbarsky)
Attachment #141121 - Flags: review?(bernd_mozilla)
Comments on the separate files: > nsTablePainter.h How about calling it mozTableBackgroundPainter.h? ;) > /* aPass params indicate whether to paint the element or to just pass through and They're called aPassThrough. Change the comment accordingly. > * See Public versions for function descriptions Why not just put these private methods down with the other private methods and skip this part? > /** Paint table elements' backgrounds down through table cells > * (Cells themselves will only be painted in border collapse) Maybe better said as "Paint background for the table frame and its children down through the cells (Cells ... ) > /** Paint table row group elements' backgrounds down through table cells > * (Cells themselves will only be painted in border collapse) And then this could be "Paint background for the table row and its children down....." > /** Paint table row elements' backgrounds down through table cells > * (Cells themselves will only be painted in border collapse) Likewise. Feel free to ignore these comments if you think what you wrote is clearer, of course. ;) > /** Calculate and set data all values to represent aFrame (which must be non-null) */ "and set all data values", perhaps? > /** True if need to set border-collapse border; must call Set beforehand */ > PRBool ShouldSetBCBorder(); Which Set? SetFull()? SetFrame()? SetBCBorder()? Some subset thereof? > private: > nsStyleBorder* mSynthBorder; Weird indent; that should line up with the other members. > nsRect mCellRect; //current cell "current cell rect" > nsTablePainter.cpp Same naming issue (make it match the classname). >TableBackgroundPainter::TableBackgroundData::SetBCBorder(nsMargin& aBorder, > mSynthBorder = new (aPainter->mPresContext) > nsStyleBorder(aPainter->mZeroBorder); So... mZeroBorder is just an optimization or something? This is the only thing it's used for, and you could as easily do mSynthBorder = new (aPainter->mPresContext) nsStyleBorder(aPainter->mPresContext); right? >TableBackgroundPainter::TableBackgroundPainter(nsTableFrame* aTableFrame, > mZeroBorder.SetBorderStyle(NS_SIDE_TOP, NS_STYLE_BORDER_STYLE_BLANK); Use STYLE_NONE here. dbaron just removed NS_STYLE_BORDER_STYLE_BLANK anyway. >TableBackgroundPainter::PaintTableFrame(nsTableFrame* aTableFrame, > tableData.mRect.MoveTo(0,0); Add "// Put tableData.mRect in the table frame's coordinate system" before that line. > if (aFirstRowGroup && aLastRowGroup && mNumCols > 0) { > //only handle non-degenerate tables; we need a more robust BC model > //to make degenerate tables' borders reasonable to deal with Weird indent. > aLastRowGroup->GetContinuousBCBorderWidth(mP2t, tempBorder); > border.bottom = tempBorder.bottom; > > nsTableRowFrame* rowFrame = aFirstRowGroup->GetFirstRow(); > if (rowFrame) { Why use the row for top and the rowgroup for bottom? Is there a reason? If so, comment it here. > border.left = aTableFrame->GetContinuousLeftBCBorderWidth(mP2t, PR_TRUE); Likewise -- why use the table's left border but the col's right border above? > nsresult rv = tableData.SetBCBorder(border, this); > if (NS_FAILED(rv)) { > return rv; > } Need to do "tableData.Destroy(mPresContext);" before returning, no? >TableBackgroundPainter::QuirksPaintTable(nsTableFrame* aTableFrame, > if (NS_FAILED(rv)) return rv; > if (rgRect.Intersects(mDirtyRect) && !rg->HasView() > && NS_SUCCEEDED(rv) && isVisible) { No need for the NS_SUCCEEDED check -- it can't fail. ;) > if (NS_FAILED(rv)) return rv; > if (mDirtyRect.YMost() > rowRect.y && !row->HasView() > && NS_SUCCEEDED(rv) && isVisible) { Again, no need for the NS_SUCCEEDED check. Why checking mDirtyRect.YMost() > rowRect.y instead of checking whether the rects intersect? Due to rowspanning cells? If so, please add a short comment to that effect. >TableBackgroundPainter::PaintTable(nsTableFrame* aTableFrame) > mCols = new ColData[mNumCols]; This allocates mCols. Since ColData has no constructor, the mColGroup pointers in all those are uninitialized. > /*Create data struct for column group*/ > cgData = new TableBackgroundData; > if (!cgData) return NS_ERROR_OUT_OF_MEMORY; Now some of the points in mCols are garbage and when you do the deletion loop over mCols in the destructor you will probably crash. I'd add a constructor to ColData that inits the mColGroup pointer to 0. This looks great, fantasai! Thank you very much for the clear up-front explanation of what this class is trying to do! Once you make the changes I just mentioned, these two files are good by me.
> How about calling it mozTableBackgroundPainter.h? ;) Scratch that. mozTableBgPainter.h/cpp, please. The other is just too long.
Comment on attachment 141121 [details] [diff] [review] patch >Index: mozilla/layout/html/table/src/nsTableCellFrame.cpp >-NS_METHOD >+NS_METHOD > nsTableCellFrame::Paint(nsIPresContext* aPresContext, NS_IMETHODIMP, as long as you're changing this. Other than that, looks good to me. sr=bzbarsky. Don't forget to change Makefile.in if when you rename nsTablePainter.cpp
Attachment #141121 - Flags: superreview?(bzbarsky) → superreview+
Attachment #135353 - Flags: superreview?(bzbarsky)
Attachment #135353 - Flags: review?(dbaron)
{ns/moz/}TableBackgroundPainter.{cpp,h} seems like fine names to me. The length isn't really a problem. Here are comments on the patch, although I skipped nsTableFrame::CalcBCBorders because it makes my head hurt, and I haven't looked through the new files thoroughly yet. nsStyleStruct.cpp: This should use a loop (probably the FOR_CSS_SIDES macro, which could be moved from nsCSSStruct.h to nsStyleConsts.h). But since that requires changing files in directories you're not otherwise touching, and should probably be done with a bit of other cleanup, I filed bug 233795 to remind myself to do it later. nsCSSRendering.h/cpp: aClipRect should probably be called aBGClipRect, since aClipRect is a reasonably standard name meaning something else nsTableFrame.h: Limiting BC borders to 63 pixels is wrong, and we shouldn't spread that elsewhere. (Same for nsTableRowFrame.h, nsTableRowGroupFrame.h, nsTableColFrame.h, nsTableColGroupFrame.h.) We should at least have a typedef for a pixel size of a BC border so we can find all the places that use PRUint8 or :8. Shouldn't GetContinuousLeftBCBorderWidth use the BC_BORDER_LEFT_HALF_COORD macro for symmetry, and in case the macros change? nsTableFrame.cpp: I dislike the NS_PAINT_FLAG_* business: NS_PAINT_FLAG_TABLE_CELL_BG_PASS is never set, so it can be removed. NS_PAINT_FLAG_TABLE_BG_PAINT can be replaced with a function on each inner table element class that checks if there's a view between that element (inclusive) and the table element (exclusive). But that is probably better done in a later patch (since it would allow the aFlags parameter to be removed throughout layout, I think). The nsTableBackgroundPainter code within nsTableFrame.cpp probably shouldn't be conditioned on the table having visibility visible (although that currently means the NS_PAINT_FLAG_TABLE_BG_PAINT flag isn't set). Instead, the nsTableBackgroundPainter code should be called unconditionally (which it looks like it will handle just fine). Likewise for the descendants with views that use it. Your use of the terms "direct" and "table-called" in comments in three places (nsTableRowGroupFrame::Paint, nsTableRowFrame::Paint, nsBCTableCellFrame::PaintUnderlay) is really making up terminology where we don't need extra terminology. What's happening is that an internal table element has a view (which, for table cells, can be caused by relative positioning, opacity, etc.). You should use the term "view" somewhere in those comments and get rid of the terms "direct" and "table-called". In the code in nsTableRow(Group)?Frame that creates a TableBackgroundPainter, do you want to set the flag tell the rows / cells not to paint? Likewise (as I said before), these calls probably shouldn't be conditional on visibility. nsTableColGroupFrame.cpp: nsTableColFrame guarantees not touching aBorder.left, so you don't need to do the fancy save-and-reassign dance in GetContinuousBCBorderWidth. nsTableColFrame: GetContinuousBCBorderWidth can return void -- nobody uses the result. nsTableCellFrame: I'm guessing Bernd wants you to remove a bunch of the PaintUnderlay-related changes, since they break NS_STYLE_TABLE_EMPTY_CELLS_SHOW_BACKGROUND, although removing the NS_PAINT_FLAG_TABLE_CELL_BG_PASS flag will allow you to remove the aPaintChildren argument. The comments you added about the flags are backwards, but you should notice that when you remove the unneeded one. Also, you're probably better in passing the value of GetStyleTableBorder()->mEmptyCells instead of a boolean and the const nsStyleTableBorder* (to get mEmptyCells again, just like for the boolean). nsTableBackgroundPainter.cpp (just the parts I've read in order to read the rest of the patch): In SetFull, is it really correct to ignore the border if visibility is hidden? That seems wrong. TableBackgroundPainter::QuirksPaintTable incorrectly applies visibility on row groups to rows and cells and visibility on rows to cells. TableBackgroundPainter::QuirksPaintTable compares the frame's rect to the dirty area when it needs to use the overflow area (unioned with the rect, or something -- it's messy). "<dfn>passed</dfn>" seems like a bad term to use because one already passes arguments to functions and paints in multiple passes. How about skipped (which conflicts only with aSkipSides)? Or just "has view"? Likewise aPassThrough / aPass should probably be renamed to aSkip or aHasView. The assertion at the beginning of PaintRow calls itself PaintRowGroup. Also, in both those assertions, "Quirks" isn't a caller, so you should probably say "must not call ... in quirks mode". Have you tested that moving a window over your testcases does the right thing? Try multiple directions, and try movement that's exactly horizontal or exactly vertical? I find the inconsistency in the ways you handle coordinate systems between cells/ rows / row groups / tables to be confusing, and confusion can easily lead to bugs. Perhaps it's better to be consistent with what the rest of the code does (always translate, i.e., get rid of mCellRect and call TranslateContext for rows as well)? Perhaps it's OK the way it is, but this really seems like premature optimization (which is said to be the root of all evil). Could you explain the logic of the changes (which I'm sure are connected) to: * the nsTableReflowState constructor * nsTableFrame::SetColumnDimensions * nsTableRowFrame::ReflowChildren Also, if you want to include added files in your diff, you can just add the following lines to the end of (or anywhere within) layout/html/table/src/CVS/Entries: /nsTableBackgroundPainter.h/0/dummy timestamp// /nsTableBackgroundPainter.cpp/0/dummy timestamp// Once you do that, if you add -N to the options you pass to diff (i.e., diff -Npu8), they should show up in the diff.
As fantasai points out, NS_PAINT_FLAG_TABLE_CELL_BG_PASS is set in the painter, so forget the bit about removing the flags. However, this means nsTableCellFrame::Paint needs to set PaintChildren based on it (just like it now does in one of the two implementations of PaintUnderlay) -- i.e., you need to pull that bit out of the BC version of PaintUnderlay.
> So... mZeroBorder is just an optimization or something? This is the only thing > it's used for, and you could as easily do > mSynthBorder = new (aPainter->mPresContext) > nsStyleBorder(aPainter->mPresContext); > right? Won't work; I need to give the border a width. STYLE_NONE makes it zero-width. And the default width is medium.
(In reply to comment #162) > Won't work; I need to give the border a width. What do you mean? Don't you explicitly assign some border widths immediately after that?
STYLE_NONE means there's no border; i.e. the border width must calculate to zero no matter what it's specified value.
Oh, hm... So do we need to restore STYLE_BLANK then?
I looked at the bc part between http://bugzilla.mozilla.org/attachment.cgi?id=141121&action=diff#mozilla/layout/html/table/src/nsTableFrame.cpp_sec11 and http://bugzilla.mozilla.org/attachment.cgi?id=141121&action=diff#mozilla/layout/html/table/src/nsTableFrame.cpp_sec19 They look OK to me. I am not too worried about NS_STYLE_TABLE_EMPTY_CELLS_SHOW_BACKGROUND changes the only wrong place I already mentioned to fantasai. Just for the record NS_STYLE_TABLE_EMPTY_CELLS_SHOW_BACKGROUND is only available in quirks mode. It emulates the NN4 which has shown the background for empty cells but not the border. NS_STYLE_TABLE_EMPTY_CELLS_SHOW_BACKGROUND is used as default, as soon as empty-cells is either shown or hide this quirk doesn't apply. >Here are comments on the patch, although I skipped >nsTableFrame::CalcBCBorders because it makes my head hurt You are of course involved in the offer I made to boris, if you rereview the 2300 lines of bc code like you did with the overflow stuff, I will bring the code in accordance to your review comments. This seems to me necessary as r=alexsavulov for this type of patch did not ensure at least readability of the code. I filed already bug 229883 and attached a patch to the bug, to get rid of the large numbers of arguments to CalcDominantBorder. But I am weak at these general style coding stuff.
I think STYLE_SOLID will work; PaintBackground doesn't paint the borders, it just uses them for image positioning.
> STYLE_NONE means there's no border; i.e. the border width must calculate to > zero no matter what it's specified value. It must _compute_ to zero, even, so this should happen during the cascade. e.g.: div { border: none 2em red; } span { border: inherit; border-style: solid; } <div><span> test </span></div> ...should not have any border. (We currently do this wrong. It works in Opera.)
Ian, that is off-topic, and this is a long bug. But since you bring it up: http://lists.w3.org/Archives/Public/www-style/2004Feb/0160.html
Blocks: 171523
Attached patch patch (obsolete) (deleted) — — Splinter Review
> Your use of the terms "direct" and "table-called" in comments in three The significance of having a view is that it means ::Paint is not called by an ancestor table element, which would have painted the element's background for it. Therefore, I prefer to keep those comments intact. I have, however, added a note about views in the flag declarations. > nsTableColFrame::GetContinuousBCBorderWidth can return void -- nobody uses the result. The painter does. > In SetFull, is it really correct to ignore the border if visibility is > hidden? That seems wrong. Why? If we're not painting the background, then we have no need for the border. > TableBackgroundPainter::QuirksPaintTable compares the frame's rect to > the dirty area when it needs to use the overflow area To the best of my knowledge, backgrounds never overflow their respective elements' mRects. > "<dfn>passed</dfn>" seems like a bad term to use.. aSkip Skip to me implies that the entire section will be skipped over. PassThrough emphasizes that we're going through the section. Changed "passed" to "passed through". > Have you tested that moving a window over your testcases does the > right thing? Yes. > Could you explain the logic of the changes (which I'm sure are > connected) to: Makes the row and column dimensions start at the first cell border edge rather than the table edge. See "Background Boundaries" in http://fantasai.tripod.com/www-style/2002/table-backgrounds/
Attachment #141121 - Attachment is obsolete: true
Attachment #141851 - Flags: superreview?(dbaron)
Attachment #141851 - Flags: review?(bernd_mozilla)
I think you should revert the GetRowGroupFrame change or modify it to compile on win with VC. e:/moz_src/mozilla/layout/html/table/src/nsTableFrame.cpp(1233) : error C2724: 'GetRowGroupFrame' : 'static' should not be used on member functions defined at file scope http://msdn.microsoft.com/library/default.asp?url=/library/en-us/vccore/html/C2724.asp
The trick is that for static member functions you're supposed to say |static| when you declare it but not when you define it.
somehow VC++ doesnt like the access to the private member variables mPresContext and mZeroBorder at the following snippet. My first guess is that you either need a get function or change the arguments of this function. nsresult TableBackgroundPainter::TableBackgroundData::SetBCBorder(nsMargin& aBorder, TableBackgroundPainter* aPainter) { NS_PRECONDITION(aPainter, "null painter"); if (!mSynthBorder) { mSynthBorder = new (aPainter->mPresContext) nsStyleBorder(aPainter->mZeroBorder); if (!mSynthBorder) return NS_ERROR_OUT_OF_MEMORY; } e:/moz_src/mozilla/layout/html/table/src/nsTablePainter.cpp(209) : error C2248: "mPresContext" : cannot access mPresContext declared in class "TableBackgroundPainter" e:/moz_src/mozilla/layout/html/table/src/nsTablePainter.h(247) : see declaration of 'mPresContext' e:/moz_src/mozilla/layout/html/table/src/nsTablePainter.cpp(210) : error C2248: "mZeroBorder" : cannot access mZeroBorder declared in class "TableBackgroundPainter" e:/moz_src/mozilla/layout/html/table/src/nsTablePainter.h(263) : see declaration of 'mZeroBorder' http://msdn.microsoft.com/library/default.asp?url=/library/en-us/vccore/html/C2248.asp
Since different versions of the C++ standard have different rules on member access and nested classes, it's best to declare all nested classes or structs this way: class Container { // ... class Nested; friend class Nested; class Nested { // ... }; // ... }; so that the nested class is a friend of its containing class (which does nothing in the current C++ standard, but did something in the 1998 version).
Even then you may need to make those members protected, not private.
Comment on attachment 141851 [details] [diff] [review] patch I removed the static at nsTableRowGroupFrame* nsTableFrame::GetRowGroupFrame(nsIFrame* aFrame, nsIAtom* aFrameTypeIn) further I added struct TableBackgroundData; friend struct TableBackgroundData; before the MOZ_DECL_CTOR_COUNTER(TableBackgroundData) With these changes I could compile it with VC++ 6.0. Then I added a nsTableFrame* table = (nsTableFrame*)aTableFrame->GetFirstInFlow(); nsFrameList& colGroupList = table->GetColGroups(); as the colgroup list is only valid on the first table frame. I wonder if it would be better to incorporate this into the getcolgroups function Further I replaced all PR_FALSE arguments in the PaintBackgroundWithSC calls with PR_TRUE otherwise we would paint the backgrounds during printing even if the user deselected it in the page setup. with those changes r+
Attachment #141851 - Flags: review?(bernd_mozilla) → review+
Attached patch final patch (?) (deleted) — — Splinter Review
Made all the changes Bernd mentioned, and switched some casts over to NS_STATIC_CAST per dbaron's instructions. r/sr?
Attachment #141851 - Attachment is obsolete: true
Blocks: 88260
So what's the status of the issues raised in comment 160? I assume that last patch addresses them? If so, please let me know and I will try to sr it this Saturday. We seem to have ended up with three reviewers and two reviews that need to happen.... and no one being sure what's going on.
Yeah, the second-to-last patch fixed them. See comment 170. Further responses: > {ns/moz/}TableBackgroundPainter.{cpp,h} Kept as nsTablePainter, per discussion on IRC. > nsStyleStruct.cpp: dbaron says he filed a bug already. > aClipRect should probably be called aBGClipRect Done. > Limiting BC borders to 63 pixels ... typedef for a pixel size of a BC border Typedefed BCPixelSize. > Shouldn't GetContinuousLeftBCBorderWidth use the BC_BORDER_LEFT_HALF_COORD macro Don't see how this comment applies. > The nsTableBackgroundPainter should be called unconditionally. Fixed. > In the code in nsTableRow(Group)?Frame that creates a > TableBackgroundPainter, do you want to set the flag tell the rows / > cells not to paint? Fixed. > don't need to do the fancy save-and-reassign dance in GetContinuousBCBorderWidth. Fixed. > break NS_STYLE_TABLE_EMPTY_CELLS_SHOW_BACKGROUND Fixed. > TableBackgroundPainter::QuirksPaintTable incorrectly applies visibility Fixed. > say "must not call ... in quirks mode". Fixed.
I'll still try to get to this tonight or tomorrow morning, if we want to land this for 1.7b. I personally feel that that's a reasonable course of action. If someone disagrees, please say so...
Boris, I disagree. Due to my limited time (and knowledge) resources, I can't make sure that I have during the the freeze enough resources to iron out more than regression. This code is large, at least IMHO, so my guess is we will have more than just one critical regression.The patch is so clearly alpha material, that I thought it might be possible to get it in in early beta, but I refuse to go days before the freeze. If you could make the sr so that it goes into 1.8a I would be already delighted.
Yeah, I can definitely make sure I sr in time for 1.8a (well before, in fact). I just hate missing the boat two milestones in a row now.... :( But if you have misgivings about this patch, then we shouldn't.
I agree with slipping to 1.8a. This is the kind of thing where we've been wrong forever, and another few months of being wrong the same way is no big deal, and far preferable to the chance of being wrong in a different way for just a few months.
I think we should get this in for 1.7b. We've got plenty of time before 1.7b releases, never mind after it, to see if things go wrong. If they go wrong badly, we can back it out and reland for 1.8a. If it's not serious, a few minor regressions are definitely worth moving towards a process with which we have a chance of attracting new contributors.
Comment on attachment 142639 [details] [diff] [review] final patch (?) Based on diffing diffs and rereading review comments above, sr=dbaron. This is a big patch, we've had a lot of review, and at some point we just need to stop reviewing and check in.
Attachment #142639 - Flags: superreview+
Attachment #141851 - Flags: superreview?(dbaron)
fix checked in, instead of flowers. Happy Intl. Womens Day Fantasai!!
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 21 years ago
Resolution: --- → FIXED
Finally! :) Excellent work all contributors.
Someone should probably go through the dependencies of this bug and see which ones are now fixed. I'm not optimistic, though -- many seem like bogus dependencies.
I added a number of bugs as dependencies because there was not much point of testing them until the patch landed... I'll probably try to go through the deps tonight.
One other question -- what's now implemented for standards mode is a good bit closer to quirks mode than the old version of standards mode background painting. Are there any reasons we still need the quirks? If not, should we file a bug to remove the mode differences when we open for 1.8a?
There was some discussion of that in comment 103, comment 104, comment 115. I would be in favor of removing the quirk in 1.8a and seeing what happens, myself. The non-quirk behavior seems vastly more preferable in most situations I can think of....
This commit have added a "may be used uninitialized" warning on brad TBox: +layout/html/table/src/nsTableFrame.cpp:5679 + `PRBool gotRowBorder' might be used uninitialized in this function
Hmm.. that warning looks correct to me. We should init it (presumably to true, given the following code?)
Hmmm. My guess was that it should be initialized to false, but I'm not sure.
Although, actually, iter.IsNewRow() might be guaranteed to be true the first time through the loop, which would mean it's a bogus warning.
iter.First calls SetNewGroup wich calls SetNewRow which sets mIsNewRow to true, so the call to IsNewRow will for the first cell that is iterated over always be true. However I think given the difficulties to read the border collapse assembly code, I will be more explicit that it is PR_FALSE at the start in the patch for bug 229883, which I delayed to get this patch here up and running first.
Oh, so /this/ is why I had unusually much bugmail. :) Thank you very much, everyone. *doffs hat and bows* So, as for Quirks, I was looking over Tantek's shoulder while he pulled up one of my tests at the Sofitel. I suspect MacIE doesn't do the transparent thing, because it was definitely painting colgroups as colgroup backgrounds and not inherited cell backgrounds. So I'm guessing we should be all right with getting rid of the quirk, although saving it for 1.8a sounds fine to me. Filed bug 237078 on the issue.
Blocks: 237078
Attachment #141121 - Flags: review?(bernd_mozilla)
(In reply to comment #152) > > This is increasing the size of nsTableRowFrame, and I _know_ we have lots > > of those.... > > As discussed, I'll file a bug on moving that into a frame property. Did that ever get filed?
No longer blocks: 233532
I think I found a bug in this fix. See bug 267592 happens in mozilla and firefox
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: