Last Comment Bug 1777 - [TEXT] 'text-decoration' should not be drawn by children (underline) [INLINE]
: [TEXT] 'text-decoration' should not be drawn by children (underline) [INLINE]
Status: VERIFIED FIXED
[patch][Hixie-P1][CSS1-5.4.3] (high p...
: css1, testcase
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: All All
: P3 normal with 5 votes (vote)
: mozilla1.3beta
Assigned To: Esben Mose Hansen
: Hixie (not reading bugmail)
Mentors:
http://www.w3.org/Style/CSS/Test/curr...
: 3488 10065 44232 44420 47749 64207 121760 142407 144596 150949 (view as bug list)
Depends on:
Blocks: 35146 20163 59109 103709 104166 121760 126259 181336
  Show dependency treegraph
 
Reported: 1998-12-04 15:44 PST by glynn
Modified: 2014-04-26 03:30 PDT (History)
39 users (show)
dbaron: blocking1.3a-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Simplest testcase for CSS and HTML versions of the bug (341 bytes, text/html)
1999-07-02 22:19 PDT, Kenny Stoltz
no flags Details
underlining and images mixed up (742 bytes, text/html)
1999-09-20 11:23 PDT, kipp
no flags Details
Reference image for attachment #1772. (14.67 KB, image/gif)
1999-09-23 01:54 PDT, Hixie (not reading bugmail)
no flags Details
What it looks like when text-decoration drawn by children (4.99 KB, image/gif)
2000-06-05 07:09 PDT, Stephen Ostermiller
no flags Details
as seen in Opera (3.58 KB, image/gif)
2002-01-01 09:57 PST, rbs
no flags Details
Extended example in Opera (1.25 KB, image/gif)
2002-01-01 13:29 PST, rbs
no flags Details
simplified testcase for http://www.slashdot.org problem (846 bytes, text/html)
2002-01-30 19:53 PST, Christine Hoffman
no flags Details
testcase used for Opera' screenshots (1.60 KB, text/html)
2002-04-22 14:16 PDT, rbs
no flags Details
1st patch attempt. Should fix 20163, too. (17.37 KB, patch)
2002-04-26 13:26 PDT, Esben Mose Hansen
no flags Details | Diff | Splinter Review
Picture to be used with the following testcase... (5.01 KB, image/png)
2002-04-26 15:42 PDT, Esben Mose Hansen
no flags Details
Another testcase. Underline only. (1.58 KB, text/html)
2002-04-26 15:52 PDT, Esben Mose Hansen
no flags Details
take 2. Now handles white-space only textFrames appropriately (18.06 KB, patch)
2002-04-27 04:08 PDT, Esben Mose Hansen
no flags Details | Diff | Splinter Review
testcase showing incorrect behavior with right alignment (450 bytes, text/html)
2002-04-27 09:51 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details
testcase showing incorrect behavior with form controls (565 bytes, text/html)
2002-04-27 09:51 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details
testcase showing problems / inconsistencies in painting order (871 bytes, text/html)
2002-04-27 10:00 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details
Take 3. Now does text-alignment, painting-layers and form elements right. I hope :) (19.56 KB, patch)
2002-05-06 13:22 PDT, Esben Mose Hansen
no flags Details | Diff | Splinter Review
Take 4. This patch is final (until otherwise proven :o) ) (19.68 KB, patch)
2002-05-10 09:07 PDT, Esben Mose Hansen
no flags Details | Diff | Splinter Review
Testcase for font-color overrides text-decoration color quirk. (254 bytes, text/html)
2002-05-10 14:03 PDT, Esben Mose Hansen
no flags Details
Testcase covering issues from comment 122 and comment 83 (1.22 KB, text/html)
2002-05-18 13:28 PDT, Esben Mose Hansen
no flags Details
Take 5:Final., unless otherwise proven. (17.37 KB, patch)
2002-05-18 13:48 PDT, Esben Mose Hansen
no flags Details | Diff | Splinter Review
Take 6. Redesign to walk up only to first inline frame. (24.42 KB, patch)
2002-05-25 00:15 PDT, Esben Mose Hansen
no flags Details | Diff | Splinter Review
Take 6. Redesign to walk up only to first inline frame. (24.42 KB, patch)
2002-05-25 00:48 PDT, Esben Mose Hansen
no flags Details | Diff | Splinter Review
Take 7: Now handles padding and borders in inline frames correctly (24.53 KB, patch)
2002-05-30 11:15 PDT, Esben Mose Hansen
no flags Details | Diff | Splinter Review
Same as take 7, but from a newer build (2002-06-08) (23.95 KB, patch)
2002-06-08 01:34 PDT, Esben Mose Hansen
no flags Details | Diff | Splinter Review
The same patch as 86924 above, but with the missing file (nsLineLayout.h) (13.59 KB, patch)
2002-06-08 11:01 PDT, Esben Mose Hansen
no flags Details | Diff | Splinter Review
Hixie's mozconfig, with which the last patch doesn't run (878 bytes, text/plain)
2002-06-08 16:22 PDT, Hixie (not reading bugmail)
no flags Details
As above, but now it compiles AND runs in optimized builds (24.78 KB, patch)
2002-06-09 00:03 PDT, Esben Mose Hansen
no flags Details | Diff | Splinter Review
Reverting to old code for quirks mode. (34.96 KB, patch)
2002-06-21 08:31 PDT, Esben Mose Hansen
no flags Details | Diff | Splinter Review
Picture describing three possible ways to overline in quirks mode. (1.89 KB, image/png)
2002-07-02 10:04 PDT, Esben Mose Hansen
no flags Details
Latest patch updated with David Baron's comments (35.27 KB, patch)
2002-07-19 17:23 PDT, Esben Mose Hansen
no flags Details | Diff | Splinter Review
As 92046/The latest patch, but /sans/ changes in whitespace. Don't check this version into CVS, please. (32.09 KB, patch)
2002-07-19 17:27 PDT, Esben Mose Hansen
no flags Details | Diff | Splinter Review
Patch for this bug and bug 20163. This is the full patch version. (35.33 KB, patch)
2002-07-28 01:49 PDT, Esben Mose Hansen
no flags Details | Diff | Splinter Review
As 93071, but without whitespace differences. Meant for reviews only. (32.15 KB, patch)
2002-07-28 01:51 PDT, Esben Mose Hansen
no flags Details | Diff | Splinter Review
Patch for this bug and bug 20163. This is the full patch version. (35.35 KB, patch)
2002-08-06 12:09 PDT, Esben Mose Hansen
no flags Details | Diff | Splinter Review
-w patch: Same as 94210, except whitespace difference are ignored. For review only. (32.17 KB, patch)
2002-08-06 12:13 PDT, Esben Mose Hansen
no flags Details | Diff | Splinter Review
Latest patch, full version (36.28 KB, patch)
2002-09-16 13:01 PDT, Esben Mose Hansen
no flags Details | Diff | Splinter Review
Latest patch, -w version, for review /only/. (33.10 KB, patch)
2002-09-16 13:02 PDT, Esben Mose Hansen
no flags Details | Diff | Splinter Review
Latest patch, including (most of) Boris Zbarsky comments (36.23 KB, patch)
2002-10-02 13:41 PDT, Esben Mose Hansen
bzbarsky: superreview+
Details | Diff | Splinter Review
as 101439, but without whitespace diffs. *For review only*. (33.04 KB, patch)
2002-10-02 13:44 PDT, Esben Mose Hansen
no flags Details | Diff | Splinter Review
Latest patch including the last of Boris Zbarsky comments. (36.36 KB, patch)
2002-10-05 23:37 PDT, Esben Mose Hansen
bzbarsky: superreview+
Details | Diff | Splinter Review
as 101870, but without whitespace diffs. *For review only*. (33.17 KB, patch)
2002-10-05 23:38 PDT, Esben Mose Hansen
no flags Details | Diff | Splinter Review
Merge conflicts resolved; textual nonsense removed (36.04 KB, patch)
2002-10-15 11:09 PDT, Esben Mose Hansen
bzbarsky: superreview+
Details | Diff | Splinter Review
Whitespace-less patch. For review only. (33.05 KB, patch)
2002-10-15 11:10 PDT, Esben Mose Hansen
no flags Details | Diff | Splinter Review
testcase for text-decoration on text inputs (1.35 KB, text/html)
2002-12-06 05:22 PST, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details
patch, with dbaron's modifications (52.03 KB, patch)
2002-12-06 12:51 PST, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
bzbarsky: superreview+
Details | Diff | Splinter Review

Description glynn 1998-12-04 15:44:20 PST
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
Comment 1 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 1998-12-20 13:59:59 PST
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.
Comment 2 leger 1999-02-03 08:08:59 PST
Setting all current Open/Normal to M4.
Comment 3 Paul MacQuiddy 1999-03-05 22:38:59 PST
per leger, assigning QA contacts to all open bugs without QA contacts according
to list at http://bugzilla.mozilla.org/describecomponents.cgi?product=Browser
Comment 4 Hixie (not reading bugmail) 1999-05-12 08:06:59 PDT
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.
Comment 5 Hixie (not reading bugmail) 1999-05-17 16:17:59 PDT
See also bug 3488.
Comment 6 Hixie (not reading bugmail) 1999-05-26 15:23:59 PDT
See also bug 1519.
Comment 7 Kenny Stoltz 1999-07-02 22:19:59 PDT
Created attachment 679 [details]
Simplest testcase for CSS and HTML versions of the bug
Comment 8 Kenny Stoltz 1999-07-02 22:22:59 PDT
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.
Comment 9 Hixie (not reading bugmail) 1999-07-03 05:16:59 PDT
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.
Comment 10 kipp 1999-09-20 11:23:59 PDT
I believe that implementing underline this way will lead to a large number of
compatability problems. See the new attached test case. Comments please...
Comment 11 kipp 1999-09-20 11:23:59 PDT
Created attachment 1772 [details]
underlining and images mixed up
Comment 12 kipp 1999-09-20 12:55:59 PDT
*** Bug 3488 has been marked as a duplicate of this bug. ***
Comment 13 Hixie (not reading bugmail) 1999-09-23 01:54:59 PDT
Created attachment 1830 [details]
Reference image for attachment #1772 [details].
Comment 14 Hixie (not reading bugmail) 1999-09-23 02:06:59 PDT
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.
Comment 15 Hixie (not reading bugmail) 1999-11-17 08:57:59 PST
*** Bug 10065 has been marked as a duplicate of this bug. ***
Comment 16 Hixie (not reading bugmail) 1999-11-17 09:01:59 PST
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
Comment 17 leger 1999-12-14 13:53:59 PST
Updating to default Layout Assignee...kipp no longer with us :-(
Comment 18 troy 1999-12-14 14:15:59 PST
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.
Comment 19 Hixie (not reading bugmail) 1999-12-23 13:56:59 PST
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.
Comment 20 Hixie (not reading bugmail) 2000-01-13 15:59:59 PST
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...
Comment 21 ekrock's old account (dead) 2000-01-21 13:51:48 PST
Bulk moving [testcase] code to new testcase keyword. Sorry for the spam!
Comment 22 Matthew Brealey 2000-01-27 07:33:58 PST
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.
Comment 23 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2000-01-27 08:08:31 PST
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?
Comment 24 buster 2000-03-03 16:08:55 PST
mine! mine mine mine!  all mine!  whoo-hoo!
Comment 25 buster 2000-06-01 11:44:04 PDT
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.
Comment 26 Stephen Ostermiller 2000-06-05 07:09:20 PDT
Created attachment 9622 [details]
What it looks like when text-decoration drawn by children
Comment 27 ekrock's old account (dead) 2000-06-05 15:14:00 PDT
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.
Comment 28 leger 2000-06-05 17:38:18 PDT
Putting on [nsbeta2+] [will be minus on 6/15] radar.
Comment 29 ekrock's old account (dead) 2000-06-14 19:08:58 PDT
Nominating nsbeta2,nsbeta3,rtm. Recc. nsbeta2+, falling through to nsbeta3+ and 
rtm+ if necessary. This is a W3C CSS1 Official Test Suite compliance bug.
Comment 30 ekrock's old account (dead) 2000-06-14 19:38:50 PDT
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.
Comment 31 ckritzer (gone) 2000-06-15 13:21:10 PDT
PDT: please leave nsbeta2+.  Standards compliance issue.
Comment 32 ckritzer (gone) 2000-06-15 13:22:19 PDT
Sorry, I was going to reference ekrock's 2000-06-14 19:08 comments...
Comment 33 Jim Roskind 2000-06-17 11:08:51 PDT
6/15 has passed... changing to nsbeta2 minus
Comment 34 Martin Horwath 2000-07-03 02:37:08 PDT
*** Bug 44420 has been marked as a duplicate of this bug. ***
Comment 35 Hixie (not reading bugmail) 2000-07-26 11:28:35 PDT
As per meeting with ChrisD yesterday, taking QA.
Comment 36 Hixie (not reading bugmail) 2000-08-09 23:00:25 PDT
*** Bug 47749 has been marked as a duplicate of this bug. ***
Comment 37 Frank Tang 2000-08-15 15:05:52 PDT
nsbeta3+ P3 per bug meeting (ekrock)
Comment 38 Jaime Rodriguez, Jr. 2000-08-24 14:24:14 PDT
Adding buster to cc: list.
Comment 39 Hixie (not reading bugmail) 2000-08-26 01:08:24 PDT
*** Bug 50410 has been marked as a duplicate of this bug. ***
Comment 40 Chris McClelland 2000-08-26 01:22:19 PDT
When is the going to be fixed???
Comment 41 Chris McClelland 2000-08-28 20:26:24 PDT
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
Comment 42 Hixie (not reading bugmail) 2000-08-28 21:55:13 PDT
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! ;-)
Comment 43 Erik van der Poel 2000-08-29 10:00:58 PDT
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?
Comment 44 Matthew Paul Thomas 2000-08-29 10:35:00 PDT
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?
Comment 45 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2000-08-29 11:34:44 PDT
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.
Comment 46 ekrock's old account (dead) 2000-08-29 16:30:26 PDT
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.
Comment 47 Hixie (not reading bugmail) 2000-08-29 16:57:56 PDT
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.
Comment 48 Christine Hoffman 2000-11-07 15:45:19 PST
Testing on 11_06 branch builds, Linux and Mac look okay. Underlining is
consistent and not bolded.
Comment 49 Chris McClelland 2000-11-09 14:44:49 PST
Still seeing this on Windows 98 with the Nov 9 daily build. What could have 
changed in the Linux and Mac builds?
Comment 50 Hixie (not reading bugmail) 2000-11-09 17:31:53 PST
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.
Comment 51 Chris McClelland 2000-11-11 11:46:49 PST
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.
Comment 52 timeless 2000-11-11 23:03:26 PST
No. And Mozilla is not Netscape6 so the claims that Mozilla will be complaint 
are not affected by Netscape6's standards support.
Comment 53 Chris McClelland 2000-11-12 00:32:23 PST
Thanks for the information
Comment 54 Asa Dotzler [:asa] 2001-01-03 13:01:49 PST
*** Bug 64207 has been marked as a duplicate of this bug. ***
Comment 55 Hixie (not reading bugmail) 2001-01-14 21:43:36 PST
BTW, per the SF F2F meeting, the working group agree that our interpretation is 
correct. (This will be made explicit in CSS3.)
Comment 56 Håkan Waara 2001-02-09 15:55:58 PST
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?
Comment 57 Hixie (not reading bugmail) 2001-02-15 23:36:52 PST
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... ;-)
Comment 58 Frank Tang 2001-05-07 23:43:04 PDT
erik resign. reassign all his bug to ftang for now.
Comment 59 Frank Tang 2001-05-08 00:15:51 PDT
assigned as future
Comment 60 Hixie (not reading bugmail) 2001-05-22 23:40:08 PDT
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)
Comment 61 David Hyatt 2001-05-23 01:48:52 PDT
Taking.
Comment 62 David Hyatt 2001-05-23 01:49:32 PDT
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.
Comment 63 Gervase Markham [:gerv] 2001-06-10 01:20:33 PDT
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
Comment 64 basic 2001-10-09 23:15:39 PDT
is this still a highrisk?
Comment 65 rbs 2001-12-25 21:36:12 PST
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>
Comment 66 rbs 2002-01-01 09:57:53 PST
Created attachment 63182 [details]
as seen in Opera

