202930 - CSS text-decoration is computed correctly but not drawn on text inside TABLEs in Standards Compliance mode

Status: RESOLVED DUPLICATE of bug 403526

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

Description

[Markus Multrus]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4b) Gecko/20030422
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4b) Gecko/20030422

This occured somewhere before 1.3 final: If you now apply text-decoration:
line-through to the <tr>- tag, the text within the cells is not striked any
more. I don't know exactly, if this is the correct behaviour, but I can say that
e.g. Konquerer does strike the text

Reproducible: Always

Steps to Reproduce:

Comment 1

[Markus Multrus]

added link to testcase, cause I'm not able to create an attachment with my
firewall settings... feel free to attach it to this bug :-)

Comment 2

[Jo Hermans]

Shouldn't that be 
.striked td {text-decoration: line-through;}
to have a line in row 2 ?

Comment 3

[Markus Multrus]

Is this correct that there is no inheritance for <td> from <tr>? Is <td> not a
block box from another block box <td>? Then sould be inheritance... This goes
out to the freaks... :) 
http://www.w3.org/TR/REC-CSS2/text.html#lining-striking-props

Comment 4

[Marco Perez [:mpk]]

I can reproduce this on Win98.

According to the specs (http://www.w3.org/TR/REC-CSS2/text.html#q3) "This
property is not inherited, but descendant boxes of a block box should be
formatted with the same decoration...". Does this mean we have a valid bug here?

Checking the computed styles with DOM inspector reveals that text-decoration of
the cell in row 2 is set to "none", but this isn't defined in
resource:///res/html.css .
Specifying  td {text-decoration:inherit }  in the testcase may serve as a
workaround.

Comment 5

[Marco Perez [:mpk]]

Attachment: The reporter's original testcase

Just in case the test URL becomes invalid someday ;-)

Comment 6

[Boris Zbarsky [:bzbarsky]]

The spec sez:

  This property describes decorations that are added to the text of an element.
  If the property is specified for a block-level element, it affects all
  inline-level descendants of the element. If it is specified for (or affects)
  an inline-level element, it affects all boxes generated by the element. 

The use of "descendant" instead of "child" here makes me think that this should
indeed strike the text....

Confirming, over to tables; it sounds like tables override some Paint() method
and don't paint decorations correctly....

Comment 7

[Boris Zbarsky [:bzbarsky]]

That said, it's not clear what setting text-decoration on a row should really
do.... Should the table display types be considered block-level for purposes of
text-decoration?  I mean things like <tr> and <thead>.... At the moment, they
are not.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: sort of a testcase...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: One possible patch

Comment 10

[Hixie (not reading bugmail)]

(For extra amusement, note that CSS3 Text disagrees with CSS2.1.)

Comment 11

[Boris Zbarsky [:bzbarsky]]

So... reading CSS3 Text, a conforming impl would _not_ underline the text in:

<div style="text-decoration: underline">
  <p>Text</p>
</div>

correct?

If so, how likely are they to make it agree with CSS2?  ;)

Comment 12

[Madhur Bhatia]

see this on winXP too 

Comment 13

[Boris Zbarsky [:bzbarsky]]

ok, assigning to hixie to decide what the right behavior here is....

Comment 14

[Hixie (not reading bugmail)]

The "right behaviour" is whichever one you want at the moment. The specs are
most likely to be changed at the end of their respective CR periods to match
what the majority of UAs do.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Well, I personally have no particular preference, and no real time to put into
thinking about it (as in, I have time to implement a behavior, but not to design
one). So any help would be much appreciated.

Comment 16

[Hixie (not reading bugmail)]

I have no preference either. Either the root inline box picks up its text
decoration from its block only, or it crawls up to the root of its formatting
context adding text decorations that aren't yet set, or it crawls up to the
document's root element adding text decorations that aren't yet set. At the
moment I guess we are doing the second of those.

Comment 17

[Boris Zbarsky [:bzbarsky]]

At the moment, it crawls up till it hits an "inline" frame ancestor.  Whatever
"inline" means.  Our current definition is "NOT block, table cell, list item, or
table".  All that patch does is change the defition of "inline" in this context
to also exclue "table-row, table-header-group, table-footer-group,
table-row-group, table-caption".

Comment 18

[Hixie (not reading bugmail)]

Ok, that's two more possiblilities: Crawl up until you hit an inline; or crawl
up until you hit an inline or the top of the formatting context.

Comment 19

[Boris Zbarsky [:bzbarsky]]

What's the actual definition of "top of formatting context"?  Is it just any
root or out of flow frame?

Comment 20

[Hixie (not reading bugmail)]

Damn, I was hoping you wouldn't ask that. Usually if I use terms confidently
like that people just nod and smile. :-P

The term "formatting context" is one I made up. Roughly speaking, a new
formatting context is formed by table cells, absolutely positioned boxes,
non-visible 'overflow', floats, and the canvas.

(David has his own term for roughly the same thing, but they are not identical.)

Comment 21

[Boris Zbarsky [:bzbarsky]]

Ian, I'm in math, remember?  Getting the definitions wrong is never good...  ;)

