Closed Bug 403526 Opened 17 years ago Closed 14 years ago

text-decoration should not be propagated to floats

Categories

(Core :: Layout: Block and Inline, defect, P2)

Product:
Core
Component:
Layout: Block and Inline
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: dbaron, Assigned: zwol)

References

Details

(Keywords: css2)

Attachments

(1 file, 4 obsolete files)

re-revised patch
13.73 KB, patch
dbaron
: review+
Details | Diff | Splinter Review 
http://www.w3.org/TR/CSS21/text.html#propdef-text-decoration says:

# It is not, however, further propagated to floating and absolutely positioned
# descendants, nor to the contents of 'inline-table' and 'inline-block'
# descendants.

We do propagate it to floating descendants.  This is causing layout/reftests/text-decoration/underline-block-propagation-standards.html to fail.

(If our current behavior is more compatible with other browsers than fixing this bug, then we should probably get the spec changed instead.  I haven't actually checked which is the case.)
As far as I can tell:
Opera 9.2 and 9.5beta, IE 5 Mac and iCab 3.03: text-decoration is _not_ propagated into a floated descendant of an inline-block.
WebKit latest and Safari 2.04, Konqueror (3.5.8): same behaviour as the latest Gecko trunk builds (propagated into floated descendants).
IE7 Windows XP: same as latest Gecko.
To fix this, we'd probably want to fix the loop over ancestors in nsHTMLContainerFrame::GetTextDecorations .
... although we're propagating only text-decorations on the containing-block chain to floats, not text-decorations on an inline wrapping the float.  That does seem a bit odd, since our notion of containing block for floats is barely even in the spec.
Depends on: 427845
I have a partial fix for this bug, which just copies the rule from the CSS2.1 spec into the code as directly as possible.  It works for floats and position:absolute, but it doesn't altogether work for inline-table and inline-block.  I will attach a screenshot showing the remaining problems.
Assignee: nobody → zweinberg
Status: NEW → ASSIGNED
*sigh* let's try that again with diff -u.
Attachment #314463 - Attachment is obsolete: true
Attached image screenshot, showing remaining problems (obsolete) — 
This is a screenshot of the rendering of layout/reftests/text-decoration/underline-block-propagation-standards.html by a browser with the patch applied.  Note that the patch in bug 427845 was also applied to the tree.

The remaining problem is that while an inline-block or inline-table SPAN now does not underline its own text nodes, the parent of that element does draw underlining at its own baseline across the space represented by the SPAN.  Per discussion in 427845, this should not be done.
To be extremely specific: in the line reading "This should be underlined but on any line", the words "on any line" and the space after that should not be underlined.  Similarly, on the line reading "This should be underlined but this should not", the words "this should not" should not be underlined.  However, the periods at the ends of both lines _should_ be underlined, and so should the space immediately before the words I've mentioned.

I think the place to fix this is nsHTMLContainerFrame::PaintTextDecorationLine, which currently just draws one line across its own bounding box (or so it appears) -- it would have to somehow segment the line so it does not extend across any children which are inline-table or inline-block (and are there any other cases?)
Fixing text-decoration in standards mode to only underline text should probably be a separate bug from this one, actually.

There are already a set of conditions for breaking at the top of the loop.  It seems like that condition should simply check for NS_STYLE_DISPLAY_BLOCK or NS_STYLE_DISPLAY_LIST_ITEM.  (It's currently allowing those two or NS_STYLE_DISPLAY_INLINE_BLOCK.)

Note that the part of the spec that it's matching is:
# 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.
    -- http://www.w3.org/TR/CSS21/text.html#lining-striking-props
so what we want to do is check for block-level in-flow descendants.  The spec defines block-level as pretty much what our IsBlockOutside does, so we could even use that.

Then, to check that it's in-flow, we could check (frame->GetStateBits() & NS_FRAME_OUT_OF_FLOW).  This would catch both floating and absolute positioning.

(It's actually a little unclear to me whether the spec is saying that it should get propagated into tables.  Read literally, it says that it's propagated to the table, but not further inside the table to the row groups, rows, or cells.  I think we may as well stick to that, since it minimizes change from the current behavior, unless other browsers do otherwise -- in which case we should comment on the spec.)
I probably should have quoted the next sentence in the spec as well:
# It is not, however, further propagated to floating and absolutely positioned
# descendants, nor to the contents of 'inline-table' and 'inline-block'
# descendants. 
Consider

<div style="text-decoration:underline">
<p>underlined</p>
<p style="float: left; text-decoration:line-through">struck out</p>
</div>

-- we agree that the word "underlined" should be underlined, and the words "struck out" struck out but not underlined, yes?  (This is what Opera does, and what Moz does with my patch.) I think that if we added the check for out-of-flow to the condition at the top of the loop, the text-decoration:line-through on the second P tag would get ignored altogether.

It seems like it ought to be possible to unify the two if statements that break out of the loop, but I don't fully understand what the conditional at the top means, so I didn't want to mess with it.
I think the condition at the top is implementing an earlier (or more ambiguous) version of exactly this rule in the spec.  I think you're right that the out-of-flow check needs to be at the bottom, though.
... and I suppose the inline-block check needs to be at the bottom as well.
We had a bunch of face-to-face conversation with the ultimate action item of
finding out what the compatible thing to do with text-decoration styles on
TABLE and TR tags was.

I no longer consider Safari an interesting reference, since it clearly has all
kinds of text-decoration bugs, but Opera (which has so far been 100% spec
compliant) honors text-decoration on both.  Here's my test case:

<!DOCTYPE HTML>
<html><head>
<title>text-decorations on table components</title>
</head>
<body>
<table style="text-decoration:underline">
  <tr><td>1<td>2<td>3</tr>
  <tr style="text-decoration:overline"><td>a<td>b<td
style="text-decoration:line-through">c</tr>
  <tr><td>do<td>re<td>mi</tr>
</table>
</body></html>
IE7 also propagates text-decoration from TABLE and TR to child TDs, although differently: in the above example, Opera draws all of the text-decorations that apply (so "c" is underlined, overlined, and struck through) but IE draws only the text-decoration for the nearest ancestor with the property (so "c" is just struck through, and "b" is just overlined).  I did not check whether IE7 also has the bugs that Safari has.
This revised patch has the same behavior as my first one (so the screenshot is still relevant) but the code has been simplified; especially note that the exit conditional at the top of the loop has been totally removed.  I have not changed the behavior wrt table cells.  There are no reftest regressions, I am trying mochitests now.
Attachment #314464 - Attachment is obsolete: true
Mochitests are pretty unlikely to be relevant for a change to this code.
I kinda thought not, but I wasn't sure.

Can you comment on the test at the bottom of the loop?  I didn't change that from the previous patch.  Yesterday you wanted that to be rather different, but the only case where I'm sure it makes a difference is <TABLE> to <TD> where the compatible thing seems to be to do it my way.  [I looked at CSS3 this morning and was not enlightened.]  I'd change my tune instantly if any reftests had broken or if you can think of another test case where it matters...
Comment on attachment 314714 [details] [diff] [review]
revised patch - code simplified, behavior should be identical

>+    // We want to ignore a text-decoration from an outer frame that is
>+    // redundant with one from an inner frame.  This isn't just an
>+    // optimization; the inner frame's color specification must win.
>+    // At any point in the loop below, this variable indicates which
>+    // decorations we are still paying attention to; it starts set to
>+    // all possible decorations.

Don't use the terms "inner frame" and "outer frame" here since we often use those to refer to matched parent/child pairs used in some cases to represent a single content node.  Use "descendant" and "ancestor" instead.

>+      // If all possible decorations have now been specified, no outer
>+      // frame can affect the rendering.  decorMask will be zero in
>+      // this case.
>+      if (!decorMask) {
>+        break;
>+      }

No need for the "decorMask will be zero in this case", since the code shows that.

>+      // CSS2.1 16.3.1 "...[This property] is not, however, propagated
>+      // to floating and absolutely positioned descendants, nor to the
>+      // contents of 'inline-table' and 'inline-block' descendants."  So
>+      // do not look at the parent frame if this frame is any of the above.

I think you should also quote the previous sentence, which says that it's only propagated through blocks...

>+      if (styleDisplay->IsFloating() ||
>+          styleDisplay->IsAbsolutelyPositioned() ||
>+          styleDisplay->mDisplay == NS_STYLE_DISPLAY_INLINE_BLOCK ||
>+          styleDisplay->mDisplay == NS_STYLE_DISPLAY_INLINE_TABLE) {
>+        break;
>+      }

...and, correspondingly, change this check to

+      if (styleDisplay->IsFloating() ||
+          styleDisplay->IsAbsolutelyPositioned() ||
+          (styleDisplay->mDisplay != NS_STYLE_DISPLAY_BLOCK &&
+           styleDisplay->mDisplay != NS_STYLE_DISPLAY_LIST_ITEM &&
+           styleDisplay->mDisplay != NS_STYLE_DISPLAY_TABLE_CAPTION &&
+           styleDisplay->mDisplay != NS_STYLE_DISPLAY_TABLE)) {

...which is a pretty minimal change.  TABLE and TABLE_CAPTION should really also be removed, but you could see that as a separate bug, or do it now.

I'd note that the old break condition was effectively

display != NS_STYLE_DISPLAY_BLOCK &&
display != NS_STYLE_DISPLAY_LIST_ITEM &&
display != NS_STYLE_DISPLAY_TABLE &&
display != NS_STYLE_DISPLAY_TABLE_CELL &&
display != NS_STYLE_DISPLAY_TABLE_CAPTION.

Table cell is now safe to drop since we're essentially breaking one step higher up the tree based on the same condition -- so in the old code we'd break on the table row; the new equivalent is a table cell.
So, given your comments on the spec and the behavior of other implementations, maybe your check for inline-block and inline-table is the way to go (i.e., checking == rather than !=), except that you should change it to check styleDisplay->IsInlineOutside(), which picks up a few more types (including, rather importantly, 'inline').
Note that picking up the check for 'inline' should be pretty important for 'text-decoration' not getting propagated into the interior of form controls (e.g., a textarea inside a block with underline shouldn't have all its text underlined) -- I think many of our form control implementations still are 'inline' (and replaced elements) even though they could be 'inline-block' -- the distinction is rather fuzzy for replaced elements.
Blocks: 428599
Attached patch re-revised patch — — Splinter Review 
This re-revised patch incorporates dbaron's comments to date and also adds some more test cases.

Also, I have split off the "remaining problems" as bug 428599 - dbaron thinks that might be a duplicate but I can't find an older bug that matches the description.

Note that if this patch were checked in as is it would also fix bug 202930 (which specifically asks for text-decorations to inherit from TABLE and TR to TD).  I have added a note to that effect to that bug, but I am not resolving that bug as a duplicate of this one, because it is still undecided whether we actually want to do that! (Waiting for feedback from www-style.)  I'm in favor of it though.
Attachment #314466 - Attachment is obsolete: true
Attachment #314714 - Attachment is obsolete: true
Comment on attachment 315215 [details] [diff] [review]
re-revised patch

I'm guessing that you want me to review this, so adding review requests to myself (although normally you'd want to do this).
Attachment #315215 - Flags: superreview?(dbaron)
Attachment #315215 - Flags: review?(dbaron)
I was waiting for feedback from www-style before formally requesting review, but as there hasn't actually *been* any ...
Flags: wanted1.9.1?
Priority: -- → P2
Flags: wanted1.9.1? → wanted1.9.1+
http://lists.w3.org/Archives/Public/www-style/2008Jul/0450.html is the proposal in front of the WG, although it hasn't been adopted yet (to be discussed next week)
AFAIK this is still blocked on the CSS WG, so I am taking it off the 1.9.1 wanted list.  If the WG has actually made a decision the patch will be easy to resurrect.
Flags: wanted1.9.1+
I think you should propagate text-decoration to all descendants except those that are:
 * floating
 * absolutely positioned (including fixed positioned)
 * inline-block
 * inline-table
I think that's basically what the spec says to do, although it's a bit unclear.

In our current standards-mode text-decoration handling, there would be a little more complexity since you'd additionally have to *not* draw a propagated decoration on an inline (but instead let it be drawn by either the nearest block or by the element on which it is specified, whichever is closer), but that should go away when we implement the spec's current model (bug 403524).
Comment on attachment 315215 [details] [diff] [review]
re-revised patch

In other words, I think this patch is good, and we should land it.  Sorry for taking so long to get to this.

r=dbaron (though I suspect this might need some updating)
Attachment #315215 - Flags: review?(dbaron) → review+
And there's a revised proposal in:
http://lists.w3.org/Archives/Public/www-style/2009Nov/0237.html (which I only just read) which I think comes out as exactly equivalent to what I was suggesting.
Wow, I thought this was never going to get resolved.

The patch applies to current trunk without modification, and the reftests pass.  Shall I go ahead and land it? :-)
'm gonna wait till next week rather than try to squeeze this under the wire of the "1.9.2 blockers only" freeze that was just announced.
(In reply to comment #33)
> 'm gonna wait till next week rather than try to squeeze this under the wire of
> the "1.9.2 blockers only" freeze that was just announced.

  It's been a few months, instead of a week--did this get forgotten? (I'm getting bit by bug 428599 in a patch I'm writing for Bugzilla itself, and this is blocking that fix.)
Yes, I totally forgot about this.  I'll put it in later tonight or tomorrow.
http://hg.mozilla.org/mozilla-central/rev/ce38556552e2

I am not likely to have time for bug 428599 or bug 403524 in the near future, though.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
David, is there a test for this in the CSS test suite?  Webkit and IE seem to fail this....
Raising the question of whether it was in fact the spec that should have changed, of course.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: