Closed Bug 191567 Opened 17 years ago Closed 16 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 slightly better
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.
-> WORKSFORME
Status: NEW → RESOLVED
Closed: 16 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: 16 years ago16 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.