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)
Core
Layout: Tables
Tracking
()
RESOLVED
FIXED
People
(Reporter: dbaron, Assigned: fantasai.bugs)
References
()
Details
(Keywords: css2, testcase, Whiteboard: [awd:tbl])
Attachments
(4 files, 17 obsolete files)
(deleted),
patch
|
karnaze
:
review+
attinasi
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(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.
Updated•26 years ago
|
Status: NEW → ASSIGNED
Target Milestone: M5
Reporter | ||
Updated•26 years ago
|
OS: Linux → All
Hardware: PC → All
Reporter | ||
Comment 2•26 years ago
|
||
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.
Updated•25 years ago
|
Target Milestone: M5 → M6
Comment 3•25 years ago
|
||
Moving to M6.
Comment 4•25 years ago
|
||
Moving to M8
Comment 6•25 years ago
|
||
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.
Updated•25 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
Comment 7•25 years ago
|
||
Fixed with latest checkin. Only works correctly in Standard Mode.
Updated•25 years ago
|
Status: RESOLVED → VERIFIED
Comment 8•25 years ago
|
||
Using 6/14 Apprunner, colors display an indicated on test cases. Verifying bug
fixed.
Comment 9•24 years ago
|
||
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)
Reporter | ||
Comment 10•24 years ago
|
||
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 → ---
Reporter | ||
Comment 11•24 years ago
|
||
I think this should be fixed for beta3. Assuming we decide to do it, it should
be very easy to do.
Keywords: nsbeta3
Updated•24 years ago
|
Status: REOPENED → ASSIGNED
Comment 12•24 years ago
|
||
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.
Updated•24 years ago
|
Target Milestone: --- → Future
Comment 14•24 years ago
|
||
*** Bug 48013 has been marked as a duplicate of this bug. ***
Comment 15•24 years ago
|
||
see also bug 47563
Reporter | ||
Comment 16•24 years ago
|
||
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
Reporter | ||
Comment 17•24 years ago
|
||
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
Assignee | ||
Comment 18•24 years ago
|
||
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>?
Comment 20•23 years ago
|
||
nomination of mozilla0.9 conflicts w/ target milestone of future. clobbering
both and nsbeta3-. Can we please try to get closure on this bug?
Assignee | ||
Comment 21•23 years ago
|
||
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=20181 [bug 46268]
should fix this for everything except table rows (a necessary quirk).
Comment 22•23 years ago
|
||
what needs to be done on this one?! Hope to see this one fixed soon.
Comment 23•23 years ago
|
||
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.
Comment 24•23 years ago
|
||
The WG is looking at this. I hope to have an answer for you on Monday.
Whiteboard: (WG bug 85)
Comment 25•23 years ago
|
||
*** Bug 87232 has been marked as a duplicate of this bug. ***
Comment 26•23 years ago
|
||
*** Bug 90247 has been marked as a duplicate of this bug. ***
Comment 27•23 years ago
|
||
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
Updated•23 years ago
|
Target Milestone: --- → mozilla1.0
Comment 28•23 years ago
|
||
*** Bug 84986 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 29•23 years ago
|
||
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"
Comment 30•23 years ago
|
||
*** Bug 113067 has been marked as a duplicate of this bug. ***
Comment 31•23 years ago
|
||
*** Bug 119442 has been marked as a duplicate of this bug. ***
Comment 32•23 years ago
|
||
*** Bug 119442 has been marked as a duplicate of this bug. ***
Keywords: mozilla0.9.9
Comment 33•23 years ago
|
||
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.
Assignee | ||
Comment 34•23 years ago
|
||
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.
Assignee | ||
Comment 35•23 years ago
|
||
Comment 36•23 years ago
|
||
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+
Assignee | ||
Comment 37•23 years ago
|
||
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 38•23 years ago
|
||
Comment on attachment 69072 [details] [diff] [review]
temporary fix
sr=attinasi (egg-shields up)
Attachment #69072 -
Flags: superreview+
a=roc+moz
Comment 41•23 years ago
|
||
fix checked in
Comment 43•23 years ago
|
||
fixed
Status: NEW → RESOLVED
Closed: 25 years ago → 23 years ago
Resolution: --- → FIXED
Updated•23 years ago
|
Keywords: mozilla0.9.9 → mozilla0.9.9+
Assignee | ||
Comment 44•23 years ago
|
||
This bug isn't actually fixed yet. Reopening and moving to future.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla1.0 → Future
Comment 45•23 years ago
|
||
*** Bug 116212 has been marked as a duplicate of this bug. ***
Comment 46•23 years ago
|
||
*** Bug 134261 has been marked as a duplicate of this bug. ***
Comment 47•22 years ago
|
||
*** Bug 137707 has been marked as a duplicate of this bug. ***
Comment 48•22 years ago
|
||
*** Bug 137779 has been marked as a duplicate of this bug. ***
Comment 49•22 years ago
|
||
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.
Assignee | ||
Comment 50•22 years ago
|
||
> 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.
Comment 51•22 years ago
|
||
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!
Comment 52•22 years ago
|
||
*** Bug 140084 has been marked as a duplicate of this bug. ***
Comment 53•22 years ago
|
||
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.
Assignee | ||
Comment 54•22 years ago
|
||
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.
Comment 55•22 years ago
|
||
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!
Reporter | ||
Comment 56•22 years ago
|
||
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.
Assignee | ||
Comment 57•22 years ago
|
||
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?
Comment 58•22 years ago
|
||
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.
Reporter | ||
Comment 59•22 years ago
|
||
> 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
Comment 60•22 years ago
|
||
*** Bug 149008 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 61•22 years ago
|
||
*** Bug 172824 has been marked as a duplicate of this bug. ***
Comment 62•22 years ago
|
||
*** Bug 175075 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 63•22 years ago
|
||
Test suite: http://fantasai.tripod.com/www-style/2002/table-backgrounds/tests/
It's a work in progress; only part A has been written.
Assignee | ||
Comment 64•22 years ago
|
||
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.
Comment 65•22 years ago
|
||
*** Bug 179086 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 66•22 years ago
|
||
*** Bug 180404 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 67•22 years ago
|
||
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
Comment 68•22 years ago
|
||
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.
Assignee | ||
Comment 69•22 years ago
|
||
> 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.
Comment 70•22 years ago
|
||
*** Bug 190455 has been marked as a duplicate of this bug. ***
Comment 71•22 years ago
|
||
*** Bug 190942 has been marked as a duplicate of this bug. ***
Comment 72•22 years ago
|
||
*** Bug 196889 has been marked as a duplicate of this bug. ***
Comment 73•21 years ago
|
||
Removing mozilla0.9.9+, mozilla1.0, and nsbeta+ keywords as they are all
obsolete by many moons.
Keywords: mozilla0.9.9+,
mozilla1.0,
nsbeta1+
Comment 74•21 years ago
|
||
Please never again remove keywords you don't know about!
Keywords: mozilla0.9.9+,
nsbeta1+
Comment 75•21 years ago
|
||
If the keywords do not have the meaning that the "Keywords" page has, then
perhaps someone needs to update that page eh???
Comment 76•21 years ago
|
||
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.
Assignee | ||
Comment 77•21 years ago
|
||
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.
Assignee | ||
Comment 78•21 years ago
|
||
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
Assignee | ||
Comment 79•21 years ago
|
||
Attachment #106551 -
Attachment is obsolete: true
Assignee | ||
Comment 80•21 years ago
|
||
Assignee | ||
Comment 81•21 years ago
|
||
ok, Bernd. Fire away!
(There are a few things I wasn't sure of; these are marked with XXX.)
Assignee | ||
Comment 82•21 years ago
|
||
Attachment #125789 -
Attachment is obsolete: true
Assignee | ||
Comment 83•21 years ago
|
||
Attachment #125790 -
Attachment is obsolete: true
Assignee | ||
Comment 84•21 years ago
|
||
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.
Assignee | ||
Comment 85•21 years ago
|
||
> 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?
Assignee | ||
Comment 86•21 years ago
|
||
fixed background leak in quirks
testcase:
http://fantasai.tripod.com/www-style/2002/table-backgrounds/tests/border-table-quirks.html
Comment 87•21 years ago
|
||
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...
Assignee | ||
Comment 88•21 years ago
|
||
> 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.
Comment 89•21 years ago
|
||
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.
Assignee | ||
Comment 90•21 years ago
|
||
> 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...
Assignee | ||
Comment 91•21 years ago
|
||
Attachment #125788 -
Attachment is obsolete: true
Assignee | ||
Comment 92•21 years ago
|
||
Attachment #125925 -
Attachment is obsolete: true
Assignee | ||
Comment 93•21 years ago
|
||
Attachment #125928 -
Attachment is obsolete: true
Assignee | ||
Comment 94•21 years ago
|
||
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...
Reporter | ||
Comment 95•21 years ago
|
||
*** Bug 214877 has been marked as a duplicate of this bug. ***
Comment 96•21 years ago
|
||
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.
Assignee | ||
Comment 97•21 years ago
|
||
> 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.
Comment 98•21 years ago
|
||
> 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
Reporter | ||
Comment 99•21 years ago
|
||
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.
Assignee | ||
Comment 100•21 years ago
|
||
Turning this over to Bernd for review/checkin/etc.
The two new files should be MPLed, of course.
And please look over comment 85.
Assignee | ||
Comment 101•21 years ago
|
||
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)
Comment 102•21 years ago
|
||
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
Comment 103•21 years ago
|
||
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?
Assignee | ||
Comment 104•21 years ago
|
||
> 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 "tr" 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 "left continuous border"? 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.)
Comment 105•21 years ago
|
||
> 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)
Comment 106•21 years ago
|
||
Assignee | ||
Comment 107•21 years ago
|
||
> 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. :)
Comment 108•21 years ago
|
||
this is not a regression it has been like this with attachment 129063 [details] [diff] [review], the trick
is the small window
Reporter | ||
Comment 109•21 years ago
|
||
*** Bug 218541 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 110•21 years ago
|
||
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
Reporter | ||
Comment 111•21 years ago
|
||
*** Bug 219404 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 112•21 years ago
|
||
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
Assignee | ||
Comment 113•21 years ago
|
||
Patch updated. And it does compile. However, I don't know if it /works/ since I
still get linking errors...
Comment 114•21 years ago
|
||
*** Bug 221020 has been marked as a duplicate of this bug. ***
Comment 115•21 years ago
|
||
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..
Assignee | ||
Comment 116•21 years ago
|
||
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...
Comment 117•21 years ago
|
||
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 ?
Assignee | ||
Comment 118•21 years ago
|
||
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?
Updated•21 years ago
|
QA Contact: amar → ian
Whiteboard: [awd:tbl](WG bug 85) → [awd:tbl]
Comment 119•21 years ago
|
||
*** Bug 224283 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 120•21 years ago
|
||
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..
Updated•21 years ago
|
Flags: blocking1.6b+
Comment 121•21 years ago
|
||
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.
Assignee | ||
Comment 122•21 years ago
|
||
Attachment #129063 -
Attachment is obsolete: true
Assignee | ||
Comment 123•21 years ago
|
||
Attachment #129064 -
Attachment is obsolete: true
Assignee | ||
Comment 124•21 years ago
|
||
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)
Comment 125•21 years ago
|
||
*** Bug 225780 has been marked as a duplicate of this bug. ***
Updated•21 years ago
|
Summary: table, row, column, *-group backgrounds not working correctly → table, row, column, *-group backgrounds not working correctly (<col>, <colgroup>, table, style, background)
Assignee | ||
Comment 126•21 years ago
|
||
So... is this going to get reviewed in time for 1.6beta or should I set the
target to January (1.7alpha)?
Comment 127•21 years ago
|
||
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...
Assignee | ||
Comment 128•21 years ago
|
||
> 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. :)
Updated•21 years ago
|
Target Milestone: mozilla1.5beta → ---
Comment 129•21 years ago
|
||
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 130•21 years ago
|
||
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.
Comment 131•21 years ago
|
||
> 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 132•21 years ago
|
||
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?
Comment 133•21 years ago
|
||
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 134•21 years ago
|
||
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 135•21 years ago
|
||
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.
Assignee | ||
Comment 136•21 years ago
|
||
> 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. :)
Comment 137•21 years ago
|
||
> 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. ;)
Comment 138•21 years ago
|
||
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.
Assignee | ||
Comment 139•21 years ago
|
||
> 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. ^^)
Comment 140•21 years ago
|
||
> 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 141•21 years ago
|
||
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 142•21 years ago
|
||
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.
Comment 143•21 years ago
|
||
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?
Comment 144•21 years ago
|
||
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....
Comment 145•21 years ago
|
||
s/CalcDominateBorder/CalcDominantBorder/g ?
/be
Comment 146•21 years ago
|
||
As a separate patch, sure. This one is already pretty big....
Comment 147•21 years ago
|
||
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.
Comment 148•21 years ago
|
||
> 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).
Assignee | ||
Comment 149•21 years ago
|
||
> 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.
Comment 150•21 years ago
|
||
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
Assignee | ||
Comment 151•21 years ago
|
||
Assignee | ||
Comment 152•21 years ago
|
||
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
Comment 153•21 years ago
|
||
> 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.
Comment 154•21 years ago
|
||
@@ -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.
Assignee | ||
Comment 155•21 years ago
|
||
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
Assignee | ||
Comment 156•21 years ago
|
||
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)
Comment 157•21 years ago
|
||
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.
Comment 158•21 years ago
|
||
> How about calling it mozTableBackgroundPainter.h? ;)
Scratch that. mozTableBgPainter.h/cpp, please. The other is just too long.
Comment 159•21 years ago
|
||
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+
Updated•21 years ago
|
Attachment #135353 -
Flags: superreview?(bzbarsky)
Attachment #135353 -
Flags: review?(dbaron)
Reporter | ||
Comment 160•21 years ago
|
||
{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.
Reporter | ||
Comment 161•21 years ago
|
||
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.
Assignee | ||
Comment 162•21 years ago
|
||
> 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.
Comment 163•21 years ago
|
||
(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?
Assignee | ||
Comment 164•21 years ago
|
||
STYLE_NONE means there's no border; i.e. the border width must calculate to zero
no matter what it's specified value.
Comment 165•21 years ago
|
||
Oh, hm... So do we need to restore STYLE_BLANK then?
Comment 166•21 years ago
|
||
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.
Assignee | ||
Comment 167•21 years ago
|
||
I think STYLE_SOLID will work; PaintBackground doesn't paint the borders, it
just uses them for image positioning.
Comment 168•21 years ago
|
||
> 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.)
Assignee | ||
Comment 169•21 years ago
|
||
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
Assignee | ||
Comment 170•21 years ago
|
||
> 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)
Comment 171•21 years ago
|
||
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
Reporter | ||
Comment 172•21 years ago
|
||
The trick is that for static member functions you're supposed to say |static|
when you declare it but not when you define it.
Comment 173•21 years ago
|
||
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
Reporter | ||
Comment 174•21 years ago
|
||
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).
Comment 175•21 years ago
|
||
Even then you may need to make those members protected, not private.
Comment 176•21 years ago
|
||
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+
Assignee | ||
Comment 177•21 years ago
|
||
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
Comment 178•21 years ago
|
||
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.
Assignee | ||
Comment 179•21 years ago
|
||
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.
Comment 180•21 years ago
|
||
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...
Comment 181•21 years ago
|
||
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.
Comment 182•21 years ago
|
||
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.
Reporter | ||
Comment 184•21 years ago
|
||
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.
Reporter | ||
Comment 185•21 years ago
|
||
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+
Reporter | ||
Updated•21 years ago
|
Attachment #141851 -
Flags: superreview?(dbaron)
Comment 186•21 years ago
|
||
fix checked in, instead of flowers.
Happy Intl. Womens Day Fantasai!!
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 21 years ago
Resolution: --- → FIXED
Comment 187•21 years ago
|
||
Finally! :) Excellent work all contributors.
Reporter | ||
Comment 188•21 years ago
|
||
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.
Comment 189•21 years ago
|
||
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.
Reporter | ||
Comment 190•21 years ago
|
||
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?
Comment 191•21 years ago
|
||
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....
Comment 192•21 years ago
|
||
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
Comment 193•21 years ago
|
||
Hmm.. that warning looks correct to me. We should init it (presumably to true,
given the following code?)
Reporter | ||
Comment 194•21 years ago
|
||
Hmmm. My guess was that it should be initialized to false, but I'm not sure.
Reporter | ||
Comment 195•21 years ago
|
||
Although, actually, iter.IsNewRow() might be guaranteed to be true the first
time through the loop, which would mean it's a bogus warning.
Comment 196•21 years ago
|
||
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.
Assignee | ||
Comment 197•21 years ago
|
||
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)
Comment 198•21 years ago
|
||
(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?
Comment 199•20 years ago
|
||
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.
Description
•