So it seems that as long as we're doing the behavior we're doing, not treating
<tr> and <tbody> (and <caption>) as blocks is silly... So we should either take
that patch or change the whole approach.

Comment 22

[Hixie (not reading bugmail)]

Is there any reason we have to list everything but 'inline', rather than listing
just the things that should make us continue looking? I would guess that the
second list would be shorter and less prone to change. (I wouldn't want us to
have to change this code just because we add another display type.)

Comment 23

[Boris Zbarsky [:bzbarsky]]

Well, our list of display types is at:

http://lxr.mozilla.org/seamonkey/source/layout/base/public/nsStyleConsts.h#311

so things suck no matter what.... ;)  What we really need are functions on
nsStyleDisplay like "isBlockRole()" and "isBlockContainer()" or whatever
dbaron's classifications were...

Comment 24

[Hixie (not reading bugmail)]

Ugh.

Oh well. Pick a random subset of that list and go with it... ;-)

Comment 25

[Boris Zbarsky [:bzbarsky]]

*** Bug 213867 has been marked as a duplicate of this bug. ***

Comment 26

[Richard Brodie]

*** Bug 248999 has been marked as a duplicate of this bug. ***

Comment 27

[Greg K.]

Sounds like text-decoration in general is not being drawn. Updating Summary.
(Note that since this affects captions as well it is not limited to TRs or TBODYs.)

Comment 28

[Greg K.]

Whoops, bug 213867 is right, this only happens in Standards Compliance mode.

Comment 29

[Greg K.]

Reproduced using Camino 0.8. Setting Hardware=All. Also, this probably belongs
on GFX, not Layout: Tables. (The style is being computed correctly, but not drawn.)

Comment 30

[Boris Zbarsky [:bzbarsky]]

This is an issue in layout code, not GFX code.  I'm not likely to have time to
look at this any time soon, but I'll review patches....

Comment 31

[Mathieu Fenniak]

Attachment: updated patch

Attaching updated patch with current location of nsHTMLContainerFrame.cpp, but no code changes.

This patch does fix the problem, text-decoration filters through tables as one would expect from the CSS2 spec, and the behavior after the patch matches IE as well.

I've run this patch against the test-decoration test cases in bug 1777 and can confirm that there are no side-effects on any of the complex test cases there.  They all display identically after this patch, no changes.

Any chance this could move forward and be committed?

Comment 32

[Boris Zbarsky [:bzbarsky]]

It's note clear that this patch gives the right behavior.  Until that happens...

Comment 33

[Boris Zbarsky [:bzbarsky]]

Also note that now we do have IsBlockInside() and IsBlockOutside(), but that nether one includes internal table elements.

Also note that it's not clear how this should interact with inline-block and inline-table.

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Note that, in general, the first step for getting a contributed patch incorporated is to request review on it.  This is done by setting the review flag (either when creating the attachment, or in the "Details" page for it) to "?" and putting in the email address of the reviewer (or a unique substring of their email or username).  In this case, Boris or I would probably be appropriate reviewers.

(In reply to comment #33)
> Also note that it's not clear how this should interact with inline-block and
> inline-table.

See also bug 371249.


What does the CSS2.1 spec say the correct behavior is?

Comment 35

[Boris Zbarsky [:bzbarsky]]

> It's note clear that this patch gives the right behavior.

I meant "It's NOT clear".

Comment 36

[Martijn Wargers (dead)]

From: http://www.w3.org/TR/CSS21/text.html#q3
"
When specified on an inline element, it affects all the boxes generated by that element; for all other elements, the decorations are propagated to an anonymous inline box that wraps all the in-flow inline children of the element, and to any block-level in-flow descendants. It is not, however, further propagated to floating and absolutely positioned descendants, nor to the contents of 'inline-table' and 'inline-block' descendants. 
"

Definition of a block-level in-flow descendant is here:
http://www.w3.org/TR/CSS21/visuren.html#block-level
"
Block-level elements are those elements of the source document that are formatted visually as blocks (e.g., paragraphs). Several values of the 'display' property make an element block-level: 'block', 'list-item', and 'run-in' (part of the time; see run-in boxes), and 'table'. 
"
So it seems to me all table display values are considered block-level.

Comment 39

[Zack Weinberg (:zwol)]

The current candidate patch for bug 403526 would also fix this bug in the manner requested by the original reporter.  As it's still undecided whether we actually want that behavior (though I am in favor of it) I am not going to do anything with this bug yet, other than add this note.

Comment 40

[Zack Weinberg (:zwol)]

Attachment: complete minimized testcase for CSSWG

This is now a css2.1 open issue; see <http://csswg.inkedblade.net/spec/css2.1#issue-56>.  fantasai requested a complete test case.

Comment 41

[Bernd]

the CSSWG needs a better proposal for the spec

Comment 42

[Zack Weinberg (:zwol)]

The CSS2.1 issues list has not been updated, but 
http://lists.w3.org/Archives/Public/www-style/2009Nov/0237.html is the current proposal, I'm informed that it has been approved, and I am going to land the patch in bug 403526 that implements it.