Dec3a Seamonkey build on NT4.0/SP3 1. Create HTML page with this test case for body content: <u>underline<b>bold</b></u> • "bold" underline is actually made bold as well, when in fact it should not be affected by the bold tags. Compared to IE4 and reference for baseline: When the font size or font changes within an underlined section, the underlining should stay at a constant vertical level: http://www.w3.org/Style/CSS/Test/current/sec13.htm

The problem behind this bug is the same as the problem, shown in http://www.w3.org/Style/CSS/Test/current/sec543.htm , that text-decoration *should* span children that have text-decoration set to none, since they are still within the element that has the text decoration.

Setting all current Open/Normal to M4.

per leger, assigning QA contacts to all open bugs without QA contacts according to list at http://bugzilla.mozilla.org/describecomponents.cgi?product=Browser

Given the following CSS reformulation of the above example: <span style="text-decoration:underline"> Outer <span style="font-weight:bold;"> Inner </span> Outer </span> The underlining should be the same throughout, as it is the underlining of the outer span that is being applied to the inner span, and not the inner span that is underlining itself. That is, the inner span should have *no* effect on the rendering of the underlines. The following test case explains everything in detail: http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/underline.html One way of looking at this is through inline boxes. The above generates two inline boxes, one for the outer span and one for the inner span. The inner inline box may overlap the outer inline box (in this case they are perfectly aligned): +-------+========+-------+ | Hello | Lovely | World | +-------+========+-------+ The underlining should (as I understand it) be applied *to the outer inline box* only. That is why, for example, the colour is the same throughout an underlining. Another way of testing this: if you made the inner span invisible (using the visibility property), the underlining should remain.

See also bug 3488.

See also bug 1519.

Sort of a lazy, easy testcase to write, but it's written. The file is basically a copy of the comments written in, including CSS and HTML versions.

Here are the four test cases mentioned so far, in order of complexity (most thorough test case first): http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/underline.html http://www.w3.org/Style/CSS/Test/current/sec543.htm http://bugzilla.mozilla.org/showattachment.cgi?attach_id=679 http://www.w3.org/Style/CSS/Test/current/sec13.htm The problem is that underlining is being applied to children. It should not. Underlining should be applied to the inline box which has "text-decoration" set. It should be drawn a fixed offset (relative to the font size) from the baseline of that element, without regard to any of the children.

I believe that implementing underline this way will lead to a large number of compatability problems. See the new attached test case. Comments please...

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

When images and text are mixed up in links, legacy behaviour has been to break the underlining under the images. This is ugly. :-) The only case where there may be an important compat issue is when <a> elements _only_ contain <img> elements [and possibly whitespace]. *In compat mode*, you could put in code to work around this, if you really want to. I'm not convinced that the problem will ever occur, since composite images with links are usually sites where the Nav4 vs CSS2 line box compat issue rears its head, and so underlining would get hidden behind the images _anyhow_. We could always use some fancy CSS3-like syntax in compat-ua.css: :link:not-matches(IMG:only-node), :visited:not-matches(IMG:only-node) { text-decoration: underline; } This would (given my various CSS proposals...) make all links that don't ONLY contain images underlined.

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

Please disregard the previous comment, I was talking nonsense. According to the SPEC, inline elements containing no text should NOT be underlined -- from CSS2: # 16.3.1 Underlining, overlining, striking, and blinking: the # 'text-decoration' property # ... # If the element has no content or no text content ... user agents must # ignore this property. So this will not cause backwards-comptability problems anything like as serious as I previously feared, and no CSS hacks will need to be used. See also this Mozilla-specific test case: http://www.bath.ac.uk/%7Epy8ieh/internet/projects/mozilla/underline-img.html

Updating to default Layout Assignee...kipp no longer with us :-(

Why are you re-reassing layout bugs? Do NOT touch layout bugs. The bugs are assigned to Kipp so they can stay neatly organized until we have a new owner for the block/inline code.

Bug 20163 has been opened and now tracks the style system side of this issue, namely that the 'text-decoration' property should not apply to text-less elements. This bug can now track just the rendering issue. To summarize: We should be applying the *-lining to the element which has 'text-decoration' set, by drawing a line at a fixed offset from the baseline (e.g. -0.01em for underline, +0.5em for strike-out, and +1.0em for overline), and not doing anything special individually for the children. So, for example, in the case of the following fragment: <span style="text-decoration:overline" id="a"> Outer <span style="font-weight:bold;" id="b"> Inner </span> Outer </span> ...the element 'a' should render the overline: ___________________ Outer Outer ...and the element 'b' should merely render its text: Inner ...which will result in: ___________________ Outer Inner Outer Thus the overline above 'Inner' is not any bolder than the rest. Similarly for if this fragment: <span style="text-decoration:overline" id="a"> Outer <span style="vertical-align: 1em;" id="b"> Inner </span> Outer </span> ...the element 'a' should render the overline: ___________________ Outer Outer ...and the element 'b' should merely render its text: Inner ...which will result in: ___________________ Outer Outer Inner Thus the overline is not broken where the child has moved. Adding dependency on 20163; that bug must be fixed before this bug can be considered totally fixed. Note, though, that bug 20163 is not required to fix the rendering side of this bug as documented above.

Migrating from {css1} to css1 keyword. The {css1}, {css2}, {css3} and {css-moz} radars should now be considered deprecated in favour of keywords. I am *really* sorry about the spam...

Bulk moving [testcase] code to new testcase keyword. Sorry for the spam!

This is incorrect. Text-decoration affects inline descendants with the characteristics of the ancestor. It is _not_ the case that it should span the descendants. For example, given SPAN.insideP {font-size: 2em} with P {text-decoration: underline}, the SPAN is underlined at the appropriate place for the SPAN but at the thickness, colour, etc., implied by the P. This bug is not a bug at all. It is undesirable and results in ugly results to strain the words of the spec to mean that text-decoration should span rather than 'affect' (which is what the spec says). I see no reason to change Mozilla to conform with something that the spec does not say and that results in ugly results. Note that this bug is not entirely INVALID because the original report was correct (that the underline should not be bold), it is just subsequent report stating that it should span rather than affect descendants that are wrong.

IMO, the spec is quite clear on this, and the above comments are wrong. CSS2 section 16.3.1, http://www.w3.org/TR/REC-CSS2/text.html#lining-striking-props , says that text-decorations are drawn on all *boxes* generated by an inline element. The boxes horizontally contain the children, so it spans children. I don't like this part of the spec, but that's what it says. I do agree that the results can be ugly. Perhaps there should be an attempt to change the spec?

mine! mine mine mine! all mine! whoo-hoo!

Here is the initial batch of layout [TEXT] bugs. I'll search for more today, but this should keep you busy for a while! Please review these for possible dogfood or nsbeta2 candidates, and assign your own priority and milestone for each. The current settings were relative to my bug list, and they might not make sense any more.

Nom. nsbeta2, recc. nsbeta2 6/15, falling through to nsbeta3+ if we miss b2. CSS1 compliance is key b2 criterion, *plus* the problem shows up on the official W3C CSS1 test suite.

Putting on [nsbeta2+] [will be minus on 6/15] radar.

Nominating nsbeta2,nsbeta3,rtm. Recc. nsbeta2+, falling through to nsbeta3+ and rtm+ if necessary. This is a W3C CSS1 Official Test Suite compliance bug.

Sorry, to clarify: this is not an nsbeta2 stopper, but I'd permit checking in prior to nsbeta2 if we have a fix. Recc. nsbeta2+[some lenient date-], falling through to nsbeta3+ hard stop if not fixed during nsbeta2. This is a W3C CSS1 Official Test Suite compliance bug.

PDT: please leave nsbeta2+. Standards compliance issue.

Sorry, I was going to reference ekrock's 2000-06-14 19:08 comments...

6/15 has passed... changing to nsbeta2 minus

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

As per meeting with ChrisD yesterday, taking QA.

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

nsbeta3+ P3 per bug meeting (ekrock)

Adding buster to cc: list.

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

When is the going to be fixed???

I've recently become aware that my last comment was rude. It was not meant to be so. I understand that a lot of the people working on this project are doing so for nothing. As such we should not expect these developers to do anything, and what they do we should be quite thankful for. P.S. I'm interested in trying to fix this bug myself. I can program, but I'm quite new to Mozilla. What section of the code should I start looking at? Thanks

At a guess, |nsTextFrame::PaintTextDecorations| in .../layout/html/base/src/nsTextFrame.cpp ...sounds like a good place to start (around lines 1516). buster? erik? dbaron? waterson? Anyone want to help here? Heejaf: Make sure you read _all_ the comments above, along with everything in all the linked files, and all the relevant documentation on mozilla.org. You'll probably save yourself a lot of pain! ;-)

David Baron wrote: >I don't like this part of the spec, but that's what it says. I do agree that >the results can be ugly. Perhaps there should be an attempt to change the spec? Perhaps we should resolve this question before deciding to make Mozilla produce ugly results. Comments?

Well, it seems there's three parts to that problem. 1. Given that Ian Hickson's eviltest says that his interpretation `generates much nicer looking underlines' (and I agree with that), what is David Baron's evidence for saying that `the results can be ugly' in real-world typography? 2. Which problem would Web authors be more likely to come up against on the Web (assuming an implementation which somehow managed to get it wrong both ways): the ugliness which Ian's interpretation solves, or the ugliness which David referred to? 3. Do you really think you can get the CSS spec changed before nsbeta3 ships, or is it better to just implement CSS1 first and ask questions later?

I retract my previous comments about the spec being wrong. I was considering an edge case that the WinIE/Nav4 version made better and note some other edge cases that the CSS spec's version made better. I think this should be fixed.

This bug has been marked [nsbeta3-] because the engineer it is assigned to is overburdened. Do not clear [nsbeta3-] unless you are reassigning the bug to yourself and committing to deliver a fix within the nsbeta3 timeframe. There is a workaround: use SPANs and set desired text-decoration problems on those rather than relying on the parent and inheritance. Future. relnote. helpwanted. Though it is unfortunate that it was not possible to fix this bug for the first release, the user and developer impact is minimal (even with the bug displayed, the text is fully readable, just not quite as elegant as desired) and folks can either work around the bug or write their content ignoring it and live with the visual display of the bug until this is fixed; we'll shoot for fixing in an early post-RTM release.

ekrock: Just for the record, there is *not* a workaround for this. The work- around you propose would actually make the problem worse. BTW, MacIE5 does this correctly.

Testing on 11_06 branch builds, Linux and Mac look okay. Underlining is consistent and not bolded.

Still seeing this on Windows 98 with the Nov 9 daily build. What could have changed in the Linux and Mac builds?

Chris: Linux and Mac are still broken in my tests -- see http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/underline.html ...for a comprehensive test suite.

Will this be fixed for NS6 1.0? I just don't see how everyone can be claiming such great standards support when Mozilla is not even passing the standard CSS1 test suite.

No. And Mozilla is not Netscape6 so the claims that Mozilla will be complaint are not affected by Netscape6's standards support.

Thanks for the information

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

BTW, per the SF F2F meeting, the working group agree that our interpretation is correct. (This will be made explicit in CSS3.)

This bug is high on my most-wanted list. Regarding to Hixie's last comment, does "the working group agree that our interpretation is correct" mean that this will be fixed, or that the current behaviour is correct?

It means that the working group agree that this bug is valid and that we are currently acting incorrectly. Now, from that to seeing a fix... ;-)

erik resign. reassign all his bug to ftang for now.

assigned as future

FYI Opera 5 actually passes the relevant test page perfectly now (well, except for a minor problem they have with images). So that's two browsers that get this almsot right (MacIE and Opera). [Hixie-B] in the status whiteboard means that I'll buy whoever fixes this a meal... :-) ftang: is this really your area? (line layout and inline frames)

Taking.

I rewrote text decoration on my style branch. I've divorced the decorations from the font object completely, so this bug should be easier to fix after I land 78695.

hyatt: Is there a chance of you getting to this in post-bug 78695 work? There's a dinner on Hixie in it for you, it would seem - and remember, he comes back to the UK in mid-August. ;-) Gerv

is this still a highrisk?

