Closed Bug 191567 Opened 22 years ago Closed 21 years ago

CSS ignored after non-supported style element used. Note: new since 1.1 release.

Categories

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

defect

Tracking

()

VERIFIED FIXED
Future

People

(Reporter: ibl, Unassigned)

References

()

Details

(Keywords: regression, testcase)

Attachments

(1 file, 1 obsolete file)

User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3a) Gecko/20021212 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3a) Gecko/20021212 the apparantly non-suported style attached to a TD <td valign=top style="background: #f0ffee url(cyansmall.jpg) repeat-y 0% 0% fixed" > causes subsequent implicit (class=... type) styles to be ignored. This is new behavior in Mozilla 1.3a. Viewing the same page shows the correct style in Mozilla 1.1. You can see the effect of removing the offending style in http://www.cs.odu.edu/~ibl/450/spr03/cssbug2.html possibly related to bug 158037. Reproducible: Always Steps to Reproduce: 1. create a page that uses a CSS stylesheet and a table. Use a stylesheet element (class=....) within a cell of the table. 2. Add the following to the TD that contains the cell just mentioned: style="background: #f0ffee url(SOMEJPG.jpg) repeat-y 0% 0% fixed" Actual Results: Stylesheet elements mentioned in #1 are ignored. Expected Results: done what the stylesheet directed it to do. Mozilla 1.1 does not reproduce the bug. But Mozilla 1.3a does. IE renders the offending stylesheet directive and otherwise behaves like 1.1.
Work for me. I uncommented this line from you example, but and it is work. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b) Gecko/20030129
I can't see the problem either... Which particular "class" styles are being ignored?
The rendering difference that I see in displaying the URL on Mozilla 1.1 20020826 vs. Mozilla 1.3a 2003013108 nightly is that on 1.1 there IS background color and a box around the "NO Assignments returned yet" text on the right, making it look the same as the phrase on the left; whereas in 2003013108 there is NEITHER background color nor a box around the aforementioned text. SUGGEST: STATUS: NEW; KEYWORD: regression
Attached file minimal-ish testcase (obsolete) (deleted) —
Attached file slightly better (deleted) —
Attachment #113316 - Attachment is obsolete: true
Ronald, thanks for the clarification. So here's the deal... When border-collapse is set to "collapse" (which rules="none" forces), the table paints things itself. In this case, it screws up the painting of a <td>s fixed background (it's not clear to me why the background is fixed, but... this is why you got the impression the style is "non-supported" -- the cell simply never intersects the background). The background overpaints the backgrounds of the <td>s kids, which is just wrong. Oh, it paints over their borders too. To tables. Note that setting opacity on the <td> or setting overflow on it does _not_ lead to this bug, so it's likely a problem with "background-position: fixed" specifically, not with "anything with a view". To recap, the minimal required elements to force a mispaint here are "border-collapse:collapse" no the table and "background-position:fixed" on the cell. The rest of the CSS in the testcase is just for looks.
Assignee: jst → table
Status: UNCONFIRMED → NEW
Component: DOM Style → Layout: Tables
Ever confirmed: true
Keywords: regression
OS: Windows 2000 → All
QA Contact: ian → amar
Hardware: PC → All
the code regulating the layering of borders and backgrounds in bc mode is at http://lxr.mozilla.org/mozilla/source/layout/html/table/src/nsTableFrame.cpp#1488
Oh, man... This is so moronic... 1) nsIFrame::Paint() can take a "aFlags" argument that defaults to 0. 2) Views are painted directly by the pres-shell, _not_ by their parent frames 3) Apparently my attempts to give the cell a view by other means than attached backgrounds were unsuccessful 4) When a table with border-collapse paints backgrounds, it does it in two passes. The first pass paints the backgrounds of the cells and _nothing_else_. The second pass paints the backgrounds and borders of the cell's contents. Then of course we do the whole foreground layer of painting. 5) The way we denote the "normal" second pass of background painting is to set aFlags to BORDER_COLLAPSE_BACKGROUNDS In other words, the _default_ way a table cell in a border-collapse table paints is to paint only its background but _not_ the borders or backgrounds of its kids. This is stupid. Now naturally when the presshell paints things with views it does not pass any special flags. So we get this bug. We could to reverse all the checks on BORDER_COLLAPSE_BACKGROUNDS such that we're setting a flag for the _first_, "special", pass. But then of course we won't paint the table cell background at all in this case, since the background is not painted in the "normal" pass and the presshell does not know it needs to do this weird thing with table flags (nor should it need to paint every single view twice on the off chance that the view corresponds to a table cell in a border-collapse table). So as I see it we have two options: 1) Use _two_ flags, not one. A flag for "first pass" and a flag for "second pass". No flags means do a normal paint. This will have the benefit of drawing the background correctly (and should not mess up borders, since nsBCTableCellFrame does not paint borders anyway). Not sure what it would break; likely nothing that's not broken worse now. The drawback is even more jungle-code than we have now. 2) Rip out this two-pass nonsense and do this in some other way. The problem, of course is that from bottom to top the layers should be stacked as: 1) Table background 2) Table element (tr/td/etc) backgrounds 3) Table element borders 4) Content backgrounds/content borders Right now we paint layer 1, then we paint layer 2 in a "special" border paint pass handled by the table elements, then we paint layer 3, handled by the _table_ (normally the table elements would handle this, but not in border-collapse mode), then we paint layer 4 in our "normal" pass. For comparison, in border-collapse:separate mode, layers 2/3/4 are all done in one pass, with each table element painting its background and borders before painting its kids. I assume we can't paint backgrounds after borders because some bastard will do dashed borders and expect the backgrounds to be under them.... (note, btw, that a table cell with a view _would_ fuck up that testcase by painting over the border when it's painted directly....) The other option, of course, is to have the _kids_ still paint the borders, even in border-collapse:collapse mode. Not sure how easy this would be to do, but in the long run it may be the most sane option within our painting architecture... roc, thoughts?
Letting the childs paint their border contradicts the border-collapse mode as the border is shared between adjacent cells, so IMHO it will never be a sane option. More than this even if one could attribute a border to a specific cell, the border would draw into the adjacent cells. Imagine a cell where the bottom border is owned by the cell below. If a dirty rect covers only the upper part of the of the first cell then it will paint the background over the border from the cell below, causing a new dirty rectangle that incorporates the cell below....
Like I said, that would be hard (the kids would have to paint not what's in their style structs but what the table tells them to paint; global information about the borders would have to be stored on the table). In the short term, using two flags as I describe is probably sufficient for "most cases".....
>global information about the borders would have to be stored on the table it is already stored there http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/celldata.h#186 the table cellmap has a additional 32bit integer per cell to hold this data.
Ok... is the information being stored insufficient to properly paint the cell borders from inside the cell, then?
I think we could have the table cells call back to the table to paint their borders. We'd call a method like nsTableFrame::PaintCellBorders(aCell) which would either do normal border painting, or in BC mode, figure out the collapsed borders and draw them *within the rect for that cell only*. The only problem is it might be slower than drawing all the collapsed borders at once.
I am not certain that I would like to throw away all the performance optimized code that Chris has written and come back to a simple cell based solution just to solve a rather obscure testcase. If my memory is correct that performance optimization has consumed a large amount of Chris time. More than this I would like to keep this area pretty stable as fantasai is working on a fix for bug 4510 which fixes real pages. So my proposal is to postpone any work on this after bug 4510 is fixed. On the other hand the bc code went in on Feb 19 2002. moz 1.1 was released August 26, 2002, the fix for bug 162252 is one of the possible regression causes (please note that I have a very bad track on finger pointing, so caillon I apologize already now) Or in other words it would be good if we could narrow down the regression .
Well... I for one would rather support border-collapse slowly than incorrectly.... That said, I doubt that painting the borders in bits will be a big perf hit; I suspect that the actual border calculation is what Chris was trying to optimize. The current implementation is having an obvious clash with the view/painting system, and something has to give -- either the tables have to be good citizens in the paint model or the paint model has to be modified to not make some assumptions it currently makes (the key one being that calling Paint() on a frame will actually properly paint it as opposed to doing something random and wacky). I can narrow down the regression time tonight, if you would like.
I know the cause of this 'regression': it was my checkin to nsCSSFrameConstructor that added CreateViewForFrame in various places, including table cells, to create views for certain types of frames that needed them. Before that we never created a view for a table cell so the problem Boris describes did not arise. Note that before that checkin, 'background-attachment:fixed' did not work on table cells. Now it works correctly on non-BC tables but screws things up worse on BC-tables. We could easily disable table-cell view creation to get back to the old behavior for 1.3final. If you think this will crop up in more than a few real-world Web sites, then I think we should do that for 1.3final. It's better to be consistent with old bugs than to fix those bugs but introduce regressions elsewhere. There are various ways we could do a real fix in 1.4alpha. But I have a question: how should 'border-collapse' interact with cells which may be positioned and also have 'z-index'? Do they take their share of the border with them when they move or change z-order? I know we don't support positioned table elements yet but this is really a question for the CSS standards people. Fantasai? I think that whatever solution we choose, we'll probably want it so that painting table cells with the default paint flags always paints the cell borders, backgrounds and content correctly. If not, we'll eventually go mad. If we decide we need to optimize border painting performance then we should specifically ask for different behavior from the cell frame by passing flags into Paint() or by calling a different method altogether.
Any redesign of the border painting should take into account all the little problems with painting dashed and dotted borders. They're not anchored to a fixed point, so they get messed up when only part of the table paints. (That happens when scrolling, dragging a dialog box, etc.) The painting code also miscalculates the end of dotted lines sometimes... You can see all this in Part B of the tests for 4510. Btw, I get the impression that Chris was optimizing for memory space, not time. > Do they take their share of the border with them when they move or change > z-order? There's (IIRC) nothing in the spec about this. My reasoning: Absolute and fixed positioning and floats don't matter since they take the element out of normal flow; as I interpret it, that means they won't be in the table to have their borders collapse in the first place. So, only relative positioning affect anything. I think if I specify "position: relative" and don't change anything else, it should look the same as "position: static". (This is how it works in other cases, so it would be consistent.) That means the borders can't uncollapse. That leaves us with several options: - don't take any border - take half the border - take the whole border - duplicate the border I'm not very familiar with positioning and z-indexes, but I think the background of the table cell would overpaint the border if we don't take any border. I don't believe that's desired. Taking half the border would look very strange if, for example, - the table has thin, black solid borders and the cell has a thick green dashed one - the table has only thick column borders It also won't work very nicely with a 1px border. Duplicating the border would be confusing, particularly in the first example. Taking the whole border seems--to me, right now--to be the best solution. The main problem I see is when adjacent cells are both positioned; for that case I suggest both positioned cells take the full shared border. (Sorry for taking so long on 4510...)
I've just skimmed this bug, but a I've always thought that borders in border collapsing, logically, belong to the table and should be painted by it. (And thus relative positioning of a cell in a border-collapsed table should move the contents of the cell but leave the borders.)
Another (expensive) option is that when table cells in such a table need to be painted they should call the table to repaint all of itself.... (I don't like this one; just brainstorming).
dbaron -- Would td.special {position: relative; background: white} be visually equivalent to td.special {background: white} if the borders belong to the table?
No. I give up. But maybe it's better even though it's not.
Activate the CSS-WG bat-signal, Commissioner!
Personally I think the most logical thing is for the cell to take its share of the border. Yes, it could lead to odd-looking things, especially 1px borders where we'll end up dropping edges on low-res screens, but it probably has the most consistency we can muster in this situation. > I think if I specify "position: relative" and don't change anything else, it > should look the same as "position: static" I agree strongly with this principle. I'm not sure what Fantasai's "taking the whole border" means. Doesn't that mean that border area sticking out of the cell into other cells will be moved along with the cell? That's kind of weird. But actually things are worse: consider an rgba() border (or, equivalently, an 'opacity:0.5' cell). Then it seems two adjacent 'position:relative' cells which are not actually moved will overpaint their shared border, which will look visibly different from the rest of the table, violating the above principle. > Would td.special {position: relative; background: white} be visually > equivalent to td.special {background: white} if the borders belong to the > table? Actually, they could. We could draw all cell backgrounds (including offsets of 'position:relative' cells), draw collapsed borders ignoring relative offsets, and then draw cell contents (including offsets of 'position:relative' cells). With this approach, 'opacity' and other effects on the table cell would not affect the borders.
> consider an rgba() border (or, equivalently, an 'opacity:0.5' cell). Good point. > We could draw all cell backgrounds... affect the borders. Suppose my table is a 50px grid with 4px borders. I mark one cell class="special". .special { background: white; position: relative; left: 25px; top: 25px; } Do I really want the bottom right border intersection on top of the cell background? Not really.
Maybe you don't want that, but you can easily work around it by putting something else with a background inside the cell. I think any author using 'border-collapse' with relative positioning is playing with fire. If we give them something that's reasonably well specified and doesn't violate "common CSS principles", I think we're doing just fine. I'd be happy with either the table-based model I just described or with the cell taking its share of the borders with it. But the WG should be the ones to decide.
Bug 135236 is a powerful argument for making table cells render their own share of collapsed borders. We do NOT want to add all sorts of special-case logic to table border painting to account for all the effects that cells might be subject to (in that case, scrolling).
Personally, I think the cells should take their share of the border with them. In the case of a shared 1px border, the cell should take its 0.5px share of the border, and whatever we do in the pixel bug should decide what exactly that looks like. Making a cell opacity:50% should thus cause half its border to become transparent. IMHO, of course.
What about taking the whole border with it, but leaving it with the other cells too? In other words, each cell would have an entire copy of its 4 borders.
Priority: -- → P2
Target Milestone: --- → Future
So how do you propose to handle the rgba() border or 'opacity:0.5' cases?
Seems like this code is rife with problems -- see the various issues shown by: http://www.hixie.ch/tests/adhoc/css/box/table/border-collapse/collapse/evil/001-demo.html
Priority: P2 → --
Target Milestone: Future → ---
I agree that the cell should take its half of the border. > see the various issues shown by I think that the grey border should go around all of the table; the table's borders shouldn't lie over the div's. That's bug 155955. The problems with double borders derive from the beveling code. IIRC beveled corners are only triggered when two perpendicular borders meet, not four. The corner code definately needs improvement--but this should, again, be a separate bug. The strange sizing is based on the size of the thickest border on each size. It's seems as if the table size constraints are calculated first, and then the table is adjusted to take in the borders. I haven't looked at the code, though, so I don't know for sure. (There's no such problem with internal borders.) Also, borders greater than a certain size don't appear. The border disappears somewhere between 3em and 4em on Hixie's test case. (The div handles 8em just fine.) Looks like some bug filing needs to be done.. if anyone gets to it before I do, please CC me. Nice testcase, Ian. :)
there seems to be a Limit on the Border Width http://lxr.mozilla.org/seamonkey/search?string=LimitBorderWidth which was introduced to preserve memory :-)
Keywords: testcase
Priority: -- → P2
Target Milestone: --- → Future
Testcase works on my build, so that should be fixed with 4510. Borders will still cause problems, though.
Depends on: 4510
-> WORKSFORME
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → WORKSFORME
er, rather fixed by bug 4510. Worskforme is for when you don't know what fixed it....
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Status: REOPENED → RESOLVED
Closed: 21 years ago21 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: