Last Comment Bug 403526 - text-decoration should not be propagated to floats
: text-decoration should not be propagated to floats
Status: RESOLVED FIXED
: css2
Product: Core
Classification: Components
Component: Layout: Block and Inline (show other bugs)
: Trunk
: x86 Linux
: P2 normal with 2 votes (vote)
: ---
Assigned To: Zack Weinberg (:zwol)
:
Mentors:
: 202930 (view as bug list)
Depends on: 427845
Blocks: 403524 428599 491670
  Show dependency treegraph
 
Reported: 2007-11-12 12:11 PST by David Baron :dbaron: ⌚️UTC+1 (busy September 14-25)
Modified: 2011-02-11 11:05 PST (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
partial patch - fixes some but not all cases (1.20 KB, patch)
2008-04-08 17:17 PDT, Zack Weinberg (:zwol)
no flags Details | Diff | Splinter Review
more readable version of partial patch (1.44 KB, patch)
2008-04-08 17:18 PDT, Zack Weinberg (:zwol)
no flags Details | Diff | Splinter Review
screenshot, showing remaining problems (71.64 KB, image/png)
2008-04-08 17:22 PDT, Zack Weinberg (:zwol)
no flags Details
revised patch - code simplified, behavior should be identical (4.32 KB, patch)
2008-04-09 15:43 PDT, Zack Weinberg (:zwol)
no flags Details | Diff | Splinter Review
re-revised patch (13.73 KB, patch)
2008-04-11 16:05 PDT, Zack Weinberg (:zwol)
dbaron: review+
Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2007-11-12 12:11:41 PST
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.)
Comment 1 philippe (part-time) 2007-11-12 19:10:47 PST
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.
Comment 2 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2007-11-15 15:48:11 PST
To fix this, we'd probably want to fix the loop over ancestors in nsHTMLContainerFrame::GetTextDecorations .
Comment 3 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2007-11-15 15:49:36 PST
... 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.
Comment 4 Gauthier Van Vreckem 2008-03-13 14:59:25 PDT
IE8 does fix it
http://samples.msdn.microsoft.com/csstestpages/Chapter_16/text-decoration-010.htm
Comment 5 Gauthier Van Vreckem 2008-03-13 15:09:24 PDT
We also do propagate it incorrectly to inline-table:
http://samples.msdn.microsoft.com/csstestpages/Chapter_16/text-decoration-012.htm
and inline-block:
http://samples.msdn.microsoft.com/csstestpages/Chapter_16/text-decoration-013.htm
Comment 6 Zack Weinberg (:zwol) 2008-04-08 17:17:20 PDT
Created attachment 314463 [details] [diff] [review]
partial patch - fixes some but not all cases

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.
Comment 7 Zack Weinberg (:zwol) 2008-04-08 17:18:37 PDT
Created attachment 314464 [details] [diff] [review]
more readable version of partial patch

*sigh* let's try that again with diff -u.
Comment 8 Zack Weinberg (:zwol) 2008-04-08 17:22:53 PDT
Created attachment 314466 [details]
screenshot, showing remaining problems

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.
Comment 9 Zack Weinberg (:zwol) 2008-04-08 17:29:05 PDT
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?)
Comment 10 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2008-04-08 17:43:56 PDT
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.)
Comment 11 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2008-04-08 17:44:57 PDT
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. 
Comment 12 Zack Weinberg (:zwol) 2008-04-08 17:57:56 PDT
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.
Comment 13 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2008-04-08 18:18:38 PDT
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.
Comment 14 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2008-04-08 18:19:08 PDT
... and I suppose the inline-block check needs to be at the bottom as well.
Comment 15 Zack Weinberg (:zwol) 2008-04-08 18:54:16 PDT
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>
Comment 16 Zack Weinberg (:zwol) 2008-04-08 19:02:58 PDT
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.
Comment 17 Zack Weinberg (:zwol) 2008-04-09 15:43:37 PDT
Created attachment 314714 [details] [diff] [review]
revised patch - code simplified, behavior should be identical

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.
Comment 18 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2008-04-09 15:48:09 PDT
Mochitests are pretty unlikely to be relevant for a change to this code.
Comment 19 Zack Weinberg (:zwol) 2008-04-09 15:54:21 PDT
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 20 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2008-04-10 16:31:24 PDT
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.
Comment 21 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2008-04-11 10:42:43 PDT
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').
Comment 22 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2008-04-11 10:44:11 PDT
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.
Comment 23 Zack Weinberg (:zwol) 2008-04-11 16:05:47 PDT
Created attachment 315215 [details] [diff] [review]
re-revised patch

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.
Comment 24 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2008-04-29 10:52:00 PDT
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).
Comment 25 Zack Weinberg (:zwol) 2008-04-29 12:10:55 PDT
I was waiting for feedback from www-style before formally requesting review, but as there hasn't actually *been* any ...
Comment 26 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2008-08-06 09:52:45 PDT
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)
Comment 27 Zack Weinberg (:zwol) 2008-10-28 12:31:56 PDT
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.
Comment 28 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2009-11-17 22:00:26 PST
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 29 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2009-11-17 22:08:58 PST
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)
Comment 30 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2009-11-17 22:52:06 PST
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.
Comment 31 Zack Weinberg (:zwol) 2009-11-17 23:02:36 PST
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? :-)
Comment 32 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2009-11-18 00:07:45 PST
Sure.
Comment 33 Zack Weinberg (:zwol) 2009-11-18 08:37:46 PST
'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.
Comment 34 Max Kanat-Alexander 2010-03-16 19:32:42 PDT
(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.)
Comment 35 Zack Weinberg (:zwol) 2010-03-16 20:09:18 PDT
Yes, I totally forgot about this.  I'll put it in later tonight or tomorrow.
Comment 36 Zack Weinberg (:zwol) 2010-03-17 09:43:25 PDT
*** Bug 202930 has been marked as a duplicate of this bug. ***
Comment 37 Zack Weinberg (:zwol) 2010-03-17 11:21:13 PDT
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.
Comment 38 Boris Zbarsky [:bz] (TPAC) 2011-02-11 11:01:07 PST
David, is there a test for this in the CSS test suite?  Webkit and IE seem to fail this....
Comment 39 Boris Zbarsky [:bz] (TPAC) 2011-02-11 11:05:37 PST
Raising the question of whether it was in fact the spec that should have changed, of course.

Note You need to log in before you can comment on or make changes to this bug.