I can have a try at this CSS1 bug if the dinner is swapped for a fix to the much neglected MathML bug 39411 in m0.9.8 :-( What makes this bug tricky is how to handle textless frames. Suppose that the container inline frame has to draw the decorations, would it to have to grovel its subtree to filter out deeper img frames & etc? Not only is this not cheap and even then, text decorations data (e.g., the width of the invisible pieces) would have to be respected in the container's coordinate system and such. At first thought, it seems to me that looking at this problem from the perspective of the container frame offers no guarantee of an easier solution either. BTW, I tried the following in Opera and the result is counter-intuitive (or "ugly" -- to borrow the terminology in earlier comments): <P style="text-decoration: overline"><font size="+4">this should</font> be underlined <B>this should be underlined and bold</B></P>

Looks more like strike-through than overline to me

That is the expected result. I didn't completely follow what you said about textless frames. If the frame with "text-decoration" set happens to contain another frame with no text, that should not affect the first frame at all, it should just draw the line regardless. (Note, though, that if the element contains no text whatsoever, then no text decoration should be drawn at all -- see bug 20163.) The reason I have been suggesting looking at this from the container frame's point of view (i.e. the frame on which text-decoration is set) is that if you have a case like this: span { font-size: larger; text-decoration: overline; } <span><span><span> Hello </span></span></span> ...the word "Hello" should have three overlines, something like: -------------------- -|---|-----|-|------ -|---|-----|-|------ +---+ .-, | | .--, | | +-' | | | | | | \_/ | | \__/ Since I don't know the code, though, I may be wrong about this being the easier way to implement it.

What I meant is what you described -- with the additional hint to cases where an inner <span> can lead to an <img> that could have been (arbirtraly) positioned with other CSS properties (e.g., &nbsp;, spacing, margin, etc). For illustration, the rendering above is from Opera (BTW, note the buggy trailing lines on the textless portion after the <img>). It corresponds to the following markup: span { font-size: larger; text-decoration: overline; } Start <span> <span> <span> Hello ^{<span> Hi <img src="http://www.w3.org/Style/CSS/Test/current/oransqr.gif"> </span>} </span> </span> </span> End

There is another bug in that Opera rendering: the top overline disappears above the image but it should not; it should carry on. Other than that, it's a very good rendering.

>the top overline disappears above the image but it should not Wouldn't this create iconsistencies? I changed the orange square to a transparent square and it just left a white gap with no overlines (from the containing spans) passing through.

The lines should go through regardless of whether there are child elements containing images, with visibility set to 'hidden', or whatever. See http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/underline.html

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

Comment from dup bug 121760 regarding Top 100 site: http://www.slashdot.org. Open the page. On the page there are major sections headed in white text with a green background. Some of these headings have links - for example, the first one that states 'Developers'. The code shows that this text is bolded by the <B> element. As the text is bolded, the anchor line underneath the text is also getting bolded. In this case, it looks bad because it's hard to tell that it is a link since the background color and the underlined link are both white. I have attached a simplified testcase to demonstrate. Compare to IE which renders the bolded text only. Attaching a simplified testcase to demonstrate.

Is this really a futured bug?

This shouldn't be hard to fix to get Hixie's desired behaviour, it can all be done in the inline container frames.

Ugh. nsTextFrame::PaintTextDecorations implements a <FONT> quirk where the colour specified in <FONT> overrides the decoration colour specified in the decorated element. Implementing THAT while painting the decoration in the container is going to be a pain. Also, there are some tricky issues with "paint decoration only if there is some text in the element" and dynamic changes. If text children are added to a decorated element then we may need to repaint the entire element. Also, the way decoration is applied to block elements seems weird. The spec says all "inline descendants" are affected. If that includes all indirect inline descendants then the effect would be very weird indeed. If it only includes direct inline descendants, what about blocks that contain other blocks? I suggest we should search down through the frame tree for each inline frame F which has only block frames between it and the decorated block frame. So, given the following frame tree: block0 block1 { text-decoration: underline; } block2 inlineA inlineB We would paint the decoration for inlineA only, using block1's colour. Another question: given <a href="..." style="text-decoration: underline"><img src="..."> </a> doesn't the A element actually contain text --- a single space? The spec doesn't say anything about the text not being whitespace.

Not sure what to do about the <font> quirks. FWIW, if you want to limit them to quirks mode, that's fine. > The spec says all "inline descendants" are affected. Ouch. I have no idea how to interpret that in CSS terms. (I tried describing it in terms of implied anonymous inline boxes, but that didn't explain why the colour was from the block setting text-decoration.) What do you think should happen in this case?: block0 { text-decoration: underline; color: rgba(255,0,0, 0.5); } block1 { text-decoration: underline; color: rgba(0,255,0, 0.5); } block2 { text-decoration: underline; color: rgba(0,0,255, 0.5); } inlineA inlineB Regarding the empty space thing: if white-space is set to 'pre' then spaces are significant in layout, otherwise they are not.

My guess is that in your example we should paint 3 composited underlines, but you're the W3C guy :-). Perhaps the WG could clarify all these issues in the CSS3 WD? > Regarding the empty space thing: if white-space is set to 'pre' then spaces > are significant in layout, otherwise they are not. I don't understand this. Even when whitespace is not 'pre', spaces are significant, otherwise we wouldn't break lines :-). I don't still don't see why in my example <a href="..." style="text-decoration: underline"><img src="..."> </a> we can say there is "no text" inside the A element. There is text there in the DOM. There is whitespace there for the purposes of line breaking.

Would the argument "whitespace isn't text" hold water? :-) Whitespace in the case you gave isn't text because it can collapse away in the rendering (e.g. if there is a space on the other side of the end tag). Also, when would an author ever _want_ the image to be underlined based on the presence of a space character? The only time I see the author caring about that space is when he explicitly says that the 'white-space' should be 'pre'served. Regarding the block issue, I've raised it in the WG.

OK, I concede that "whitespace isn't really text" is one plausible interpretation of the language in the recommendation, but I respectfully suggest that it is not the only possible interpretation. It would be great if you could ask the WG to clarify that too.

I actually raised that issue as well in my previous mail to the WG. :-) I completely agree that the spec is vague in this respect.

Ok, the WG's internal draft for text-decoration currently reads as follows: # These properties describe decorations that are added to the text of an # element. If they are specified for a block-level element, it affects the root # inline element (the anonymous inline element which wraps all the inline # children of the block element). # # If they are specified for (or affects) an inline-level element, it affects all # boxes generated by the element. If an element is empty or is a replaced # element (e.g., the IMG element in XHTML), user agents must ignore these # properties. Text content also excludes white space characters that are # collapsed during the white space processing. So: block0 { text-decoration: underline; color: rgba(255,0,0, 0.5); } block1 { text-decoration: underline; color: rgba(0,255,0, 0.5); } block2 { text-decoration: underline; color: rgba(0,0,255, 0.5); } <anon1: anonymous inline wrapping inline contents of block2> inlineA inlineB inlineA and inlineB have no text-decoration of their own, and anon1 has one underline, that of block2. Similarly: block0 block1 { text-decoration: underline; } block2 <anon1: anonymous inline wrapping inline contents of block2> inlineA inlineB block1's underline propagates to anon1. The colour comes from block2, since the propagated value of text-underline-color is 'auto' and that means 'take it from the 'color' property' which is inherited from block2 to anon1. Another example: block0 { text-decoration: underline; color: rgba(255,0,0, 0.5); } block1 { text-decoration: underline; color: rgba(0,255,0, 0.5); } block2 { text-decoration: underline; color: rgba(0,0,255, 0.5); } inlineA block3 { color: rgba(0,128,128, 0.5); } inlineB InlineA would have one set of underline (solid auto continuous), using the colour of block2 (inherited from the block to the anonymous inline and then used by the underline because of the 'auto'), and inlineB would have the same style underline, but using a different colour (inherited from block 3 and then used in the same way). Finally, same tree but different style rules: * { text-underline: solid rgba(0, 0, 0, 0.2); } InlineA's text would have two sets of underline, one from block2 propagating the underline to its anonymous inline, and one on inlineA itself. Similarly, inlineB would have the underline from block3 and from itself. Note that inlineA wouldn't affect inlineB because the anonymous inline box would get its underline from block3 -- if block3 had had no underline, it would instead have taken its underline style from inlineA and inlineB's text would still have two underlines. Looking at this from the point of view of root inline boxes: their computed value of text-underline-* would be the text-underline-* of the nearest ancestor whose text-underline-style is not none, and similarly for overline, strike- through and blink.

I'm having the beginning to a fix for this one... (in one special case, but I believe that the general case will be similar.)... may I take over the bug, please? Or do I leave it with hyatt@netscape.com, and just post a patch when I have a patch ready? (It will probably be 7 or 14 days, depending on how much time I have, but that's just guesswork.)

Go fast, 1.0 is trying to wrap up RC2 in a week or so. RC2 might be 1.0, so don't count on getting this into 1.0 if you don't get into RC2. /be

I don't think this would get approved for 1.0, it's not important enough. Let us know how you're planning to fix this so we can give you feedback.

Here my 5€ ;-) The only problem with waiting for 1.0 with this one is that mozilla arguable isn't fully CSS1 compliant before this, among other things are fixed. But it don't think it would be wise, anyhow. No browser out there is, as far as I know... The fix isn't hard: The idea is to move the responsibility of textdecoratios up to the containing frame. This is complicated a bit by the fact that this seems to be two different frame types: nsHTMLContainerFrame for inline types and (I think) nsBlockFrame for block types. I've only dealt with the former, yet. I've simply moved up the painting of the textdecorations to nsHTMLContainerFrame's Paint() function. This and /not/ going up the tree to find fixes the bug in some cases, like in <U>some <B>big</B> text, with the style rule B { font-size: 200% } So that there's one, simple underline under the whole thing. I'm using the font properties of the nsHTMLContainerFrame, and this seems to work nice, though I was only guessing as to what font to use. I've left the painting of the selection with the nsTextFrame, where I thought it belonged anyway. BTW: I don't wan't two pieces of basically identical code in nsHTMLContainerFrame.cpp and nsBlockFrame.cpp... and I could create a static function that could do the work, but the I'd have to put it somewhere. Or, I could make it a memberfunction of nsFrame, which is a common ancestor, but I'm not sure that polluting that is a good idea either. A bit of advice in this regard would be much appreciated :o) In the hope that I'm actually doing more good than harm :o) Esben

Oops, forget my rambling about nsBlockFrame and nsHTMLContainerFrame. I see that the former is derived from the latter. Must've been blind :-]. I'll just place the neccessary rendering in nsHTMLContainerFrame. I'll press on; I'm pretty happy with HTMLContainerFrame's rendering of the textdecorations now. Now I'm working on nsBlockFrame, which is a litte more difficult, as I have to look at the lines in the frame, I think. rbs@maths.uq.edu.au: If you still have the testcases from which the Opera-images are derived, I'd dearly like to get my dirty hands on them :O) P.S: Could somebody please tell me if I should grap the bug, or leave it with hyatt@netscape.com?

Please read all of the above comments especially my comment #77. Moving all the painting up to the container is OK but that alone is not enough to make the FONT quirk work. Please also read Ian's comments about the action of text-decoration set on containing blocks and make sure that what you're planning will do the right thing for those examples. Thanks!!!

So this is my very first mozilla patch! :-D The patch is purely a rendering patch of bug1777, which, IMHO, is suboptimal. I will perhaps take a stab at migrating some of the work to the parser, which would be the best place, IMHO.

I haven't looked at the patch in any detail yet, but: * Could you describe exactly what you consider the correct behavior to be. It would be useful to know what you're implementing. There's a bunch of code in the style contexts dealing with text decorations that could probably be removed. Will the behavior depend on standards/quirks mode? Should it? (Will we break pages if it doesn't?) * This definitely should *not* be in the parser. * I think this patch would have bad effects on paint layers -- in particular, given <a href=" ...">foo<img style="vertical-align: top" />bar</a>, I would expect the image to paint on top of the decoration, and likewise for form controls. Thus I would think you would want the painting code to be in nsInlineFrame rather than nsBlockFrame, and do it before painting children. Others might disagree with me on this, though. * You should *not* be manipulating the font object that you get out of a style context.

Any implementation should pass these tests: http://www.w3.org/Style/CSS/Test/current/sec13.htm http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/underline.html I need to write some tests for the issues mentioned in comment 83.

The testcase I've used under development, in addition to those supplied in this bug. See the comment below.

As to whether the painting of this should be in nsInlineFrame or nsBlockFrame as suggested by dbaron@fas.harvard.edu, this is immaterial. nsInlineFrame inherits from nsHTMLContainerFrame, and has no Paint() implementation of it's own. In other words, I've already done as dbaron@fas.harvard.edu suggested. BTW: the testcase supplied renders as expected, that is, the line goes through the image, whether rendering by nsBlockFrame (that is, remove the <A>'s) or nsHTMLContainerFrame (use the text case as is). Concerning the font object: See above; I've stolen the code. If the code is bugridden, I'll try figure it out. (I'm not too sure where exactly I stole it; I believe it was from nsTextFrame somewhere....) I apologize for spelling mistakes etc. I'm too tired to read this through more than once. Ian 'Hixie' Hickson: I've already verified against these testcase. Unless I've missed something, it passes with, excuse the pun, flying color. Well, except the blink doesn't work, but there's a seperate bug on that. On a lighter node: I see Hixie-B is still in the whiteboard --- do this I get a dinner? ;-) I know several nice and not-too-expensive places right here in Copenhagen >:-}

I see today that my comment detailing the implementation was lost, somehow :-( So, somewhat briefer, it goes like this: In the Paint() methods in nsHTMLContainerFrame and nsBlockFrame, PaintTextDecorations() is called if the frame is a inlineframe, or if the frame contains lines, respectively. PaintTextDecoration immediately calls GetTextDecorations() which determines which text-decorations should be painted, like this: First, starting with the current frame and walking up the tree, the frames are tested for text-decoration styles. The first time one is found, the color is grapped and this text decoration type (e.g. underline) is marked as "we've got it.", so that we don't grab it again. This is exactly as it was done before. The difference is that now it stops processing if going up the tree would get a non-inline frame. See comment 77 and 83 below as to why this should be so. We're not done with GetTextDecorations() yet! In order to avoid making text decorations for text-less frames (see comment 16,77, 83 and others), we have to do something expensive: check *all* the descendants of the current frame until we find a textframe(!). If none are found, the textdecorations are reset to NS_STYLE_TEXT_DECORATION_NONE aka none. If any text-decoration were found above, we need some font info, a rather, a nsIFontMetrics object. I stole this code from nsTextFrame.cpp somewhere, so it should be no worse than what's already out there :O) Anyways, the font belonging to the current frame is used for calculation of ascent and offsets. Then, for the painting, a virtual function is used to distinguish between nsHTMLContainerFrame and nsBlockFrame. The former is very straightforward, but for the latter we need to paint all lines marked as "inline." In order to do this, I need the distance from the top of the line to the baseline /of the font initially used/ in the frame. This value was apparantly calculated in nsLineLayout.cpp, but never stored anywhere. I've changed this by adding the value to nsLineBox, include a pair of Get/SetAscent(). (I called it ascent in want of a better name.) From here on, it's very simple. dbaron@fas.harvard.edu:

Looking at the testcases to bug 20163 it was evident that my patch did not take into account textFrames with only whitespace. The attached patch does so.

I think this patch is probably too risky for Mozilla 1.0. It seems to be making a lot of changes, and I'm not sure how well it's imitating the quirks that were in the old code. I also suspect that, in general, fixing this bug will break sites, and we'll need to have some quirk. I could be wrong, though, and I still haven't looked at it in that much detail yet. You're still walking up the style context tree to get the decorations, which makes me think you're painting all the decorations multiple times, on top of each other. The point of walking up the tree was that we only painted the decorations from text frames, i.e., that text decorations were being drawn by children. Not doing this is the whole point of this bug. Given that, I don't think you need as much color manipulation. However, you still need a way to imitate the quirks, perhaps. The patch is missing the changes to nsBlockFrame.h The patch in general has many overly-long lines. Please try not to have lines longer than 80 characters, and please try to follow the surrounding style. The patch has a number of comments that are really questions and that shouldn't be checked in (since they don't really make sense as part of the code rather than part of a patch, e.g., "Stole much of this from PaintTextDecoration" when it's no longer in PaintTextDecoration, or "I'll go learn about the parser"), and also some that are commenting code that is self-documenting. I still have no idea what you mean about handling this in the parser (CSS parser or HTML parser), and I highly doubt it would work. The patch uses some bad style in the surrounding code. New code should use nsCOMPtr for owning references rather than NS_ADDREF / NS_RELEASE. See http://mozilla.org/projects/xpcom/nsCOMPtr/ . Some stylistic nits (randomly selected): + PRBool someKidIsANsTextFrame = findTextFrameAsChild(aPresContext, this); + if (!someKidIsANsTextFrame) *pDecorations = NS_STYLE_TEXT_DECORATION_NONE; The variable name |someKidIsANsTextFrame| doesn't make sense to me, and it's awfully long. How about |hasTextFrameChild|? Please break the if-then into two lines (for breakpointing and for readability). The name |findTextFrameAsChild| should follow the usual case conventions and begin with a capital letter. However, what you really mean, I think, is |HasTextFrameChild|. + if (decorations!=NS_STYLE_TEXT_DECORATION_NONE) { Space around != on both sides, please.

When this is ready for review, I'll build with the patch and check to see if it works as expected from a black box point of view. I suggest that, given the complexity of this bug, any r=/sr= should be dependent on this qa=. > Hixie-B ... do this I get a dinner? ;-) If this fixes the bug to my satisfaction and if I'm ever in the same place as you, then yes, of course! :-)

I'm working to iron out the issues pointed out to me. I have two questions: testcase showing problems / inconsistencies in painting order akahttp://bugzilla.mozilla.org/attachment.cgi?id=81296&action=view: While this testcase illustrates the problem well, I don't understand the text. Surely the images /should/ have lines through them? I thougt that sort-of was the whole point! testcase showing incorrect behavior with form controls aka http://bugzilla.mozilla.org/attachment.cgi?id=81295&action=view : As above. I can't find any exception stating that form controls should be excluded from text-decoration --- if it exist, can you give me a pointer, pretty please?. However, the line should go all the way through. I'll fix that. The right alignment thing will need a bit of work. The darn line bounds is relative to it's own lower left corner, that is, always (0,0), so I'll have to query the style content unless I can figure out something else. I've made stylistic changes requested which will appear in my next patch. Do I /hate/ the 80-char linewidth restriction --- it makes perfectly sensible code into a mess :-( I have to assume that this is due to some obscure compiler/platform or other? (This was the case is OS390 until recently....) Oh, and on the same node, I refuse to do if (something) do something; Not after using two hours debugging (without a debugger) an Italien program because a programmer had done this: if (something) do something else; do something; So I've change the offending places to: if (something) { do something } which should make everyone happy, right? Hixie: Sounds great :-) In what country would I have to be in to satisfy the requirements? :O)

The 80 character restriction is because some people use editors at a width of 80 characters per line. Some people use them somewhat wider as well, but I've never seen anyone advocate 175 characters per line as your patch used. It's acceptable to go over the limit occasionally, but, for example, for wrapping multi-line comments, you should definitely wrap before the 80th column, or otherwise they'll look, to many people, like: // This is a comment that is very difficult to read because it was written by someone // who wraps lines at longer than 80 characters, so that they're very hard for // everyone else to read. You shouldn't assume everyone else displays code in the exact same editor, at the exact same width, that you do.

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

After studying the CSS2 specs again, I believe that this patch creates the right behaviour in all the attached testcases, and a couple more. This patch is lacking one of two items, but I'm not sure which path to take. As the patch stands, text-decorations are somewhat expensive: Every time a frame /might/ have text-decoration, an entire section of the tree is queried. If this is the path to take, the bits like HasTextDecorations() should be removed from ns(I)StyleContext.* If this is not the path to take, I will have to come up with a system of keeping track of what frame have text-decoration. In the light of dynamic changes and the somewhat convoluted specs regarding text-decorations, this will be neither simple nor cheap. But it may be cheaper :-) Thanks for all the help I'm getting here. Your feedback is invaluable :-D P.S: I've edited the patch as it contained some changes I didn't make, as well as some spacing issues. Please tell me of the patch doesn't work or something.

reassign to patch author

Accepting bug. Also, I've decided to go with the current solution, as I'm unconvinced that the more sofisticated solution (described above) would perform any better. The solution in the above patches perform more or less like the current, incorrect, implementation. As soon as my wife relinquishes my computer, I'll create and upload a "final" patch, where the (now) unused functions in nsStyleContext are removed.

Comment on attachment 82518 [details] [diff] [review] Take 3. Now does text-alignment, painting-layers and form elements right. I hope :) I'm still very concerned about the the quirk code. I think the current code breaks it pretty badly, although I haven't tested. You need to analyze why it was put in and get some testcases. I'm also somewhat concerned that moving to this behavior will require *more* quirks. I also think it's way too late to consider this for 1.0. This also probably needs some more testcases for testing in general. >Index: nsLineLayout.cpp >+ // It appears that text is placed along "0", with the line extending >+ // from mMinY (which will be negative) to mMaxY (which will be >+ // positive). So mMinY will be the "ascent" of the line... that is >+ // the distance from the top of the line to the baseline of the >+ // fonts placed normally along the line. >+ aLineBox->SetAscent(-psd->mMinY); "It appears"? That suggests the patch isn't ready yet. Can you say something more definitive? But nsLineLayout::VerticalAlignLine already computes the line's ascent. Why can't you use that calculation? >Index: nsHTMLContainerFrame.h >+ /** Helper function to paint text decorations for this frame. This function >+ attempts to be general; hopefully particular frames can get away with >+ overriding PaintTextDecorationLines. >+ */ For this and other comments, could you try to stick to the existing style in layout, i.e., /* * Comment here. */ >+ void GetTextDecorations(nsIPresContext* aPresContext, >+ PRUint8* pDecorations, >+ nscolor* pUnderColor, >+ nscolor* pOverColor, >+ nscolor* pStrikeColor); The comment for this function should say that what it handles is the rules that text-decoration should be painted only on elements with text. >+ aRenderingContext: If one would draw anything, one needs one of these babies :O) If that's all you have to say, could you just skip the comment? Also, I think |PaintTextDecorationLines| should not have the final "s", since it only paints one line. offset should also be described as the distance *above* the baseline, so that it's clear that negative numbers mean below the baseline (which is the reverse of our usual coordinate space for more general layout). >+ PRBool HasTextFrameChild(nsIPresContext* aPresContext, nsIFrame* parent); Why is this a member function? It doesn't look like it uses |this| at all, so you're wasting time passing an extra parameter all the way down the recursion. It seems like it should be a global static function within the .cpp file. >Index: nsHTMLContainerFrame.cpp >+void >+nsHTMLContainerFrame::GetTextDecorations(nsIPresContext* aPresContext, >+ PRUint8* pDecorations, >+ nscolor* pUnderColor, >+ nscolor* pOverColor, >+ nscolor* pStrikeColor) { Could you push the { to the beginning of the next line (as other code in the module does) rather than leaving the next line blank? Same for all the other functions. >+ >+ PRBool useOverride = PR_FALSE; >+ nscolor overrideColor; >+ nsCOMPtr<nsIStyleContext> context = mStyleContext; >+ >+ *pDecorations = NS_STYLE_TEXT_DECORATION_NONE; >+ PRUint8 decorMask = NS_STYLE_TEXT_DECORATION_UNDERLINE >+ | NS_STYLE_TEXT_DECORATION_OVERLINE >+ | NS_STYLE_TEXT_DECORATION_LINE_THROUGH; // A mask of all possible decorations. >+ >+ PRBool isBlock; >+ do { >+ // find text-decorations. Inherit from block frames, but /only/ if no >+ // inline frames comes between. This should prevent multiple drawings >+ // of the same text-decoration AND make block-level text-decoration >+ // apply to the nearest inline child only. But you have the block frame paint 'text-decoration' itself, so why is this necessary? Aren't you painting twice? >+ const nsStyleTextReset* styleText = (const nsStyleTextReset*)context->GetStyleData(eStyleStruct_TextReset); Line too long. And the next few as well, I think. >+ if (!useOverride && (NS_STYLE_TEXT_DECORATION_OVERRIDE_ALL & styleText->mTextDecoration)) { >+ // This handles the <a href="blah.html"><font color="green">La la la</font></a> case. >+ // The link underline should be green. >+ const nsStyleColor* styleColor = >+ (const nsStyleColor*)context->GetStyleData(eStyleStruct_Color); >+ useOverride = PR_TRUE; >+ overrideColor = styleColor->mColor; >+ } Since this code is only being used for the direct children of blocks, this is insufficient to handle the quirk. You could say PRUint8 decors = decorMask & styleText->mTextDecoration and then use that in these 4 places (except I don't think you need this coee at all): >+ if (decorMask & styleText->mTextDecoration) { >+ if (NS_STYLE_TEXT_DECORATION_UNDERLINE & decorMask & styleText->mTextDecoration) { >+ if (NS_STYLE_TEXT_DECORATION_OVERLINE & decorMask & styleText->mTextDecoration) { >+ if (NS_STYLE_TEXT_DECORATION_LINE_THROUGH & decorMask & styleText->mTextDecoration) { leak. You need dont_AddRef(). >+ const nsStyleDisplay* display; >+ this->GetStyleData(eStyleStruct_Display, (const nsStyleStruct*&) display); Prefer |::GetStyleData(this, &display);| >+ isBlock = NS_STYLE_DISPLAY_INLINE != display->mDisplay; >+ } >+ } while ((isBlock) && (nsnull != context) && (0 != decorMask)); You need to check |decorMask| first if you want to avoid reading uninitialized memory (isBlock). Context should be null-checked without the "nsnull !=". >+ return; >+} Not really needed in a |void| function at the end. Also, please leave a line of whitespace between this function and the next. >+PRBool >+nsHTMLContainerFrame::HasTextFrameChild(nsIPresContext* aPresContext, >+ nsIFrame* parent) { >+ >+ nsIFrame* kid = nsnull; >+ parent->FirstChild(aPresContext,nsnull,&kid); Please put a space after each ",". >+ nsCOMPtr<nsIAtom> frameType; >+ >+ while (nsnull != kid) { This could be a for loop: for (parent->FirstChild(aPresContext, nsnull, &kid); kid; kid->GetNextSibling(&kid)) { ... but I say that because I like |for| loops, and you don't need to do this. >+ kid->GetFrameType(getter_AddRefs(frameType)); >+ if (frameType.get() == nsLayoutAtoms::textFrame) { No need for the |.get()|. >+ // This is only a candidate. We need to determine if this >+ // textFrame is empty, as in containing only (non-pre) >+ // whitespace. See bug 20163 >+ nsCompatibility mode; >+ aPresContext->GetCompatibilityMode(&mode); >+ PRBool isEmpty; >+ PRBool isPre = PR_FALSE; >+ const nsStyleText* styleText; >+ kid->GetStyleData(eStyleStruct_Text, (const nsStyleStruct*&) styleText); prefer ::GetStyleData(kid, &styleText); >+ if ((NS_STYLE_WHITESPACE_PRE == styleText->mWhiteSpace) || >+ (NS_STYLE_WHITESPACE_MOZ_PRE_WRAP == styleText->mWhiteSpace)) { >+ isPre = PR_TRUE; >+ } This should be: PRBool isPre = styleText->mWhiteSpace == NS_STYLE_WHITESPACE_PRE || styleText->mWhiteSpace == NS_STYLE_WHITESPACE_MOZ_PRE_WRAP; (Note there are no extra parentheses, so some compilers will warn about equality mistyped as assignment.) >+ >+ Only one new line between functions, though, unless there's some big logical split (which there doesn't seem to be here). >+void If it's virtual, it might be nice to write /* virtual */ void >+nsHTMLContainerFrame::PaintTextDecorationLines( >+ nsIRenderingContext& aRenderingContext, >+ nscolor color, nscoord offset, >+ nscoord ascent, >+ nscoord size) { >+ >+ aRenderingContext.SetColor(color); >+ aRenderingContext.FillRect(0, ascent - offset, mRect.width, size); >+ >+ return; >+} Again, { on its own line, and unneeded |return;|. >+void >+nsHTMLContainerFrame::PaintTextDecorations( >+ nsIPresContext* aPresContext, >+ nsIRenderingContext& aRenderingContext) { >+ >+ nscolor underColor, overColor, strikeColor; >+ PRUint8 decorations; >+ GetTextDecorations(aPresContext, >+ &decorations, >+ &underColor, >+ &overColor, >+ &strikeColor); You don't need each argument on its own line. >+ >+ if (decorations != NS_STYLE_TEXT_DECORATION_NONE) { >+ >+ nsStyleFont* font; Make this |const nsStyleFont *font|. >+ font = (nsStyleFont*) mStyleContext->GetStyleData(eStyleStruct_Font); ::GetStyleData(this, &font); >+ // Cache the original decorations and reuse the current font >+ // to query metrics, rather than creating a new font which is expensive. >+ nsIFontMetrics* normalFont; You should use nsCOMPtr<nsIFontMetrics>, and it should be declared closer to first use (below). >+ nsFont* plainFont = &font->mFont; ...and use NS_CONST_CAST here to show that what you're doing is dangerous. (Old-style casts are evil because they hide too well.) >+ NS_ASSERTION(plainFont, >+ "null plainFont: font problems " >+ "in nsHTMLContainerFrame::Paint"); No need for "in nsHTMLContainerFrame::Paint", since the NS_ASSERTION macro gives line numbers. (So this can all be one line. But why even bother with the assertion? If |font| is null then |plainFont| will be 0x4. >+ PRUint8 originalDecorations = plainFont->decorations; >+ plainFont->decorations = NS_FONT_DECORATION_NONE; Maybe try an NS_ASSERTION(plainFont->decorations == NS_FONT_DECORATION_NONE, "fonts on style structs shouldn't have decorations"); and see if it fires? But perhaps leave the correction code in for at least a bit, anyway? >+ nsCOMPtr<nsIDeviceContext> deviceContext; >+ aRenderingContext.GetDeviceContext(*getter_AddRefs(deviceContext)); >+ deviceContext->GetMetricsFor(*plainFont, normalFont); *getter_AddRefs(normalFont) >+ plainFont->decorations = originalDecorations; >+ >+ /* Get ascent of the font */ >+ NS_ASSERTION(normalFont, "null normal font cannot be handled"); >+ nscoord ascent; >+ normalFont->GetMaxAscent(ascent); >+ >+ /* Paint */ These two comments seem unnecessary, since they only state the obvious and nothing more. >+ nscoord offset, size; >+ if (decorations & >+ (NS_STYLE_TEXT_DECORATION_UNDERLINE >+ | NS_STYLE_TEXT_DECORATION_OVERLINE)) { >+ normalFont->GetUnderline(offset, size); >+ if (NS_STYLE_TEXT_DECORATION_UNDERLINE & decorations) { >+ PaintTextDecorationLines(aRenderingContext, >+ underColor, >+ offset, >+ ascent, >+ size); You don't need each argument on its own line. Just wrap when they hit 80 characters back to the indentation where you put each one. >+ } >+ if (NS_STYLE_TEXT_DECORATION_OVERLINE & decorations) { >+ PaintTextDecorationLines(aRenderingContext, >+ overColor, >+ ascent, >+ ascent, >+ size); >+ } >+ } >+ if (NS_STYLE_TEXT_DECORATION_LINE_THROUGH & decorations) { >+ normalFont->GetStrikeout(offset, size); >+ PaintTextDecorationLines(aRenderingContext, >+ strikeColor, >+ offset, >+ ascent, >+ size); >+ } >+ } >+ >+ return; >+} Again, no |return;| with blank line before, but a blank line after the function. > NS_IMETHODIMP > nsHTMLContainerFrame::Paint(nsIPresContext* aPresContext, > nsIRenderingContext& aRenderingContext, >@@ -131,6 +321,15 @@ > // hidden > > PaintChildren(aPresContext, aRenderingContext, aDirtyRect, aWhichLayer, aFlags); >+ >+ /* After painting the children, let's add the text-decoraions. That way >+ they won't get painted over. */ No, this should be before. I suspect you're failing testcases like: <style type="text/css"> span.one { color: red; text-decoration: underline; } span.two { color: green; text-decoration: underline; } </style> <p><span class="one">There <span class="two">should only be a tiny bit of red underline in this</span> paragraph</span>.</p> >+ if (eFramePaintLayer_Overlay == aWhichLayer Test NS_FRAME_PAINT_LAYER_FOREGROUND rather than the enum value, to be consistent with other code. >+ && frameType.get() == nsLayoutAtoms::inlineFrame) { >+ PaintTextDecorations(aPresContext, aRenderingContext); >+ } >+ >+ > > return NS_OK; > } No need for so many blank lines. >Index: nsBlockFrame.h > protected: >+ /** >+ Function that does the actual drawing of the textdecoration. >+ input: >+ aRenderingContext: If one would draw anything, one needs one of these babies :O) >+ color: the color of the text-decoration >+ ascent: ascent of the font from which the text-decoration was derived. >+ offset: distance from baseline to where the text-decoration should be drawn. >+ size: the thickness of the line >+ output: >+ none */ Probably the comment should just say /* override member function of nsHTMLContainerFrame. */ and perhaps also say *why* the override is needed, but not repeat the comment that's on nsHTMLContainerFrame (which is likely to change without this one being updated, or vice versa). >Index: nsBlockFrame.cpp >=================================================================== >RCS file: /cvsroot/mozilla/layout/html/base/src/nsBlockFrame.cpp,v >retrieving revision 3.506 >diff -u -r3.506 nsBlockFrame.cpp >--- nsBlockFrame.cpp 3 May 2002 13:49:44 -0000 3.506 >+++ nsBlockFrame.cpp 6 May 2002 19:54:16 -0000 >@@ -5220,6 +5220,29 @@ > return rv; > } > >+void >+nsBlockFrame::PaintTextDecorationLines(nsIRenderingContext& aRenderingContext, >+ nscolor color, >+ nscoord offset, >+ nscoord ascent, >+ nscoord size) { >+ aRenderingContext.SetColor(color); >+ nscoord accHeight = 0; >+ for (nsLineList::iterator line = begin_lines(), line_end = end_lines(); line != line_end; ++line) { >+ if (!line->IsBlock()) { >+ nsPoint firstChildOrigin; >+ line->mFirstChild->GetOrigin(firstChildOrigin); >+ aRenderingContext.FillRect(firstChildOrigin.x, Seems like you should just use line->mBounds.x rather than this |firstChildOrigin| stuff. >+ accHeight + line->GetAscent() - offset, >+ line->mBounds.width, >+ size); >+ accHeight += line->mBounds.height; >+ } >+ } >+ >+ return; >+} >+ Same as before about |return;|. > NS_IMETHODIMP > nsBlockFrame::Paint(nsIPresContext* aPresContext, > nsIRenderingContext& aRenderingContext, >@@ -5301,6 +5324,14 @@ > if (NS_STYLE_OVERFLOW_HIDDEN == disp->mOverflow) { > PRBool clipState; > aRenderingContext.PopState(clipState); >+ } >+ >+ if (aWhichLayer == eFramePaintLayer_Overlay && !mLines.empty()) { Again, for consistency with other code, check NS_FRAME_PAINT_LAYER_FOREGROUND. >+ nsCOMPtr<nsIAtom> frameType; >+ GetFrameType(getter_AddRefs(frameType)); >+ if (frameType.get() == nsLayoutAtoms::blockFrame) { >+ PaintTextDecorations(aPresContext, aRenderingContext); >+ } What other frame type do you expect an nsBlockFrame to be? Is this because of form controls? If so, you need a comment. However, this seems like the wrong way to solve a form control problem, if that's what it is. (Any particular reason there were a very large number of blank lines at the end of the patch?)

I chopped out the code I was quoting for the comment "leak. You need dont_AddRef().", which was context = context->GetParent(); Also, there were some other unneeded .get() uses on == comparisons with |nsCOMPtr|s that I didn't mention in addition to the ones that I did. (They used to be needed due to an old nsCOMPtr bug, but they aren't anymore.)