Looks more like strike-through than overline to me
Comment 67 Hixie (not reading bugmail) 2002-01-01 12:27:41 PST
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.
Comment 68 rbs 2002-01-01 13:29:28 PST
Created attachment 63191 [details]
Extended example in Opera

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
      <sup>
	<span>
	  Hi
	  <img src="http://www.w3.org/Style/CSS/Test/current/oransqr.gif">
	</span>
      </sup>
    </span>
  </span>
</span>
End
Comment 69 Hixie (not reading bugmail) 2002-01-02 02:14:41 PST
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.
Comment 70 rbs 2002-01-02 10:43:49 PST
>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.
Comment 71 Hixie (not reading bugmail) 2002-01-03 02:44:36 PST
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
Comment 72 Kevin McCluskey (gone) 2002-01-29 11:48:49 PST
*** Bug 121760 has been marked as a duplicate of this bug. ***
Comment 73 Christine Hoffman 2002-01-30 19:38:35 PST
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.

Comment 74 Christine Hoffman 2002-01-30 19:53:06 PST
Created attachment 67207 [details]
simplified testcase for http://www.slashdot.org problem
Comment 75 Christine Hoffman 2002-01-30 19:53:46 PST
Is this really a futured bug?
Comment 76 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2002-04-02 11:57:10 PST
This shouldn't be hard to fix to get Hixie's desired behaviour, it can all be
done in the inline container frames.
Comment 77 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2002-04-03 06:06:31 PST
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.
Comment 78 Hixie (not reading bugmail) 2002-04-06 16:48:26 PST
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.
Comment 79 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2002-04-06 18:39:50 PST
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.
Comment 80 Hixie (not reading bugmail) 2002-04-07 15:37:35 PDT
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.
Comment 81 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2002-04-07 16:13:18 PDT
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.
Comment 82 Hixie (not reading bugmail) 2002-04-07 17:19:44 PDT
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.
Comment 83 Hixie (not reading bugmail) 2002-04-13 01:09:42 PDT
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.
Comment 84 Esben Mose Hansen 2002-04-21 12:03:50 PDT
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.)
Comment 85 Brendan Eich [:brendan] 2002-04-21 13:19:22 PDT
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
Comment 86 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2002-04-21 15:16:18 PDT
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.
Comment 87 Esben Mose Hansen 2002-04-22 09:21:23 PDT
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
Comment 88 Esben Mose Hansen 2002-04-22 12:24:47 PDT
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? 
Comment 89 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2002-04-22 13:47:11 PDT
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!!!
Comment 90 rbs 2002-04-22 14:16:50 PDT
Created attachment 80448 [details]
testcase used for Opera' screenshots
Comment 91 Esben Mose Hansen 2002-04-26 13:26:11 PDT
Created attachment 81201 [details] [diff] [review]
1st patch attempt. Should fix 20163, too.

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.
Comment 92 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-04-26 14:10:22 PDT
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.
Comment 93 Hixie (not reading bugmail) 2002-04-26 15:15:19 PDT
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.
Comment 94 Esben Mose Hansen 2002-04-26 15:42:22 PDT
Created attachment 81224 [details]
Picture to be used with the following testcase...
Comment 95 Esben Mose Hansen 2002-04-26 15:52:05 PDT
Created attachment 81228 [details]
Another testcase. Underline only.

The testcase I've used under development, in addition to those supplied in this
bug. See the comment below.
Comment 96 Esben Mose Hansen 2002-04-26 15:54:26 PDT
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 >:-}
Comment 97 Esben Mose Hansen 2002-04-27 00:42:29 PDT
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: 
Comment 98 Esben Mose Hansen 2002-04-27 04:08:56 PDT
Created attachment 81273 [details] [diff] [review]
take 2. Now handles white-space only textFrames appropriately

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.
Comment 99 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-04-27 09:51:14 PDT
Created attachment 81294 [details]
testcase showing incorrect behavior with right alignment
Comment 100 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-04-27 09:51:39 PDT
Created attachment 81295 [details]
testcase showing incorrect behavior with form controls
Comment 101 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-04-27 09:52:37 PDT
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.
Comment 102 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-04-27 10:00:17 PDT
Created attachment 81296 [details]
testcase showing problems / inconsistencies in painting order
Comment 103 Hixie (not reading bugmail) 2002-04-27 16:48:31 PDT
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! :-)
Comment 104 Esben Mose Hansen 2002-04-28 05:19:34 PDT
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)
Comment 105 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-04-28 07:55:59 PDT
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.
Comment 106 Christian Schmidt 2002-05-05 12:13:43 PDT
*** Bug 142407 has been marked as a duplicate of this bug. ***
Comment 107 Esben Mose Hansen 2002-05-06 13:22:29 PDT
Created attachment 82518 [details] [diff] [review]
Take 3. Now does text-alignment, painting-layers and form elements right. I hope :)

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.
Comment 108 timeless 2002-05-09 09:05:48 PDT
reassign to patch author
Comment 109 Esben Mose Hansen 2002-05-10 02:36:36 PDT
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 110 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-05-10 09:02:02 PDT
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?)
Comment 111 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-05-10 09:06:14 PDT
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.)
Comment 112 Esben Mose Hansen 2002-05-10 09:07:09 PDT
Created attachment 83043 [details] [diff] [review]
Take 4. This patch is final (until otherwise proven :o) )

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.
Comment 113 Esben Mose Hansen 2002-05-10 09:08:43 PDT
Updating keywords to reflect status...
Comment 114 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-05-10 09:19:25 PDT
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.
Comment 115 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-05-10 09:26:18 PDT
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.
Comment 116 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-05-10 09:28:05 PDT
... 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?
Comment 117 Esben Mose Hansen 2002-05-10 14:03:18 PDT
Created attachment 83090 [details]
Testcase for font-color overrides text-decoration color quirk.

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.
Comment 118 Esben Mose Hansen 2002-05-11 06:18:22 PDT
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 :)
Comment 119 Håkan Waara 2002-05-11 06:42:49 PDT
> 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.
Comment 120 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-05-11 07:48:02 PDT
> 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.
Comment 121 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-05-11 07:57:05 PDT
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.
Comment 122 Hixie (not reading bugmail) 2002-05-11 09:48:40 PDT
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.)
Comment 123 Esben Mose Hansen 2002-05-12 01:34:14 PDT
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                    
Comment 124 Esben Mose Hansen 2002-05-12 02:34:40 PDT
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...
Comment 125 Esben Mose Hansen 2002-05-12 02:49:48 PDT
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...
Comment 126 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-05-12 07:26:07 PDT
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).
Comment 127 Boris Zbarsky [:bz] 2002-05-15 00:26:31 PDT
*** Bug 144596 has been marked as a duplicate of this bug. ***
Comment 128 Esben Mose Hansen 2002-05-17 14:48:58 PDT
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.
Comment 129 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2002-05-17 15:46:57 PDT
> 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
Comment 130 Esben Mose Hansen 2002-05-18 13:28:06 PDT
Created attachment 84174 [details]
Testcase covering issues from comment 122 and comment 83
Comment 131 Esben Mose Hansen 2002-05-18 13:34:59 PDT
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.
Comment 132 Esben Mose Hansen 2002-05-18 13:48:08 PDT
Created attachment 84175 [details] [diff] [review]
Take 5:Final., unless otherwise proven. 

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.
Comment 133 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-05-18 14:40:02 PDT
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.
Comment 134 Esben Mose Hansen 2002-05-19 00:26:27 PDT
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.

Comment 135 Hixie (not reading bugmail) 2002-05-20 08:59:10 PDT
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 :-) ).
Comment 136 Esben Mose Hansen 2002-05-20 12:17:08 PDT
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.
Comment 137 Hixie (not reading bugmail) 2002-05-20 14:08:39 PDT
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?
Comment 138 Esben Mose Hansen 2002-05-21 09:38:46 PDT
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. 
Comment 139 Hixie (not reading bugmail) 2002-05-21 14:34:08 PDT
> 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
Comment 140 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-05-21 14:57:14 PDT
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).
Comment 141 Hixie (not reading bugmail) 2002-05-21 16:34:58 PDT
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...)
Comment 142 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-05-21 16:42:17 PDT
Oh, if that's true, then it's easy -- for a block, only walk up as long as the
parent is a block.
Comment 143 Hixie (not reading bugmail) 2002-05-21 17:45:41 PDT
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.
Comment 144 Esben Mose Hansen 2002-05-22 10:32:16 PDT
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.
Comment 145 Esben Mose Hansen 2002-05-25 00:15:40 PDT
Created attachment 85015 [details] [diff] [review]
Take 6. Redesign to walk up only to first inline frame.

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.
Comment 146 Esben Mose Hansen 2002-05-25 00:48:48 PDT
Created attachment 85019 [details] [diff] [review]
Take 6. Redesign to walk up only to first inline frame.

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.
Comment 147 Esben Mose Hansen 2002-05-25 11:26:40 PDT
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.
Comment 148 Esben Mose Hansen 2002-05-30 11:15:17 PDT
Created attachment 85627 [details] [diff] [review]
Take 7: Now handles padding and borders in inline frames correctly

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.
Comment 149 Hixie (not reading bugmail) 2002-06-07 13:40:36 PDT
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
Comment 150 Esben Mose Hansen 2002-06-08 01:29:45 PDT
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.
Comment 151 Esben Mose Hansen 2002-06-08 01:34:59 PDT
Created attachment 86924 [details] [diff] [review]
Same as take 7, but from a newer build (2002-06-08)
Comment 152 Hixie (not reading bugmail) 2002-06-08 05:34:50 PDT
I see the problem. The declaration is hidden inside an |#ifdef DEBUG| section,
and I'm building an optimised build.
Comment 153 Hixie (not reading bugmail) 2002-06-08 05:40:12 PDT
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'
Comment 154 Esben Mose Hansen 2002-06-08 11:01:28 PDT
Created attachment 86952 [details] [diff] [review]
The same patch as 86924 above, but with the missing file (nsLineLayout.h)

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 :-)
Comment 155 Hixie (not reading bugmail) 2002-06-08 16:20:38 PDT
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.
Comment 156 Hixie (not reading bugmail) 2002-06-08 16:22:30 PDT
Created attachment 86964 [details]
Hixie's mozconfig, with which the last patch doesn't run
Comment 157 Esben Mose Hansen 2002-06-09 00:03:14 PDT
Created attachment 86988 [details] [diff] [review]
As above, but now it compiles AND runs in optimized builds

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?
Comment 158 Hixie (not reading bugmail) 2002-06-09 04:39:02 PDT
Yup, why d'you think I use it? :-)

I'm building now.
Comment 159 Hixie (not reading bugmail) 2002-06-09 05:06:17 PDT
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.
Comment 160 Esben Mose Hansen 2002-06-09 05:21:00 PDT
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.
Comment 161 Esben Mose Hansen 2002-06-09 05:40:32 PDT
No luck :-( Both pages show up nice and stunningly fast. Even bugzilla flies
Sunday afternoon :-)
Any ideas?
Comment 162 Hixie (not reading bugmail) 2002-06-09 06:00:24 PDT
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. :-)
Comment 163 Hixie (not reading bugmail) 2002-06-09 06:13:56 PDT
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.
Comment 164 Esben Mose Hansen 2002-06-09 13:46:19 PDT
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 :-)
Comment 165 Hixie (not reading bugmail) 2002-06-10 05:22:08 PDT
Is the crash on http://off.net/~shaver/diary/ related to this patch?
Comment 166 Christian Eyrich 2002-06-10 06:38:04 PDT
No, I don't think so since my nightly 2002060808 without the patch also crashes.
Comment 167 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-06-11 13:14:46 PDT
*** Bug 150949 has been marked as a duplicate of this bug. ***
Comment 168 Hixie (not reading bugmail) 2002-06-11 13:23:06 PDT
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.)
Comment 169 Hixie (not reading bugmail) 2002-06-11 13:31:01 PDT
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... :-(
Comment 170 Esben Mose Hansen 2002-06-12 10:15:44 PDT
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.
Comment 171 Esben Mose Hansen 2002-06-12 12:35:59 PDT
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
Comment 172 Hixie (not reading bugmail) 2002-06-12 18:37:34 PDT
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.
Comment 173 Esben Mose Hansen 2002-06-21 08:31:14 PDT
Created attachment 88644 [details] [diff] [review]
Reverting to old code for quirks mode.

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? ;-)
Comment 174 Hixie (not reading bugmail) 2002-06-21 13:41:01 PDT
Don't worry about the CSS1 test suite. :-)
Comment 175 Esben Mose Hansen 2002-06-21 14:13:13 PDT
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..
Comment 176 Hixie (not reading bugmail) 2002-06-21 14:35:23 PDT
looks good to me; qa=hixie
Comment 177 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-07-01 11:54:39 PDT
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.
Comment 178 joakim_46 2002-07-01 15:16:43 PDT
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).
Comment 179 Esben Mose Hansen 2002-07-02 09:59:01 PDT
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....
Comment 180 Esben Mose Hansen 2002-07-02 10:04:43 PDT
Created attachment 89942 [details]
Picture describing three possible ways to overline in quirks mode.

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 181 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-07-17 09:11:35 PDT
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 182 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-07-17 10:28:55 PDT
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
Comment 183 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-07-17 10:37:34 PDT
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.
Comment 184 Esben Mose Hansen 2002-07-19 17:23:06 PDT
Created attachment 92046 [details] [diff] [review]
Latest patch updated with David Baron's comments

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.
Comment 185 Esben Mose Hansen 2002-07-19 17:27:50 PDT
Created attachment 92047 [details] [diff] [review]
As 92046/The latest patch, but /sans/ changes in whitespace. Don't check this version into CVS, please.

Adding patch /sans/ whitespace, as requested.
Comment 186 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-07-25 10:56:29 PDT
Did you send email asking for super-review?
Comment 187 Esben Mose Hansen 2002-07-26 00:20:58 PDT
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 188 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-07-26 06:31:50 PDT
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.
Comment 189 Esben Mose Hansen 2002-07-28 01:49:35 PDT
Created attachment 93061 [details] [diff] [review]
Patch for this bug and bug 20163. This is the full patch version.