This is my final patch (if no further concerns are raised.) After reviewing the content in nsStyleContext, I realized that this was a) still used by xul and b) usable for my patch as well. It simply establishes a neccessary (but not sufficient) condition for a frame to have any text-decorations. With this optimization, I'm quite sure that this patch is a good solution for this bug. Anyone is welcome to disagree. On a side note, after (re)reading the keyword section, I don't think this bug merits the high-risk keyword. I'll remove it in a bit, just add it again if you disagree. I wouldn't recommend it for mozilla 1.0, though.

Updating keywords to reflect status...

Comment on attachment 83043 [details] [diff] [review] Take 4. This patch is final (until otherwise proven :o) ) All or nearly all of my review comments on the previous patch also apply to this one.

Regarding my comment "But you have the block frame paint 'text-decoration' itself, so why is this necessary? Aren't you painting twice?": I see now that this is an attempt to handle the issues in comment 83. However, those issues aren't the ones where the quirk is involved -- it's involved for inlines. (Seeing the quirk code there originally made me expect the condition to be |!isBlock|, and I didn't fully readjust to what it was really doing.) However, in light of that, I think display != NS_STYLE_DISPLAY_INLINE may be too generous a definition of a block. I'd need to think about it, though.

... but if that's the point, you should be setting |isBlock| before you do the |GetParent|, right, rather than after? Otherwise you paint block text decorations twice, no?

I can see I misunderstood how the font quirk is supposed to work. My patch breaks this testcase. I'll have to think about this. I supposed the easiest way would be to let <FONT> inline frames paint any text-decorations on their own, overpainting the "true" text-decoration. If anybody knows any other quirks in this context, PLEASE tell me.

I've made the correction suggested below, with the following comments. Everything that has been suggested and to which I had no comment I've simply implemented. One issue remains: Painting in the foreground layer does not garantee that form controls & images does not paint the text-decorations over. PLEASE: I need some help here. Should I add a new layer? What should I call it? NS_FRAME_PAINT_LAYER_DECORATIONS? Also, adding a new layer sounds expensive... but I see no other way. >I'm still very concerned about the the quirk code. I think the >current code breaks it pretty badly, although I haven't tested. You >need to analyze why it was put in and get some testcases. I'm also >somewhat concerned that moving to this behavior will require *more* >quirks. > I've searched the web for any information about this, and only found the font thing. How can I "analyse" why it was put in? I can only mimic it's behaviour... I've created testcase, that I think demostrates the quirk in question. I'm updating the patch to handle this case... by making the <FONT> tag draw it's own text-decoration, including the ones "applied" from an ancestor. Practically speaking, I'll walk up the tree in GetTextDecorations if either it's a FONT tag or if a block-element. >I also think it's way too late to consider this for 1.0. > >This also probably needs some more testcases for testing in general. > There is a fair bit, including the links in the bug comments below, but more are welcome. (I can provide some too, of course, but I'm more or less out of ideas...) / >>Index: nsLineLayout.cpp >>+ // It appears that text is placed along "0", with the line extending >>+ // from mMinY (which will be negative) to mMaxY (which will be >>+ // positive). So mMinY will be the "ascent" of the line... that is >>+ // the distance from the top of the line to the baseline of the >>+ // fonts placed normally along the line. >>+ aLineBox->SetAscent(-psd->mMinY); >> >> > >"It appears"? That suggests the patch isn't ready yet. Can you >say something more definitive? > >But nsLineLayout::VerticalAlignLine already computes the line's >ascent. Why can't you use that calculation? > I assume you're referring to aLineBoxAscent, which is a paramenter to nsLineLayout::VerticalAlignLine. This is not the correct "ascent". In the face of multiple box-sizes on the line, the "ascent of a line" is not as well-defined as in the case of fonts. I can't remember what distance aLineBoxAscent does measure, but I can assure everyone, it's the wrong one :o) But I can update the comment to be more assertive :) The reason for the indecisiveness was the bits below which readjust the lineheight in some cases, and I wasn't quite sure that this didn't effect this distance. As far as I could determine, the later adjustments was irrelevant and therefore the code is correct. I have just rechecked this and I'm 99% sure that this is so. >Also, I think |PaintTextDecorationLines| should not have the final "s", >since it only paints one line. > Err. It sometimes paints several lines, namely if a nsBlockFrame contain multiple inline lines. >>+ >>+ PRBool useOverride = PR_FALSE; >>+ nscolor overrideColor; >>+ nsCOMPtr<nsIStyleContext> context = mStyleContext; >>+ >>+ *pDecorations = NS_STYLE_TEXT_DECORATION_NONE; >>+ PRUint8 decorMask = NS_STYLE_TEXT_DECORATION_UNDERLINE >>+ | NS_STYLE_TEXT_DECORATION_OVERLINE >>+ | NS_STYLE_TEXT_DECORATION_LINE_THROUGH; // A mask of all possible decorations. >>+ >>+ PRBool isBlock; >>+ do { >>+ // find text-decorations. Inherit from block frames, but /only/ if no >>+ // inline frames comes between. This should prevent multiple drawings >>+ // of the same text-decoration AND make block-level text-decoration >>+ // apply to the nearest inline child only. >> >> > >But you have the block frame paint 'text-decoration' itself, so why >is this necessary? Aren't you painting twice? > I've changed this to be both clearer and more correct. Basically, I only walk up the tree if it's a blockframe (or the FONT quirk). Put another way: There are two cases for this bug: There is the block frame case and the inline case. The inline case is simple: Just draw the text-decorations specified for this element. The block case is more complex, as the text-decorations should be drawn on all inline children. To make this work in cases like <div class="underline><div><p>Hello</div></div> I need to make <p> walk up the tree to fetch the underline from the outer <div class="underline">. >>+ if (!useOverride && (NS_STYLE_TEXT_DECORATION_OVERRIDE_ALL & styleText->mTextDecoration)) { >>+ // This handles the <a href="blah.html"><font color="green">La la la</font></a> case. >>+ // The link underline should be green. >>+ const nsStyleColor* styleColor = >>+ (const nsStyleColor*)context->GetStyleData(eStyleStruct_Color); >>+ useOverride = PR_TRUE; >>+ overrideColor = styleColor->mColor; >>+ } >> >> > >Since this code is only being used for the direct children of >blocks, this is insufficient to handle the quirk. > Does the testcase cover your concern? I've only learned HTML/CSS the standards way, so I'm not very knowledgeable about the quirks, unfortunately. I have changed the behaviour of the quirk handling to be able to handle the new testcase. >You could say >PRUint8 decors = decorMask & styleText->mTextDecoration >and then use that in these 4 places (except I don't think >you need this coee at all): > I do need to walk up (or down) the tree. Walking down the tree (that is, letting the frame where the text-decoration is specified draw the text-decoration for all it's inline descendents/children) is certainly a possibility. Not too easy though. Would it be any better, you think? >>+ nsCOMPtr<nsIAtom> frameType; >>+ >>+ while (nsnull != kid) { >> >> > >This could be a for loop: >for (parent->FirstChild(aPresContext, nsnull, &kid); kid; > kid->GetNextSibling(&kid)) { >... but I say that because I like |for| loops, and you don't >need to do this. > I like the |for| loop. It's a little clearer. >>+ NS_ASSERTION(plainFont, >>+ "null plainFont: font problems " >>+ "in nsHTMLContainerFrame::Paint"); >> >> > >No need for "in nsHTMLContainerFrame::Paint", since the >NS_ASSERTION macro gives line numbers. (So this can all be >one line. But why even bother with the assertion? If >|font| is null then |plainFont| will be 0x4. > You're right. Should have checked the code I stole more closely :-[ >>+ PRUint8 originalDecorations = plainFont->decorations; >>+ plainFont->decorations = NS_FONT_DECORATION_NONE; >> >> > >Maybe try an >NS_ASSERTION(plainFont->decorations == NS_FONT_DECORATION_NONE, > "fonts on style structs shouldn't have decorations"); >and see if it fires? But perhaps leave the correction code in for >at least a bit, anyway? > Good points. I'll do this. >>NS_IMETHODIMP >>nsHTMLContainerFrame::Paint(nsIPresContext* aPresContext, >> nsIRenderingContext& aRenderingContext, >>@@ -131,6 +321,15 @@ >> // hidden >> >> PaintChildren(aPresContext, aRenderingContext, aDirtyRect, aWhichLayer, aFlags); >>+ >>+ /* After painting the children, let's add the text-decoraions. That way >>+ they won't get painted over. */ >> >> > >No, this should be before. I suspect you're failing testcases like: > ><style type="text/css"> >span.one { > color: red; > text-decoration: underline; >} > >span.two { > color: green; > text-decoration: underline; >} ></style> > ><p><span class="one">There <span class="two">should only be a tiny bit of >red underline in this</span> paragraph</span>.</p> > Right you are. This may be a bigger problem, since pictures, forms and such are already drawn in the foreground, and I need to do it in a layer before this. Adding another layer seems a bit drastic, but currently I see no other way.... See also my comments at the top of this comment. >>+ for (nsLineList::iterator line = begin_lines(), line_end = end_lines(); line != line_end; ++line) { >>+ if (!line->IsBlock()) { >>+ nsPoint firstChildOrigin; >>+ line->mFirstChild->GetOrigin(firstChildOrigin); >>+ aRenderingContext.FillRect(firstChildOrigin.x, >> >> > >Seems like you should just use line->mBounds.x rather than >this |firstChildOrigin| stuff. > Nice to know I'm not the only one to think so :-D Only, as your testcase illustrated, line->mBounds.x is always 0, even when a short line is right-aligned or centered. Could be a bug in lineBox? >>+ nsCOMPtr<nsIAtom> frameType; >>+ GetFrameType(getter_AddRefs(frameType)); >>+ if (frameType.get() == nsLayoutAtoms::blockFrame) { >>+ PaintTextDecorations(aPresContext, aRenderingContext); >>+ } >> >> > >What other frame type do you expect an nsBlockFrame to be? Is this >because of form controls? If so, you need a comment. However, this >seems like the wrong way to solve a form control problem, if that's >what it is. > Ah. Well, the thing is, I need to be sure that this frame is indeed a block frame, CSS box-model speaking. Form controls subclasses nsBlockFrame and don't override the Paint() so I need to do something to filter these and similar cases out. I'll add a comment for now. What I really want is a "isBlockFrame()" member function, but no such function is avaible. >(Any particular reason there were a very large number of blank lines >at the end of the patch?) > > I only ran cvs diff -u [filenames] >~/bug1777-4.patch Don't know why this should give a lot of blank lines... :-] But then, I'm no expert in this. P.S: How do you guys quote each other in bugzilla? I used the composer, then converted the message to plaintext by sending it to myself, and then copied this to bugzilla... not exactly elegant :O) P.P.S: I have the patch working for all the above testcases, URL and with dbarons suggestions incorporated, except the issue with the painting layer. I'll spare you the spam, though, but if anyone wants to see the patch, just tell me :)

> How can I "analyse" why it was put in? Here's one way: go to http://lxr.mozilla.org/seamonkey, find the file you are interested in, open it, click on the "CVS blame" link at the top of the page. Now you can go to any line of that file you are interested in, and see who changed it last, what bug that change was part of a fix for etc.

> Right you are. This may be a bigger problem, since pictures, forms and > such are already drawn in the foreground, and I need to do it in a layer > before this. Adding another layer seems a bit drastic, but currently I > see no other way.... See also my comments at the top of this comment. I don't think you need a new layer, and I don't think this is a bug. I think you just should be consistent and allow children to paint over their ancestors' text decorations by putting the painting of text decorations before PaintChildren.

If aLineBoxAscent is the wrong number, then I think it should be fixed. Do you have a testcase where it does the wrong thing? It should be giving the ascent for the "root inline box", i.e., the anonymous inline box that doesn't really exist that wraps all the children of the block.

I can't remember where I read this, but IIRC the paint order should be: ,-------------------. '-> underline | overline | text | children -------' line-through I thought it was in the latest CSS3 Text Module draft but I can't find it there. So: .outer { text-underline-color: red; text-line-through-color: green; } .inner { text-underline-color: green; text-line-through-color: red; } span { text-underline-style: solid; text-line-through-style: solid; } <span class="outer"><span class="inner"> This should have green underline and green line-through. </span></span> Of course that is hard to test with the current patch since it only supports CSS2 and not CSS3, but it should give you an idea as to how to best implement it if it has to be easily extended to CSS3. (Not implementing CSS3 is fine, BTW. The spec isn't even a last call draft, so we shouldn't be implementing it yet. As far as I can tell this patch should make implementing CSS3's new properties a lot easier when the spec is published, which is great.)

On the FONT quirk: Well, lxr+bugzilla is way cool 8-) dbaron, thanks for pointing this out to me. Unfortunately, it didn't help me much. The relevant lines were checked in for a performance bug; probably hyatt moved them from somewhere else.... perhaps hyatt can remember from where or something? I do remember reding somewhere that the PaintTextDecorations() in nsTextFrame was coalescented from existing code. I also tried to query bugzilla, but I didn't find anything... The relevant entry from lxr/CVS blame: *1769 hyatt* 1.306 const nsStyleTextReset* styleText = 1770 (const nsStyleTextReset*)context->GetStyleData(eStyleStruct_TextReset); 1771 if (!useOverride && (NS_STYLE_TEXT_DECORATION_OVERRIDE_ALL & styleText->mTextDecoration)) { 1772 // This handles the <a href="blah.html"><font color="green">La la la</font></a> case. 1773 // The link underline should be green. 1774 const nsStyleColor* styleColor = 1775 (const nsStyleColor*)context->GetStyleData(eStyleStruct_Color); 1776 useOverride = PR_TRUE; 1777 overrideColor = styleColor->mColor; 1778 } 1779

dbaron: you were right about the mLineAscent calculated is the right one... the ascent was measured from the top of the containing block frame, and somehow, I misintepreted this :-( Thanks for pointing this out to me. I've changed my patch to reflect this. Also, since the patch stores the ascent of each line anyway, I've removed aLineAscent from the parameter list of VerticalAlignLine, as any function who calls VerticalAlignLine and needs aLineAscent might as well query the nsLineBox directly. I've changed the one place VerticalAlignLine is called to reflect this. The other line issue, that is that the line->mBounds.x is always zero, even when the origin of the first child frame is non-zero: line->mBounds.x=0, firstChildOrigin.x=14638 This is from a test I ran. Is this a bug in nsLineBox? It sure does look like one...

Some further investigation regarding painting order: According to the CCS2 specs, block frames should be painting /behind/ floats which should be painted behind inline frames. See http://bugzilla.mozilla.org/show_bug.cgi?id=36710 Practically, this means that some content (inline frames) are painted in the foreground layer, which, in turn, means that textdecorations get painted behind these frames. This cannot be the expected, or correct, behaviour?! This means, e.g., that strike-though is painted behind any images in the flow...

On searching LXR (Re comment 123): If you're looking at CVS blame for a file, you can add, e.g., "&rev=1.32" to the end of the URL to get blame for that revision. You can use this (and queries of bonsai, http://bonsai.mozilla.org/ , to trace code that moved around). On the line box (Re comment 124): It might be better not to change what nsLineBox::mBounds::x means, since a lot of complicated code has been written around the current assumptions. However, it really seems wrong that the x is not the x that corresponds to the range of the width. Perhaps leave your current workaround and file a separate bug? On the painting order (Re comment 125): Ian's comment 122 seems correct, so I think you should call GetTextDecorations from the paint method and then call PaintTextDecorations twice, with diffent parameters for which decorations to paint. One more comment: The |isBlock| check in the loop in GetTextDecorations really isn't the right test. You really want to pass in a boolean parameter |aIsBlock| depending on who is calling the function, and if it's true, check the bits on the style context and walk all the way up (except we might need a quirk for tables to stop the walking in quirks mode).

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

OK, I *think* I've made it all work :) Tomorrow I'll clean up the code and submit another patch. However, in order to make form controls work, I've had to specialcase them, and I'm somewhat unhappy with this. However, since the buttom of a tree containing a form control looks something like this: scrollFrame textInputFrame <--- the text-decorations are here scrollFrame AreaFrame <--- the lines, and consequently, the text, are here. I need to use a fairly tight criterium to detect a possible situation like this, and then walk up the tree to find (if) there is a textInputFrame above the AreaFrame in the tree. From this textInputFrame I need to pull the textdecorations. The criterium I use is that the frame must be an areaFrame with NS_FRAME_HAS_VIEW state, view the styleContext having the HasTextDecoration property. This specialcasing raises another issue: I would want this kind of logic in the GetTextDecoration function; otherwise I'll either need to encumber nsBlockFrame with another helpermethod or make nsBlockFrame::Paint() longer than I would like. This means that dbarons suggestion in comment 126 about the aIsBlock will have to be replaced with a logic based on the current frameType. I'll try to come up with something reasonable, and then you can pick it apart :o) On this node: Can somebody create a point to a reasonable complex form testcase with text-decorations? I'm really quite unfamiliar with forms, so I'm not too sure I've covered all eventualities. Some table testcases would be neat, too. Re comment 126 and others: I've implemented Ian suggestion. I'll file a bug regarding the nsLineBox tomorrow. I'm unable to find anything on hyatt's code regarding the FONT quirk excepting the checkin comment to 1.306 of nsTextFrame.cpp stating "rule matching improvements". This seems to part of a rather large checkin, but scanning this has given me no new insights. But given the size, I might easily have missed something. This is the complete checkin, should anyone care :) http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=SeaMonkeyAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=hyatt.*&whotype=regexp&sortby=Date&hours=2&date=explicit&mindate=05%2F31%2F2001+15%3A00&maxdate=05%2F31%2F2001+16%3A00&cvsroot=%2Fcvsrootcheck-in, That was my 5€. Now I need some sleep, and tomorrow I'll create a new "final" version of the patch. I'll also attach the a testcase not represented above regarding nested text-decorations and block frames. P.S: One thing mystified me about forms: in the form control testcase above http://bugzilla.mozilla.org/attachment.cgi?id=81295&action=view the entire tree rendering the form has display property set to block. Shouldn't this property be inline?! If not, then form controls are invalid in <P></P> tags, I suppose.

> The criterium I use is that the frame must be an areaFrame with > NS_FRAME_HAS_VIEW state Don't do that; there are many ways a frame can have that state set. > Can somebody create a point to a reasonable complex form testcase > with text-decorations? Debug | Viewer Demos | #5 More Styles

Robert O'Callahan: Don't worry about the NS_FRAME_HAS_VIEW, it's only used as (a part of) a neccessary criterium, not to determine when to draw text-decorations. Regarding the viewer demo: I've no idea if it should look like this. It look reasonable, but whether the rendering is 100% correct, I really don't know. It is, IMHO, better than the current rendering.

I've implemented the suggestions from David Baron and Ian Hixie. This patch correctly renders all the testcases in this bug. I've also watched choffman's international browser bust for some time without encountering anything untoward. See my comments above for implementation details. What I havn't done is make any performance regression checks; I couldn't figure out how to do this. However, I don't see anything in the patch that should cause much worse performance than today, except perhaps if a frame with text-decorations have a large subtree without any text-frames.

Robert's comment 129 is correct in that you should not be checking whether a frame has a view for something like this. I haven't had a chance to look at the patch yet.

Re: NS_FRAME_HAS_VIEW. I added the check for performance reasons, so it's not essential. But why not use it? Is the statement AreaFrame is part of a frame control => NS_FRAME_HAS_VIEW is set false? It seemed to be true judging from the code, and also from my understanding. Is it something else? Note, I'm not interested in the opposite implication (which, as pointed out, is false), only left to right.

What exactly is "the form control problem"? (i.e. what behaviour are you trying to implement and what is the problem you run into?) I couldn't find a description above, although I may have just missed it (feel free to just point me to a previous comment :-) ).

I've detailed the problem at the top of comment 128. Basically, the problem is to detect the text form controls, as the style information is associated further up the tree than the actual line which contain the text-frames.

No, I mean, what is the problem. You say "in order to make form controls work" but, what do you call "work", and what doesn't "work" about them without any special coding?

Sorry. Without special coding the textdecoration are drawn on all form controls which have any text-decorated parents, as the form controls report themselves as display: block even though they are inline. If I force them to be display:inline, no text-decorations are drawn, as the text-decoration reside further up the tree. To draw... text-decorated frame [...] textInputFrame, where any text-decoraton style info reside ^[...] | areaFrame, where the lines to be text-decorated reside. +--------+ where text-decotaion should be fetched from (only) Simply having the text form controls report the display property as inline would help to make the special case cleaner, BTW.

> Without special coding the textdecoration are drawn on all form controls > which have any text-decorated parents ...and why is that a problem? Form controls should be display: -moz-inline-box or inline-block or something like that. If they are contained within underlined text, the underline should go under the form controls. If they are contained within line-through text, they should be struck out: _( )__Underlined_Radio_Button_And_Label._ ... :-: Unchecked checkbox with dotted border and line-through text decoration. ''' ___________________________________ Edit box with overline: [ ] Note how the radio button overlaps the underlining, the line-through goes over the checkbox, and the overline would go under the text box except that (in this case) the textbox is smaller. Now, take this case: <p> abc <textarea> def </textarea> ghi </p> ...which might look like this: ,---------------. | def | abc | | ghi `---------------' ...would change, if you added these styles: p { text-decoration: underline; } textarea { text-decoration: overline; } ...so that there would be a single underline from abc to ghi, and the text def would be struck out. The def text would not be underlined though. Note that this has nothing to do with special casing form elements. A normal block, inline-block, table, inline-table, or xul element in an inline element all have exactly the same effect. See also the recently published http://www.w3.org/TR/css3-text/#text-decoration

It is, however, a bug if we paint an *additional* text decoration inside form controls, which I presume is what's happening (since we have an anonymous div inside of textarea and input type=text).

Right. Similarly, a block within an underlined inline element should not be underlined. (Right? That's what the draft says. Is it what I said above? No, it's not. Bummer. Ok, how do we handle this...)

Oh, if that's true, then it's easy -- for a block, only walk up as long as the parent is a block.

From the draft: : If they are specified for a block-level element, it affects the root inline : element (the anonymous inline element which wraps all the inline children of : the block element). : : If they are specified for (or affects) an inline-level element, it affects all : boxes generated by the element. That seems to me to back what dbaron just said in comment 142. Which is fine. It makes it easy to implement. I'm not entirely sure if it's what authors expect though. Maybe we should take this back to the WG.

Walking up until the parent isn't a block frame anymore is what I always wanted to do ( ;-) ) Unfortunately, this doesn't solve my problem, since the frame where the text-decoration for a form control would reside, is INLINE, as it should be, BTW. (You can disregard my writing about the frame being having display: block as a tired man's blunderings :-[ ) So in this case, the above stategy will never yield a text-decoration. To avoid confusion, I'm talking about text-decoration specified for the form control, not for the surrounding context (which works fine.) I suppose this is due to an INLINE frame being expanded to an inline frame containing several box-frames which contain the context. If this happens in other other circumstances, maybe a general strategy for this situaiton could be developed. The strategy could be to walk up the tree until the _current_ frame is an inline frame, thus including this frame's text-decoration in the result.(is IFRAME such a situation?) Ian Hixie: The textInputFrame has display=NS_STYLE_DISPLAY_INLINE, while the children have display=NS_STYLE_DISPLAY_BLOCK.

Besides changing the code to walk up only to the first inline frame only, I've redone GetTextDecorations to walk up the frame tree, rather than the styleContext tree. This enabled me to do the textInputFrame special case in a much more canonical way, without relying on any "neccessary, but not sufficient" criteria. I've discovered something that doesn't work as espected: scroll frames are not slashed through with text-decoration: line-through, see e.g. Viewer Demo 5. But as this is IMHO a bordercase, and as this would probably be difficult to fix, I would like to open a separate bug on this. My patch doesn't change the current behaviour in this regard, and so I would rather fix the 99% cases. Hope this patch is acceptable :-) It really does improve the text-decoration handling, and I believe that my code and the code I stole should now be in alignment of it's context. Oh, one final thing: The bug about lineBox.mBounds.x=0 thing mentioned above has been marked INVALID, with no explanation outside "lineBox doesn't work like this." I've asked for a more detailed explanation, but that seems to be it.

Besides changing the code to walk up only to the first inline frame only, I've redone GetTextDecorations to walk up the frame tree, rather than the styleContext tree. This enabled me to do the textInputFrame special case in a much more canonical way, without relying on any "neccessary, but not sufficient" criteria. I've discovered something that doesn't work as espected: scroll frames are not slashed through with text-decoration: line-through, see e.g. Viewer Demo 5. But as this is IMHO a bordercase, and as this would probably be difficult to fix, I would like to open a separate bug on this. My patch doesn't change the current behaviour in this regard, and so I would rather fix the 99% cases. Hope this patch is acceptable :-) It really does improve the text-decoration handling, and I believe that my code and the code I stole should now be in alignment of it's context. Oh, one final thing: The bug about lineBox.mBounds.x=0 thing mentioned above has been marked INVALID, with no explanation outside "lineBox doesn't work like this." I've asked for a more detailed explanation, but that seems to be it.

This page breaks with my patch: http://velvet.net/~fun/mozilla/faq-1.0rc2.xml The underline of the "Back to top" links are not below. I will investigate tomorrow or tonight, perhaps.

The bug turned out to be that I had forgotten about padding and border. Shame on me. The new patch fixes this. The patch is IMO ready for review, if anybody can spare the time. If somebody can supply an test case of the more exotic cases, like inline tables, this would be great, too.

That patch doesn't build for me on Linux, CVS tip: nsBlockFrame.cpp:5456: no `void nsBlockFrame::PaintTextDecorationLines (nsIRenderingContext &, unsigned int, int, int, int)' member function declared in class `nsBlockFrame' make[1]: *** [nsBlockFrame.o] Error 1 make[1]: Leaving directory `/home/ianh/Mozilla/opt/mozilla/layout/html/base/src' make: *** [all] Error 2

Ian: I'm sorry, but I'm unable to either understand or reproduce it. The unfound function declaration reside in nsBlockFrame.h, which is included at the very buttom of the patch: + virtual void PaintTextDecorationLines(nsIRenderingContext& aRenderingContext, + nscolor color, + nscoord offset, + nscoord ascent, + nscoord size); I've just tried to make a new, clean build of mozilla and apply the patch, and it works perfectly. This is in Linux (SuSE 7.2). This is very frustrating, and unfortunately, cross-platform is a weak point for me (part of why I wanted to get involved in hacking mozilla in the first place.) Could I impose on you to check if this function declaration is actually in your patched version of nsBlockFrame.h? (in mozilla/layout/html/base/src, though you probably knew that.) If it is, could you post it here or send it to me together with the actual function call? If it isn't, could you try to apply the patch again? I'll make a new patch, too, in case this bug is due to the patch being old to be applied.

I see the problem. The declaration is hidden inside an |#ifdef DEBUG| section, and I'm building an optimised build.

Now I get a different error (below). I suggest getting a new tree, applying the patch, and trying to compile it as an optimised build. :-) nsBlockFrame.cpp: In method `nsresult nsBlockFrame::PlaceLine (nsBlockReflowState &, nsLineLayout &, nsLineList_iterator, PRBool *, int)': nsBlockFrame.cpp:4073: no matching function for call to `nsLineLayout::VerticalAlignLine (nsLineList_iterator &, nsSize &)' nsLineLayout.h:117: candidates are: void nsLineLayout::VerticalAlignLine (nsLineBox *, nsSize &, int &) make[4]: *** [nsBlockFrame.o] Error 1 make[4]: Leaving directory `/home/ianh/Mozilla/opt/mozilla/layout/html/base/src'

The optimized build works now. Turned out I was a shade too hasty when rebuilding the patch, and forgot a file :-((( This time I've: Wiped a build dir. Fetched mozilla. Patched mozilla. Build with debug. "make -f client.mk clean". Build with optimization (-O) and without debug. Both worked. So now I hope it works, this time. I'll add this procedure to my future "final" patches :-)

My optimised build will build, but it won't run... I get this error: nsNativeComponentLoader: GetFactory(/home/ianh/Mozilla/opt/mozilla/dist/bin/com ponents/libgklayout.so) Load FAILED with error: /home/ianh/Mozilla/opt/mozilla/ dist/bin/components/libgklayout.so: undefined symbol: PaintTextDecorationLines_ _20nsHTMLContainerFrameR19nsIRenderingContextUiiii I tried a complete clobber and rebuild, it didn't help. I'll attach my mozconfig, which may be the cause of the trouble.

It is not Hixie fault, it's mine, again :-( Somehow, I managed to enter the nsHTMLContainerFrame.h twice; and stupid me didn't think to test this far. I've done a complete rebuild to see that it all works, including a few testcases, using Hixies .mozconfig. I'm sorry to have caused all that bother for nothing. Note: The patch is pulled from my old tree again. Put it worked find on a CVS pull from yesterday. P.S: Mozilla really flies with that .mozconfig, doesn't it?

Yup, why d'you think I use it? :-) I'm building now.

Well, it seems to do well on several pages. However, it crashes on my main testcase: http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/underline.html As well as on this bug: http://bugzilla.mozilla.org/show_bug.cgi?id=1777 Other than that, it looks sweet. The form control handling in particular looks very nice, and the painting order seems to be correct.

Strange. I'm looking at these two pages now with a build from (too) early this morning, and no crash (obviously.) I'll do a CVS update and see if that, erhm, helps. I do hope I can reproduce this... Crashing, that's BAD. Could you also take a look at viewer demos #5 and tell me what you think? We should write that .mozconfig with huge letters somewhere :-D I wonder what options the milestones are built with.

No luck :-( Both pages show up nice and stunningly fast. Even bugzilla flies Sunday afternoon :-) Any ideas?

Hmm. I can't get either to crash in my debug build. Trying again with my opt build. BTW, I see no unexpected problems with Viewer #5 nor with my underlining evil test. I love it. :-)

It's not crashing in my opt build either now. I assume I made some stupid mistake while building it the first time. Sorry about the false alarm! Alright, qa=hixie on the patch. :-) I'll run with it to see if I see any problems.

Found this keyword :o) BTW: The cvs-build is quite bugridden at the moment, I get some random crashes. I even got paranoid to make a clean, optimized build, and it's the same. (I got one reproducible, and filed it as a bug.) Thanks to all of you have helped me stumble along this first patch :-) It's been a real experience :-)

Is the crash on http://off.net/~shaver/diary/ related to this patch?

No, I don't think so since my nightly 2002060808 without the patch also crashes.

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

I found a few bugs, but I haven't checked if they are specific to this patch or not yet. Here are a couple of test cases that fail: http://www.hixie.ch/tests/adhoc/compat/fonts/002.html http://www.hixie.ch/tests/adhoc/compat/fonts/003.html (The first spotted on theonion.org's "feature" sidebar, the second seen in various places, such as on satirewire.com's links on the left hand side.)

Well, neither bug occurs on the 1.0 branch, so I'm going to guess both are caused by this bug's patch. These bugs are, in my opinion, serious enough (in the first case) and common enough (in the second) to require further work before we check this patch in. Both are caused by our having to support compatability renderings. If only we could junk quirks mode altogether... :-(

First, from the standard on the FONT (to see what we're supposed to do in standardsmode): http://www.w3.org/TR/html401/present/graphics.html#edef-FONT "The FONT element changes the font size and color for text in its contents." Text decorations aren't mentioned here, so in standards mode, at least, the text-decoration should hold true to the containing <A> block. I see 3 problems with my patch in your textcases: 1) The text-decorations in constructions like <A><FONT size="1">Blah blah</FONT></A> are drawn twice, and may not overlap. I believe that the right solution for this would be to either a) REVERT to the old behaviour for quirksmode, and only use my code (without the FONT-quirk) with standards mode. b) Drop the quirk behaviour, that is, not handle FONT specially. 2) The text-decoration is much too heavy for the font. Solution for b) would reduce this to the "normal font-sized" underlining, which is IMHO the correct, but not quirklike, behaviour. Solution a) would give the size=1 underline weight in quirksmode and behave like b) in standardsmode. 3) The underlining is off. This shouldn't happen; but it does even with the FONT code turned off. I'll try to fix it one of these days. Some more thoughts: What should happen in cases like this? <A href="#top">Some people <font size="1">like different</font> sized fonts <font size="1> in their links</font></A> 1a) and 1b) gives quite different results here. I'm not sure what's supposed to happen. That said, there's something awfully wrong with the line-height on that last line; this bug isn't introduced by my patch, but it seems my code doesn't follow the bug :O) In conclusion: The text-decorations should at least use the same line-height as the rest of the layout engine on the last line. I'll look into this. Tell me what you think. I don't really have any opionion about quirksmode.

Ok, I'm stupid. The http://www.hixie.ch/tests/adhoc/compat/fonts/002.html renders as it does because the text-decoration code believes that the baseline of the <A> tag is at the <A>'s font ascent. In standards mode, this is true, but in quirks, it obviusly isn't. So if people here'll agree that reverting to the old behaviour/code for quirks mode is a good solution, then I won't have to fix this. (Actually, this will be pretty hard to fix in quirks mode as the <A> doesn't contain the neccesary information (the baseline) to draw the text-decoration. So, in this case I'll have to walk down the tree... like the 1a above.) Please advice! If I hear nothing in a week or so, I'll make a patch where quirk rendering is done by the old code and standard mode is done by the new code minus the FONT tag workaround. Maybe I should put this to discussion in the developer groups? If nothing else, a bit of developer discussion there may lend credence to the "this is a developer group only" banner :-D

I would be ok with using the current code in quirks mode, if that is what is easiest... I dunno, it's up to dbaron really.

There is a problem with using the old code for quirks mode: The CSS1 test suite renders incorrectly: http://www.w3.org/Style/CSS/Test/CSS1/current/sec543.htm This is because the DOCTYPE <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN" "http://www.w3.org/TR/REC-html40/loose.dtd"> triggers quirks-mode, and thus, get rendered incorrectly. What to do? Attached is a patch using the old textdecoration code for quirks mode and the new for standards. Should we evangalize w3.org? ;-)

Don't worry about the CSS1 test suite. :-)

In that case, the patch is ready for review. :-D Thanks, Ian. Please don't blame me too much for the code in nsTextFrame::PaintTextDecorations()... I didn't clean it up beyond linebreaking and such :o) However, I indented half the function..

looks good to me; qa=hixie

So could you briefly explain why it's *not possible* to use the new code for the quirk? I'm not enthusiastic about this big a code fork, since we know the standards-mode code won't get tested well.

If you revert the old code for quirks mode, the bug will appear in most pages, as they are open in quirks mode by default. Mozilla should have an option to force page rendering in quirks/standard mode (I already suggested that).

I don't think it is impossible to do (mostly anything). I was merely fishing for a recommended solution to the quirks problem :-) Here's the alternatives, as I see it. 1. Keep the old textdecoration layout code for quirks mode. Pros: Doesn't break any quirks, easy to do. Con: forks the code, new code has low visibility. 2. Work around the problem. This would involve walking down the tree, finding any FONT tags, get their color, and correct the color of the text-decoration. Furthermore, the line height must be recalculated to the "quirk lineheight" in these cases (this is probably diffucult). We'll also need a decision as to which font ascent to use, see below. 3. Correct LineBox. This involves making the FONT tag correct the lineboxes in quirk mode so that LineBox reports the correct height. Then I would still need to work down the tree, find each FONT tag, extract the beginning and the ending horizontal coordinates, and (re)draw the font decoration in the appropriate color. Also, see below about the font ascent. Pro: The data in lineBox would get useful again, the solution is IMHO the "right" solution. Con: It might a lot of work, it might break some other code which depends on the quirky LineBox behaviour. 4. Ignore the quirks problem: According to Ian Hixie, this causes some highly visible layout errors. Ignoring the FONT quirk would make 3 easier, but not unneccessary, as the lineheight on the LineBox'es would still be incorrect and throw the linedrawing off. Pro: Easy. Con: Breaks pages. It also not clear what font ascent to use, really. Consider <P class="overlined"><FONT size=2>I like to <FONT size=20>shout!</FONT><FONT> I'll attach an image illustrating the dilemma. Conclusion: I take it from Ian's comments that 4 is not an option. I also take it from Davids comment that 1 is not a viable option. That leaves 2 and 3. I really don't want to implement 2, but if you (David) want me to implement 3, I will do it.... e.g. in bug 145467. Then this one could be marked dependant on the 145467, and the "right" solution could be made. I would recommmend this bug for 1.2, since it would need at least some testing before it's ready for a milestone....

In the picture, 1: Using the ascent of the biggest font/the font which determines the lineheight to draw the overline. 2: Using the ascent of the first font to draw the overline. 3: The way it is currently done.

Comment on attachment 88644 [details] [diff] [review] Reverting to old code for quirks mode. >+ if (!useOverride && (NS_STYLE_TEXT_DECORATION_OVERRIDE_ALL >+ & styleText->mTextDecoration)) >+ { This is one of a number of places in the code that you reindented where you changed the bracing style to move the brace onto the next line. Layout code typically uses brace on the same line, so could you leave it that way? Also, expressions like this are probable more cleanly written as: if (!useOverride && (NS_STYLE_TEXT_DECORATION_OVERRIDE_ALL & styletext->mTextDecoration)) { (perhaps breaking after the & as well). >- PaintTextDecorations(aRenderingContext, aStyleContext, >- aTextStyle, dx, dy, width, text, details,0,(PRUint32)textLength); >+ PaintTextDecorations(aRenderingContext, aStyleContext,aPresContext, >+ aTextStyle, dx, dy, width, text, details,0,(PRUint32)textLength); Could you leave things like this so that the arguments line up? >@@ -3049,12 +3074,14 @@ > > if (isPaginated && !iter.IsBeforeOrAfter()) { > aRenderingContext.SetColor(nsCSSRendering::TransformColor(aTextStyle.mColor->mColor,canDarkenColor)); >- RenderString(aRenderingContext,aStyleContext, aTextStyle, currenttext, >- currentlength, currentX, dy, width, details); >+ RenderString(aRenderingContext,aStyleContext, aPresContext, >+ aTextStyle, currenttext, currentlength, >+ currentX, dy, width, details); > } else if (!isPaginated) { > aRenderingContext.SetColor(nsCSSRendering::TransformColor(currentFGColor,canDarkenColor)); >- RenderString(aRenderingContext,aStyleContext, aTextStyle, currenttext, >- currentlength, currentX, dy, width, details); >+ RenderString(aRenderingContext,aStyleContext, aPresContext, >+ aTextStyle, currenttext, currentlength, currentX, >+ dy, width, details); > } > > //increment twips X start but remember to get ready for next draw by reducing current x by letter spacing amount Again, it would be nice if the arguments lined up. Also, getting rid of the tabs is good, but there are a few that you missed on some of these RenderString lines. When you're changing code anyway, it's good to get rid of the tabs. :-) It's also nice to provide an alternative diff -uw patch (in addition to the diff -u) to make the parts that you reindented easier to review. (For the previous patch, though, there's no need, since I did it myself.) I'll try and add some more substantive comments soon. Sorry for the delay...

Comment on attachment 88644 [details] [diff] [review] Reverting to old code for quirks mode. (nsTextFrame::PaintTextDecorations) >+ // Quirks mode font decoration are rendered by children; see bug 1777 >+ nsCompatibility mode; >+ aPresContext->GetCompatibilityMode(&mode); >+ if (eCompatibility_NavQuirks == mode) { This comment should refer to nsHTMLContainerFrame::Paint, which does the painting for non-quirks mode. >+ kid->IsEmpty((mode == eCompatibility_NavQuirks), isPre, &isEmpty); On the current trunk, |(mode == eCompatibility_NavQuirks)| should be replaced by |mode|. See bug 153032. (nsHTMLContainerFrame::Paint) + if (eCompatibility_Standard == mode && + NS_FRAME_PAINT_LAYER_FOREGROUND == aWhichLayer + && frameType == nsLayoutAtoms::inlineFrame) { 1) There should be a comment here pointing to nsTextFrame::PaintTextDecorations. 2) Thanks to bug 153032, this needs to be |eCompatibilityNavQuirks != mode| to be symmetric with nsTextFrame::PaintTextDecorations

If you fix the issues in comment 181 and comment 182, you'll have r=dbaron. (I'm trusting you that you've addressed the issues in my previous comments. I didn't go through the details of all the new code again.) Once you fix those, you'll need to get super-review (see http://mozilla.org/hacking/reviewers.html ). A good super-reviewer for this would probably be bzbarsky.

I've updated the patch as per your comments. In answer to your question, I have updated my patch with your other comments during previous iterations. Thank you for reviewing the patch :-) I'll attach a -uw diff shortly; then I'll see about a super-review tomorrow.

Adding patch /sans/ whitespace, as requested.

Did you send email asking for super-review?

Sorry, my bad. I was under the impression form http://mozilla.org/hacking/reviewers.html that I should wait for the r=<email> thingy. The letter email has been sent a few seconds ago. Thanks for your time! :-D

Comment on attachment 92046 [details] [diff] [review] Latest patch updated with David Baron's comments Still one whitespace issue: >@@ -3217,8 +3245,8 @@ > // simplest rendering approach > aRenderingContext.SetColor(nsCSSRendering::TransformColor(aTextStyle.mColor->mColor,canDarkenColor)); > aRenderingContext.DrawString(text, PRUint32(textLength), dx, dy + mAscent); >- PaintTextDecorations(aRenderingContext, aStyleContext, aTextStyle, >- dx, dy, width); >+ PaintTextDecorations(aRenderingContext, aStyleContext, >+ aPresContext, aTextStyle, dx, dy, width); > } > else { > SelectionDetails *details; >@@ -126,11 +348,40 @@ > return NS_OK; > } > >+ // Paint underlines+overlines behind children, but line-throughs in front >+ // of children. See bug 1777 for details. >+ // This for is standards mode only. For Quirks mode, see >+ // nsTextFrame::PaintTextDecorations. >+ nscolor underColor, overColor, strikeColor; >+ PRUint8 decorations = NS_STYLE_TEXT_DECORATION_NONE; >+ nsCompatibility mode; >+ aPresContext->GetCompatibilityMode(&mode); >+ if (mode && >+ NS_FRAME_PAINT_LAYER_FOREGROUND == aWhichLayer && >+ frameType == nsLayoutAtoms::inlineFrame) { Eh? This mode check is definitely unclear, if not wrong. Don't rely on a named enum value being zero or nonzero. >+ // Paint textdecorations: under+overline behind children, line-through >+ // in front. >+ // This for is standards mode only. For Quirks mode, see >+ // nsTextFrame::PaintTextDecorations. >+ PRUint8 decorations = NS_STYLE_TEXT_DECORATION_NONE; >+ nscolor underColor, overColor, strikeColor; >+ nsCompatibility mode; >+ aPresContext->GetCompatibilityMode(&mode); >+ if (mode && >+ aWhichLayer == NS_FRAME_PAINT_LAYER_FOREGROUND && !mLines.empty()) { Same for this one.

Suggestion above implemented. Sorry for being unable to read english :-[

Comment on attachment 93061 [details] [diff] [review] Patch for this bug and bug 20163. This is the full patch version. This still has an incorrect mode check in nsTextFrame.cpp.

Comment on attachment 93062 [details] [diff] [review] As 93071, but without whitespace differences. Meant for reviews only. You use a lot of weird indentations, like sometimes in the patch you use 4 spaces, and sometimes 2, and sometimes you don't indent the code under a if() at all. I assume that you have fixed all of this (to comply with the current coding style!) in the whitespace-sensitive patch. Please be consistent. :-) Now for some nits. You can combine the UNDERLINE and OVERLINE cases since they call PaintTextDecorationLines() with the same arguments, and you already made the checks one time above... >+ if (decoration & >+ (NS_STYLE_TEXT_DECORATION_UNDERLINE >+ | NS_STYLE_TEXT_DECORATION_OVERLINE)) { >+ normalFont->GetUnderline(offset, size); >+ if (NS_STYLE_TEXT_DECORATION_UNDERLINE & decoration) { >+ PaintTextDecorationLines(aRenderingContext, color, offset, ascent, size); >+ } >+ else if (NS_STYLE_TEXT_DECORATION_OVERLINE & decoration) { >+ PaintTextDecorationLines(aRenderingContext, color, ascent, ascent, size); >+ } >+ } Same here. >+ GetTextDecorations(aPresContext, PR_FALSE, &decorations, &underColor, >+ &overColor, &strikeColor); >+ if (decorations & NS_STYLE_TEXT_DECORATION_UNDERLINE) { >+ PaintTextDecorations(aPresContext, aRenderingContext, >+ NS_STYLE_TEXT_DECORATION_UNDERLINE, underColor); >+ } >+ if (decorations & NS_STYLE_TEXT_DECORATION_OVERLINE) { >+ PaintTextDecorations(aPresContext, aRenderingContext, >+ NS_STYLE_TEXT_DECORATION_OVERLINE, overColor); >+ } >+ } >+

Please don't comment on whitespace in a diff -w. The two cases quoted above pass different arguments. There might be an advantage to using ?:, but that sometimes tends to make the code unclear.

Corrected the missed mode check. I disagree with joining the two if's as the parameters differ. (offset vs. ascent). Admittedly, I could trade one integer comparison for a little bloat by moving the font thingy inside each if's, but I see little gain there. I must admit that emacs is the one that indent my code mostly. I've tried to going over my code looking for the issues mentioned, but I havn't found anything. It's probably me who are blind :-/ Whitespace patch to follow...

Comment on attachment 94213 [details] [diff] [review] -w patch: Same as 94210, except whitespace difference are ignored. For review only. >+ // The ascent of the linebox, i.e. the distance from the top to the baseline >+ // of the (initial) font: >+ nscoord mAscent; Use javadoc syntax? So: /** (two stars, important!) * The ascent .... */ >+ if (!useOverride && >+ (NS_STYLE_TEXT_DECORATION_OVERRIDE_ALL >+ & styleText->mTextDecoration)) { Keep the two things being &-ed on the same line? It's not too long, as far as I can tell.... >+ PaintTextDecorations(aRenderingContext, aStyleContext,aPresContext, >+ aTextStyle, dx, dy, width, text, details,0, space before the '0'? >+ * Input: PRBool isBlock - whether _this_ is a block frame or no. @param isBlock whether ... @param pDecorations [OUT] the decorations to apply. Will be set to NS_STYLE_.... @param pUnderColor [OUT] the color of the underline or null if there is no underline And so forth. If the colors will _not_ be set to null when the appropriate decoration is not applied, the comment should say that. Finally, is there a good reason to use nscolor* instead of nscolor& ? Use aFoo instead of pFoo for arguments to functions, please ('a' for 'argument'). "isBlock" should be "aIsBlock". >+ /** >+ * Function that does the actual drawing of the textdecoration. >+ * input: >+ * aRenderingContext. >+ * color: the color of the text-decoration >+ * ascent: ascent of the font from which the text-decoration was derived. >+ * offset: distance *above* baseline where the text-decoration should be >+ * drawn, i.e. negative offsets draws *below* the baseline. >+ * size: the thickness of the line >+ */ Again, use @param syntax for the params. >+ if (!mStyleContext->HasTextDecorations()) { >+ //This is a neccessary, but not sufficient, condition for textdecorations. >+ return; >+ } Should this not set *pDecorations to something useful before returning? And null out the other pointers? + if (frame) { >+ /* >+ * Special case: text form controls. >+ */ So... since we broke out of the loop above and |frame| is non-null, decorMask must be null. >+ PRUint8 decors = decorMask & styleText->mTextDecoration; Then how is this supposed to do anything useful? In any case, please have a slightly more detailed explanation of why text inputs are special. >+ parent->FirstChild(aPresContext, nsnull, &kid); >+ nsCOMPtr<nsIAtom> frameType; >+ >+ for (parent->FirstChild(aPresContext, nsnull, &kid); kid; That first FirstChild call can be removed, no? HasTextFrameChild() seems to be potentially expensive... Any decent way of caching that information so it does not have to be recomputed on every Paint()? We could invalidate the cached info on content changes..... The rest looks good to me so far, but I'm still working on grokking this fully.

I'm really sorry, but my Linux HD crashed a few days ago. I'll look into it when I can, but I have no timeframe yet :-( Good comments, though. I'll certainly make the corrections when I can. And look into caching the HasTextFrameChild thing. It could be expensive if a large subtree with no TextFrameChild is encountered, yes.