Suggestion above implemented. Sorry for being unable to read english :-[
Comment 190 Esben Mose Hansen 2002-07-28 01:51:28 PDT
Created attachment 93062 [details] [diff] [review]
As 93071,  but without whitespace differences. Meant for reviews only.
Comment 191 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-07-28 06:02:03 PDT
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 192 Håkan Waara 2002-07-30 19:53:11 PDT
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);
>+    }
>+  }
>+
Comment 193 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-07-31 09:46:43 PDT
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.
Comment 194 Esben Mose Hansen 2002-08-06 12:09:58 PDT
Created attachment 94210 [details] [diff] [review]
Patch for this bug and bug 20163. This is the full patch version.

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 195 Esben Mose Hansen 2002-08-06 12:13:03 PDT
Created attachment 94213 [details] [diff] [review]
-w patch: Same as 94210, except whitespace difference are ignored. For review only.
Comment 196 Boris Zbarsky [:bz] 2002-08-10 21:07:06 PDT
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.
Comment 197 Esben Mose Hansen 2002-08-11 01:05:58 PDT
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.
Comment 198 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-09-10 12:44:26 PDT
Any chance of a patch that addresses the comments?
Comment 199 Esben Mose Hansen 2002-09-10 15:21:26 PDT
Sure, I can have a patch without the caching thing ready in a couple of days, if
anybody want. It would be my pleasure.

The following is all about this caching. If anybody wants this caching, some
comments to these lines would be very, very welcome.

I'm currently trying to do some measuring for the HasTextFrameChild, for the
"big" comment below, the caching of hasTextFrameChild() in comment 196. I'm
beginning to wonder whether caching anything is worth the bother. 85% or so hits
seems to find the answer in one level of recursion; 98--99% in 2 or less and the
rest in 3 and 4. (Taking from the "Internation Browser Busting Test" and some
personal poking around in pages with lots of links. So the first question is: Is
it worth it? Can we produce additional profiling data?

Anyway, it would be very helpful if anybody could hint at how such a caching
could be /invalidated/ (or updated)... it should be invalidated whenever a child
is added to or removed from the subtree. I can't figure out how to cache
something like that without making this invalidation (much) more expensive than
the recursive call :-/ However I look at it, I can only come up with these two
solutions:

1) Walk back up the tree from the inserted/removed frame, and invalidating as I
go. Looking at my "profiling" data, that's IMHO just not worth it.

2) Make a listener which registers itself to every child frame. This would make
the first calculation much more expensive and incur a significant memory
overhead, but would at least achieve cheap redraws.

So I just don't know. Any comments/ideas? 

P.S: When I started on this, I had a wild idea: All frames could have a bit,
HAS_TEXT_FRAME_CHILD, which would originally be set to false. Then, whenever a
textframe was added to the document tree, we could walk up the tree, setting the
bit to TRUE until we encounted a frame with HAS_TEXT_FRAME_CHILD already set to
TRUE. Then, when an TEXT_FRAME_CHILD was removed, the bit would have to be
recalculated for the entire tree. This would make initial draw and redraw very
cheap (O(1)), frame adding fairly cheap (O(treedepth)) and removal expensive
(O(treesize)). But then, everybody pays, whether text-decorations are used or
not, which is why I gave it up :-/ (the removal part could be optimized at the
expense of some memory, making it O(treedepth). 
Comment 200 Boris Zbarsky [:bz] 2002-09-10 16:12:48 PDT
I say skip on the caching for now and land the rest.  We can then optimize
HasTextFrameChild() as a separate endeavor....
Comment 201 Esben Mose Hansen 2002-09-16 12:59:20 PDT
The comments /sans/ the caching of HasTextFrameChild is implemented, except this
one:

> So... since we broke out of the loop above and |frame| is non-null, decorMask
> must be null.

Well, no actually. There's a break statement in the block above. I've added more
commments to the logic to make this more apparent. Also, I've added && decorMask
to emphasize that decorMask != 0. 

Finally:

> We could invalidate the cached info on content changes.....

On 2nd reading, you seem to imply that there's some standard event system for
doing this. Any hints to where I might look? And why not go ahead and cache the
result fo the entire GetTextDecorations()?

Patches are comming up...
Comment 202 Esben Mose Hansen 2002-09-16 13:01:09 PDT
Created attachment 99407 [details] [diff] [review]
Latest patch, full version
Comment 203 Esben Mose Hansen 2002-09-16 13:02:12 PDT
Created attachment 99408 [details] [diff] [review]
Latest patch, -w version, for review /only/.
Comment 204 Boris Zbarsky [:bz] 2002-09-20 09:18:53 PDT
Look at the ContentAppended, ContentInserted, ContentRemoved, ContentReplaced,
etc methods on nsIDocumentObserver...  These notifications get dispatched to the
PresShell and thence to the style set and the frame constructor.
Comment 205 Boris Zbarsky [:bz] 2002-09-30 16:06:36 PDT
Nits:

+        if (NS_STYLE_TEXT_DECORATION_LINE_THROUGH & decorMask 
+            & styleText->mTextDecoration) {

Keep the operator on the previous line in cases like this.  This comes up all
over in the code around that chunk.

+   *  @param aUnderColor   The color of underline if the appropriate bit 
+   *                       above is set. It is undefined otherwise.

s/above/in aDecorations/ please?  Same for the other params.

+   *  NOTE: This function return with aDecorations==NS_STYLE_TEXT_DECORATION_NONE

"This function returns"

+                      | NS_STYLE_TEXT_DECORATION_OVERLINE 

again, put the operator on the previous line.

+      aUnderColor =styleColor->mColor;

space after the '='?

+  for (aParent->FirstChild(aPresContext, nsnull, &kid); kid; 
+       kid->GetNextSibling(&kid))
+    {

follow the prevailing bracing style in the file?  (either brace-on-same-line
or brace-on-next-line-lined-up-with-"for", whichever is used more)

+       | NS_STYLE_TEXT_DECORATION_OVERLINE)) {

the usual.  Put the '|' on the previous line.

Fix those, and sr=bzbarsky, assuming dbaron reviews.
Comment 206 timeless 2002-09-30 17:12:17 PDT
+   *  @param aUnderColor   The color of the underline if the appropriate bit 
+   *                       in aDecorations is set. It is undefined otherwise.
+   *  @param aOverColor    The color of overline if the appropriate bit 
+   *                       in aDecorations is set. It is undefined otherwise.
+   *  @param aStrikeColor  The color of strike-through if the appropriate bit 
+   *                       in aDecorations is set. It is undefined otherwise.

If this were a public interface, i'd prefer
aUnderlineColor/aOverlineColor/aStrikethroughColor, but this isn't :-)
Comment 207 Esben Mose Hansen 2002-10-02 13:41:52 PDT
Created attachment 101439 [details] [diff] [review]
Latest patch, including (most of) Boris Zbarsky comments

I've decided not to change the @param as requested. For one, the sed installed
in my brain could not find a match for the pattern requested, and for another,
my version looks like the other code with @param:

http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsBlockFrame.h#322


So if anyone feel it is important enough that I should correct this, could you
please give a reference to some layout-code that uses the syntax you want? 

re timeless: I agree, on both counts.
Comment 208 Esben Mose Hansen 2002-10-02 13:44:18 PDT
Created attachment 101440 [details] [diff] [review]
as  101439, but without whitespace diffs. *For review only*.
Comment 209 Boris Zbarsky [:bz] 2002-10-02 14:07:21 PDT
All I meant was to replace "if the appropriate bit above is set" with "if the
appropriate bit in aDecorations is set".
Comment 210 Boris Zbarsky [:bz] 2002-10-05 18:46:01 PDT
Comment on attachment 101439 [details] [diff] [review]
Latest patch, including (most of) Boris Zbarsky comments

sr=bzbarsky, but please do make that comment clarification change...
Comment 211 Esben Mose Hansen 2002-10-05 23:37:23 PDT
Created attachment 101870 [details] [diff] [review]
Latest patch including the last of Boris Zbarsky comments.

But of course. Your wish is my command :O) Thanks for spelling that last one
out....
Comment 212 Esben Mose Hansen 2002-10-05 23:38:40 PDT
Created attachment 101871 [details] [diff] [review]
as  101870, but without whitespace diffs. *For review only*.
Comment 213 timeless 2002-10-05 23:49:34 PDT
err, i messed up when i commented, this is what i was going for, i think...
+   *  @param aUnderColor   The color of the underline if the appropriate
+   *                       bit in aDecoration is set. It is 
+   *                       undefined otherwise.
+   *  @param aOverColor    The color of the overline if the appropriate
+   *                       bit in aDecoration is set. It is 
+   *                       undefined otherwise.
+   *  @param aStrikeColor  The color of the strike-through if the 
+   *                       appropriate bit in aDecoration is set. It is 
+   *                       undefined otherwise.
Comment 214 Esben Mose Hansen 2002-10-05 23:57:48 PDT
This is how it looks in the latests patch. It may be early in the morning, but
it looks identical to me :-D (Admittedly, it's minutes since I change it :o) )

   *  @param aUnderColor   The color of underline if the appropriate bit 
   *                       in aDecoration above is set. It is undefined 
   *                       otherwise.
   *  @param aOverColor    The color of overline if the appropriate bit 
   *                       in aDecoration above is set. It is undefined 
   *                       otherwise.
   *  @param aStrikeColor  The color of strike-through if the appropriate bit 
   *                       in aDecoration above is set. It is undefined 
   *                       otherwise.
Comment 215 Esben Mose Hansen 2002-10-15 11:09:08 PDT
Created attachment 102946 [details] [diff] [review]
Merge conflicts resolved; textual nonsense removed

The patch for 

Bug 171808, r=jkeiser, sr=dbaron

caused merge conflicts with my patch, so I resolved them. This also gave me an
opportunity to correct a textual error pointed out by timeless (thanks!)

Whitespace-patch coming up.

Also, could I ask you to please not cut the part of the patch where the file
you're commenting on is mentioned? The patch is big enough that I don't care to
hunt for the correct file. Thanks!
Comment 216 Esben Mose Hansen 2002-10-15 11:10:15 PDT
Created attachment 102947 [details] [diff] [review]
Whitespace-less patch. For review only.
Comment 217 Christian :Biesinger (don't email me, ping me on IRC) 2002-10-15 14:08:18 PDT
nsBlockFrame.cpp:
+  // This for is standards mode only

shouldn't that be "This is for standards mode only"?
Comment 218 Boris Zbarsky [:bz] 2002-10-25 23:20:49 PDT
Comment on attachment 102946 [details] [diff] [review]
Merge conflicts resolved; textual nonsense removed

sr=bzbarsky still applies...
Comment 219 Hixie (not reading bugmail) 2002-11-18 09:00:45 PST
dbaron: Could you take this, fix the problems you have with it, and check it in?
It has sr=bz and is just waiting for your review. Cheers!
Comment 220 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-12-06 05:22:17 PST
Created attachment 108484 [details]
testcase for text-decoration on text inputs

This tests 'text-decoration' on text inputs, for which a large chunk of code
was added in nsHTMLContainerFrame in the patch.  I replaced that code with two
single line changes to nsTextControlFrame.cpp (modifying DIV_STRING and
DIV_SINGLELINE_STRING) that cause the same results.  However, I question
whether those results are even correct and we need the changes at all.

(I have a bunch of other changes to the patch in my tree, but I've only gone
through one file so far).
Comment 221 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-12-06 12:51:12 PST
Created attachment 108525 [details] [diff] [review]
patch, with dbaron's modifications

This is the previous patch, merged to the tip and with a significant number of
modifications I made.  (See Hixie's suggestion a few comments up.)

I have to run now, but I'll provide a description of the changes I made later
this evening.
Comment 222 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-12-07 12:48:02 PST
The modifications I made in attachment 108525 [details] [diff] [review] (relative to attachment 102946 [details] [diff] [review]) are:

Cleaned up whitespace (particularly extra blank lines and extra whitespace at
the end of lines) throughout.  Also made some indentation changes to new and
existing code.  I also removed many of the comments on end braces saying what
they were closing.

Renamed |HasTextFrameChild| to |HasTextFrameDescendant|.

Moved the declaration and use of the |styleContext| variable in
nsHTMLContainerFrame::GetTextDecorations down to where it can be different from
|mStyleContext|.

Added .get() to many places where |::GetStyleData| was used with an
nsCOMPtr<nsIStyleContext>, since otherwise we'll break IRIX and OS/2.

Removed the huge text inputs hack in nsHTMLContainerFrame::GetTextDecorations
and replaced it with a two line change to CSS in nsTextControlFrame.cpp.

Changed the loop in nsTextFrame::PaintTextDecorations from do-while to while,
added |useDecorations| variable to avoid repeated calculation of the same thing,
converted existing code to use typesafe |GetStyleData|.  I might have done some
other cleanup here that I've forgotten.

Fixed up spaces between parameters for function calls in nsTextFrame (and lots
of whitespace at end of line).

Changed the ascent stored on the line box to being relative to the top of the
line box (so that it's the ascent of the anonymous inline box for the block that
we don't actually create) rather than being relative to the parent block.

Refactored a number of paint methods significantly to reduce the duplicated code
in this patch:
 * Moved the stuff in nsHTMLContainerFrame::Paint that was specific to inline
   frames and canvas frame into paint methods on those frames (creating new one
   on inline frames)
 * In order to do this, I consolidated the background, border, and outline code
   into a nonvirtual member function of nsHTMLContainerFrame, |PaintSelf|.
   (I could probably do a bit more conversion to PaintSelf.)  Note that this
   makes one minor change, which is that nsHTMLContainerFrame's Paint method
   is now using nsIFrame::IsVisibleForPainting whereas it used to do the
   visibility check manually.  (There's a slight difference.)

Changed nsHTMLContainerFrame::PaintTextDecorations so that the caller passes in
the font metrics.  (This probably isn't worth it as an optimization, but it
really isn't any simpler the old way.)
 * I then moved the common pattern of painting underline and overline, painting
   children, and then painting line-through (strict mode text decorations only)
   into another new nonvirtual member function of nsHTMLContainerFrame,
   |PaintDecorationsAndChildren|.  This is called by Paint methods on
   nsInlineFrame and nsBlockFrame.
Comment 223 Hixie (not reading bugmail) 2002-12-08 22:03:46 PST
It would be excellent if this could go into 1.3a to get some testing.
Comment 224 Boris Zbarsky [:bz] 2002-12-08 22:06:07 PST
I have a final till about 1pm CST tomorrow... I can try to review right after that.
Comment 225 Esben Mose Hansen 2002-12-09 10:15:45 PST
The changes look really cool, especially the bit about the special-handling for
input forms. 

Anyway, I consider this patch out of my hands now (i.e., I won't post any more
updates if the patch break.)
Comment 226 Boris Zbarsky [:bz] 2002-12-09 13:40:51 PST
Comment on attachment 108525 [details] [diff] [review]
patch, with dbaron's modifications

> +      // XXXldb This is the wrong way to set |isPre|.

Is there a bug on having a function on the style struct for that?  If not, we
should file one....

Nice cleanup.  ;)
Comment 227 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-12-10 12:14:02 PST
Filed bug 184702 (more Paint refactoring) and bug 184703 (pre method on style
struct and fixing the XXX comment bz quoted above).
Comment 228 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-12-10 12:21:07 PST
Taking this bug so I remember to check it in.
Comment 229 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-12-10 19:52:36 PST
I removed the firstChildOrigin hack in nsBlockFrame::PaintTextDecorationLines
(and just used line->mBounds.x) since bug 145467 is fixed now.
Comment 230 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-12-10 20:03:09 PST
->esben, for credit
Comment 231 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-12-10 20:04:21 PST
Fix checked in to trunk, 2002-12-10 20:00 PDT.  (That's *after* the 1.3alpha
release was tagged, so it won't be in 1.3alpha.)
Comment 232 Aleksey Nogin 2002-12-11 01:08:50 PST
This commit have added a warning on Brad Tbox: 

+layout/html/base/src/nsTextFrame.cpp:1952
+ `nscoord baseline' might be used uninitialized in this function

Indeed the baseline variable declared on line 1952 (do not confuse it with the
one declared on line 1895) is *never* initialized at all, but is being used in
many places!

P.S. Per bug 179819 the "uninitialized" warnings ought to be treated as errors,
so I am reopening the bug.
Comment 233 Boris Zbarsky [:bz] 2002-12-11 04:37:56 PST
Doh.  

+          nscoord offset, size, baseline;

should be 

+          nscoord offset, size, baseline = mAscent;

no?
Comment 234 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-12-11 07:59:52 PST
Fixed.
Comment 235 Hixie (not reading bugmail) 2002-12-12 00:41:38 PST
VERIFIED.

Esben: If I'm ever in Copenhagen, I'll buy you dinner. :-)
Comment 236 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-12-16 07:39:12 PST
This caused regression bug 185581 (which was simple to fix).
Comment 237 Koike Kazuhiko 2003-05-10 02:18:18 PDT
*** Bug 44232 has been marked as a duplicate of this bug. ***
Comment 238 Caleb 2005-01-07 07:26:41 PST
How come is this bug marked VERIFIED FIXED when the first testcase can still be
reproduced in the latest moz 1.8a5 ?

Will this problem remain in mozilla FOREVER ???? 

It is really ugly and all the other browsers treat it correctly (I'm talking
about the first/second testcase).

I'm tired of seeing that thick underline :(

Please fix it!
Comment 239 Boris Zbarsky [:bz] 2005-01-07 07:55:57 PST
The first two testcases in this bug are in quirks mode.  In quirks mode, we do
quirky, more-compatible-with-IE decoration painting.  Please test in standards
mode to see the fix for this bug.
Comment 240 Caleb 2005-01-07 08:43:51 PST
Oh, that's very interesting, I have tested it and it works perfectly in
standards mode.

Will this be fixed in quirks mode then? Since most pages do not specify a doctype.

This bug has been bugging me for years since I see its effects on pages that I
regularly visit, like Google.com for example.
Comment 241 Boris Zbarsky [:bz] 2005-01-07 09:00:12 PST
> Will this be fixed in quirks mode then? 

No, because doing full-standards painting treats colors and such very
differently from the way most pages expect them to be treated.
Comment 242 neil@parkwaycc.co.uk 2007-01-22 06:51:24 PST
Comment on attachment 108525 [details] [diff] [review]
patch, with dbaron's modifications

>+      if (!styleDisplay->IsBlockLevel()) {
>+        // If an inline frame is discovered while walking up the tree,
>+        // we should stop according to CSS3 draft. CSS2 is rather vague
>+        // about this.
>+        break;
>+      }
This seems to have broken painting of decorations on <label> elements with child text nodes i.e. <xul:label style="color: blue; text-decoration: underline;">Click here</xul:label>. (labels aren't blocks). Bug coming up.
Comment 243 neil@parkwaycc.co.uk 2007-01-22 07:30:58 PST
I don't see the issue on branch, so maybe this bug isn't directly responsible; fallout from the paint refactoring perhaps?
Comment 244 Boris Zbarsky [:bz] 2007-01-22 07:56:24 PST
The quote in comment 242 is for the case when a block-level container is asked to paint its decorations.  All that check does is make block-inside-inline situations not go up to the inline looking for decorations.

So in your case, if you have a block somewhere inside the label it could be an issue...

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