Closed
Bug 1777
Opened 26 years ago
Closed 22 years ago
[TEXT] 'text-decoration' should not be drawn by children (underline) [INLINE]
Categories
(Core :: Layout: Text and Fonts, defect, P3)
Core
Layout: Text and Fonts
Tracking
()
VERIFIED
FIXED
mozilla1.3beta
People
(Reporter: glynn, Assigned: esben)
References
(Blocks 1 open bug, )
Details
(Keywords: css1, testcase, Whiteboard: [patch][Hixie-P1][CSS1-5.4.3] (high profile: css1 test suite) underline not displayed properly across bold modifications [Hixie-B])
Attachments
(18 files, 27 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
image/gif
|
Details | |
(deleted),
image/gif
|
Details | |
(deleted),
image/gif
|
Details | |
(deleted),
image/gif
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
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•26 years ago
|
||
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 3•26 years ago
|
||
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•25 years ago
|
||
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.
Updated•25 years ago
|
Summary: underline not displayed properly across bold modifications → {css1} underline not displayed properly across bold modifications
Updated•25 years ago
|
QA Contact: 4144 → 4110
Comment 7•25 years ago
|
||
Updated•25 years ago
|
Whiteboard: [makingtest] spacecow@mis.net
Comment 8•25 years ago
|
||
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.
Updated•25 years ago
|
Whiteboard: [makingtest] spacecow@mis.net → [TESTCASE]
Comment 9•25 years ago
|
||
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•25 years ago
|
||
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•25 years ago
|
||
Comment 12•25 years ago
|
||
*** Bug 3488 has been marked as a duplicate of this bug. ***
Comment 13•25 years ago
|
||
Comment 14•25 years ago
|
||
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•25 years ago
|
||
*** Bug 10065 has been marked as a duplicate of this bug. ***
Comment 16•25 years ago
|
||
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•25 years ago
|
||
Updating to default Layout Assignee...kipp no longer with us :-(
Comment 18•25 years ago
|
||
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.
Updated•25 years ago
|
Depends on: 20163
Summary: {css1} underline not displayed properly across bold modifications → {css1} 'text-decoration' should not be drawn by children
Whiteboard: [TESTCASE] → [TESTCASE] underline not displayed properly across bold modifications
Comment 19•25 years ago
|
||
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.
Summary: {css1} 'text-decoration' should not be drawn by children → {css1} [TEXT] 'text-decoration' should not be drawn by children
Comment 20•25 years ago
|
||
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•25 years ago
|
||
Bulk moving [testcase] code to new testcase keyword. Sorry for the spam!
Keywords: testcase
Updated•25 years ago
|
Summary: {css1} [TEXT] 'text-decoration' should not be drawn by children → [TEXT] 'text-decoration' should not be drawn by children (underline)
Whiteboard: [TESTCASE] underline not displayed properly across bold modifications → underline not displayed properly across bold modifications
Comment 22•25 years ago
|
||
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•25 years ago
|
||
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 25•24 years ago
|
||
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.
Assignee: buster → erik
Comment 26•24 years ago
|
||
Comment 27•24 years ago
|
||
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.
Keywords: nsbeta2
Updated•24 years ago
|
Status: NEW → ASSIGNED
Comment 28•24 years ago
|
||
Putting on [nsbeta2+] [will be minus on 6/15] radar.
Whiteboard: underline not displayed properly across bold modifications → [nsbeta2+][will be minus on 6/15]underline not displayed properly across bold modifications
Comment 29•24 years ago
|
||
Nominating nsbeta2,nsbeta3,rtm. Recc. nsbeta2+, falling through to nsbeta3+ and
rtm+ if necessary. This is a W3C CSS1 Official Test Suite compliance bug.
Updated•24 years ago
|
Comment 30•24 years ago
|
||
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•24 years ago
|
||
PDT: please leave nsbeta2+. Standards compliance issue.
Comment 32•24 years ago
|
||
Sorry, I was going to reference ekrock's 2000-06-14 19:08 comments...
Comment 33•24 years ago
|
||
6/15 has passed... changing to nsbeta2 minus
Whiteboard: [nsbeta2+][will be minus on 6/15]underline not displayed properly across bold modifications → [nsbeta2-]underline not displayed properly across bold modifications
Comment 34•24 years ago
|
||
*** Bug 44420 has been marked as a duplicate of this bug. ***
Updated•24 years ago
|
Keywords: correctness
Comment 35•24 years ago
|
||
As per meeting with ChrisD yesterday, taking QA.
QA Contact: chrisd → py8ieh=bugzilla
Updated•24 years ago
|
Whiteboard: [nsbeta2-]underline not displayed properly across bold modifications → [nsbeta2-] hit during nsbeta2 standards compliance testing | underline not displayed properly across bold modifications
Comment 36•24 years ago
|
||
*** Bug 47749 has been marked as a duplicate of this bug. ***
Comment 37•24 years ago
|
||
nsbeta3+ P3 per bug meeting (ekrock)
Whiteboard: [nsbeta2-] hit during nsbeta2 standards compliance testing | underline not displayed properly across bold modifications → [nsbeta2-] nsbeta3+ hit during nsbeta2 standards compliance testing | underline not displayed properly across bold modifications
Comment 38•24 years ago
|
||
Adding buster to cc: list.
Comment 39•24 years ago
|
||
*** Bug 50410 has been marked as a duplicate of this bug. ***
Comment 40•24 years ago
|
||
When is the going to be fixed???
Comment 41•24 years ago
|
||
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•24 years ago
|
||
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! ;-)
OS: Windows NT → All
Hardware: PC → All
Whiteboard: [nsbeta2-] nsbeta3+ hit during nsbeta2 standards compliance testing | underline not displayed properly across bold modifications → [nsbeta2-] [nsbeta3+] hit during nsbeta2 standards compliance testing | underline not displayed properly across bold modifications
Comment 43•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
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.
Keywords: helpwanted,
relnote
Whiteboard: [nsbeta2-] [nsbeta3+] hit during nsbeta2 standards compliance testing | underline not displayed properly across bold modifications → [nsbeta2-] [nsbeta3-] hit during nsbeta2 standards compliance testing | underline not displayed properly across bold modifications
Target Milestone: M17 → Future
Comment 47•24 years ago
|
||
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.
Updated•24 years ago
|
Updated•24 years ago
|
Summary: [TEXT] 'text-decoration' should not be drawn by children (underline) → [TEXT] 'text-decoration' should not be drawn by children (underline) [INLINE]
Updated•24 years ago
|
Keywords: relnote3
Whiteboard: [nsbeta2-] [nsbeta3-] hit during nsbeta2 standards compliance testing | underline not displayed properly across bold modifications → [nsbeta2-] [nsbeta3-] relnote-devel hit during nsbeta2 standards compliance testing | underline not displayed properly across bold modifications
Comment 48•24 years ago
|
||
Testing on 11_06 branch builds, Linux and Mac look okay. Underlining is
consistent and not bolded.
Comment 49•24 years ago
|
||
Still seeing this on Windows 98 with the Nov 9 daily build. What could have
changed in the Linux and Mac builds?
Comment 50•24 years ago
|
||
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.
Keywords: mozilla0.9
Comment 51•24 years ago
|
||
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•24 years ago
|
||
No. And Mozilla is not Netscape6 so the claims that Mozilla will be complaint
are not affected by Netscape6's standards support.
Comment 53•24 years ago
|
||
Thanks for the information
Comment 54•24 years ago
|
||
*** Bug 64207 has been marked as a duplicate of this bug. ***
Updated•24 years ago
|
Keywords: nsbeta2,
nsbeta3,
relnoteRTM
Whiteboard: [nsbeta2-] [nsbeta3-] relnote-devel hit during nsbeta2 standards compliance testing | underline not displayed properly across bold modifications → [Hixie-P2] (high profile: css1 test suite) underline not displayed properly across bold modifications
Comment 55•24 years ago
|
||
BTW, per the SF F2F meeting, the working group agree that our interpretation is
correct. (This will be made explicit in CSS3.)
Comment 56•24 years ago
|
||
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•24 years ago
|
||
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... ;-)
Updated•23 years ago
|
Whiteboard: [Hixie-P2] (high profile: css1 test suite) underline not displayed properly across bold modifications → [Hixie-P1] (high profile: css1 test suite) underline not displayed properly across bold modifications [Hixie-B]
Comment 58•23 years ago
|
||
erik resign. reassign all his bug to ftang for now.
Assignee: erik → ftang
Status: ASSIGNED → NEW
Comment 60•23 years ago
|
||
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 62•23 years ago
|
||
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.
Status: NEW → ASSIGNED
Comment 63•23 years ago
|
||
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
Updated•23 years ago
|
Whiteboard: [Hixie-P1] (high profile: css1 test suite) underline not displayed properly across bold modifications [Hixie-B] → [Hixie-P1][Hixie-1.0] (high profile: css1 test suite) underline not displayed properly across bold modifications [Hixie-B]
Comment 64•23 years ago
|
||
is this still a highrisk?
Comment 65•23 years ago
|
||
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•23 years ago
|
||
Looks more like strike-through than overline to me
Comment 67•23 years ago
|
||
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•23 years ago
|
||
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., , 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•23 years ago
|
||
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•23 years ago
|
||
>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•23 years ago
|
||
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
Updated•23 years ago
|
Whiteboard: [Hixie-P1][Hixie-1.0] (high profile: css1 test suite) underline not displayed properly across bold modifications [Hixie-B] → [Hixie-P1] (high profile: css1 test suite) underline not displayed properly across bold modifications [Hixie-B]
Comment 72•23 years ago
|
||
*** Bug 121760 has been marked as a duplicate of this bug. ***
Comment 73•23 years ago
|
||
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•23 years ago
|
||
Comment 75•23 years ago
|
||
Is this really a futured bug?
Updated•23 years ago
|
Whiteboard: [Hixie-P1] (high profile: css1 test suite) underline not displayed properly across bold modifications [Hixie-B] → [Hixie-P1][CSS1-5.4.3] (high profile: css1 test suite) underline not displayed properly across bold modifications [Hixie-B]
Updated•23 years ago
|
Keywords: mozilla1.0+
This shouldn't be hard to fix to get Hixie's desired behaviour, it can all be
done in the inline container frames.
Ugh. nsTextFrame::PaintTextDecorations implements a <FONT> quirk where the
colour specified in <FONT> overrides the decoration colour specified in the
decorated element. Implementing THAT while painting the decoration in the
container is going to be a pain.
Also, there are some tricky issues with "paint decoration only if there is some
text in the element" and dynamic changes. If text children are added to a
decorated element then we may need to repaint the entire element.
Also, the way decoration is applied to block elements seems weird. The spec says
all "inline descendants" are affected. If that includes all indirect inline
descendants then the effect would be very weird indeed. If it only includes
direct inline descendants, what about blocks that contain other blocks? I
suggest we should search down through the frame tree for each inline frame F
which has only block frames between it and the decorated block frame.
So, given the following frame tree:
block0
block1 { text-decoration: underline; }
block2
inlineA
inlineB
We would paint the decoration for inlineA only, using block1's colour.
Another question: given
<a href="..." style="text-decoration: underline"><img src="..."> </a>
doesn't the A element actually contain text --- a single space? The spec doesn't
say anything about the text not being whitespace.
Comment 78•23 years ago
|
||
Not sure what to do about the <font> quirks. FWIW, if you want to limit them to
quirks mode, that's fine.
> The spec says all "inline descendants" are affected.
Ouch. I have no idea how to interpret that in CSS terms. (I tried describing it
in terms of implied anonymous inline boxes, but that didn't explain why the
colour was from the block setting text-decoration.) What do you think should
happen in this case?:
block0 { text-decoration: underline; color: rgba(255,0,0, 0.5); }
block1 { text-decoration: underline; color: rgba(0,255,0, 0.5); }
block2 { text-decoration: underline; color: rgba(0,0,255, 0.5); }
inlineA
inlineB
Regarding the empty space thing: if white-space is set to 'pre' then spaces are
significant in layout, otherwise they are not.
My guess is that in your example we should paint 3 composited underlines, but
you're the W3C guy :-). Perhaps the WG could clarify all these issues in the
CSS3 WD?
> Regarding the empty space thing: if white-space is set to 'pre' then spaces
> are significant in layout, otherwise they are not.
I don't understand this. Even when whitespace is not 'pre', spaces are
significant, otherwise we wouldn't break lines :-). I don't still don't see why
in my example
<a href="..." style="text-decoration: underline"><img src="..."> </a>
we can say there is "no text" inside the A element. There is text there in the
DOM. There is whitespace there for the purposes of line breaking.
Comment 80•23 years ago
|
||
Would the argument "whitespace isn't text" hold water? :-)
Whitespace in the case you gave isn't text because it can collapse away in the
rendering (e.g. if there is a space on the other side of the end tag). Also,
when would an author ever _want_ the image to be underlined based on the
presence of a space character? The only time I see the author caring about that
space is when he explicitly says that the 'white-space' should be 'pre'served.
Regarding the block issue, I've raised it in the WG.
OK, I concede that "whitespace isn't really text" is one plausible
interpretation of the language in the recommendation, but I respectfully suggest
that it is not the only possible interpretation. It would be great if you could
ask the WG to clarify that too.
Comment 82•23 years ago
|
||
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•23 years ago
|
||
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.
Assignee | ||
Comment 84•22 years ago
|
||
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•22 years ago
|
||
Go fast, 1.0 is trying to wrap up RC2 in a week or so. RC2 might be 1.0, so
don't count on getting this into 1.0 if you don't get into RC2.
/be
I don't think this would get approved for 1.0, it's not important enough.
Let us know how you're planning to fix this so we can give you feedback.
Assignee | ||
Comment 87•22 years ago
|
||
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
Assignee | ||
Comment 88•22 years ago
|
||
Oops, forget my rambling about nsBlockFrame and nsHTMLContainerFrame. I see that
the former is derived from the latter. Must've been blind :-]. I'll just place
the neccessary rendering in nsHTMLContainerFrame. I'll press on; I'm pretty
happy with HTMLContainerFrame's rendering of the textdecorations now. Now I'm
working on nsBlockFrame, which is a litte more difficult, as I have to look at
the lines in the frame, I think.
rbs@maths.uq.edu.au: If you still have the testcases from which the Opera-images
are derived, I'd dearly like to get my dirty hands on them :O)
P.S: Could somebody please tell me if I should grap the bug, or leave it with
hyatt@netscape.com?
Please read all of the above comments especially my comment #77. Moving all the
painting up to the container is OK but that alone is not enough to make the FONT
quirk work. Please also read Ian's comments about the action of text-decoration
set on containing blocks and make sure that what you're planning will do the
right thing for those examples. Thanks!!!
Comment 90•22 years ago
|
||
Assignee | ||
Comment 91•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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.
Assignee | ||
Comment 94•22 years ago
|
||
Assignee | ||
Comment 95•22 years ago
|
||
The testcase I've used under development, in addition to those supplied in this
bug. See the comment below.
Assignee | ||
Comment 96•22 years ago
|
||
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 >:-}
Assignee | ||
Comment 97•22 years ago
|
||
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:
Assignee | ||
Comment 98•22 years ago
|
||
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•22 years ago
|
||
Comment 100•22 years ago
|
||
Comment 101•22 years ago
|
||
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•22 years ago
|
||
Updated•22 years ago
|
Attachment #81296 -
Attachment description: testcase bug 1777 - problems / inconsistencies in painting order → testcase showing problems / inconsistencies in painting order
Updated•22 years ago
|
Keywords: mozilla1.0.1
Comment 103•22 years ago
|
||
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! :-)
Assignee | ||
Comment 104•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
*** Bug 142407 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 107•22 years ago
|
||
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.
Attachment #81201 -
Attachment is obsolete: true
Attachment #81273 -
Attachment is obsolete: true
Comment 108•22 years ago
|
||
reassign to patch author
Assignee | ||
Comment 109•22 years ago
|
||
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.
Status: NEW → ASSIGNED
Comment 110•22 years ago
|
||
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•22 years ago
|
||
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.)
Assignee | ||
Comment 112•22 years ago
|
||
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.
Attachment #82518 -
Attachment is obsolete: true
Assignee | ||
Comment 113•22 years ago
|
||
Updating keywords to reflect status...
Comment 114•22 years ago
|
||
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.
Attachment #83043 -
Flags: needs-work+
Comment 115•22 years ago
|
||
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•22 years ago
|
||
... 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?
Assignee | ||
Comment 117•22 years ago
|
||
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.
Assignee | ||
Comment 118•22 years ago
|
||
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•22 years ago
|
||
> 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•22 years ago
|
||
> 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•22 years ago
|
||
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•22 years ago
|
||
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.)
Assignee | ||
Comment 123•22 years ago
|
||
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
Assignee | ||
Comment 124•22 years ago
|
||
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...
Assignee | ||
Comment 125•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
*** Bug 144596 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 128•22 years ago
|
||
OK, I *think* I've made it all work :) Tomorrow I'll clean up the code and
submit another patch. However, in order to make form controls work, I've had to
specialcase them, and I'm somewhat unhappy with this. However, since the buttom
of a tree containing a form control looks something like this:
scrollFrame
textInputFrame <--- the text-decorations are here
scrollFrame
AreaFrame <--- the lines, and consequently, the text, are here.
I need to use a fairly tight criterium to detect a possible situation like this,
and then walk up the tree to find (if) there is a textInputFrame above
the AreaFrame in the tree. From this textInputFrame I need to pull the
textdecorations. The criterium I use is that the frame must be an areaFrame with
NS_FRAME_HAS_VIEW state, view the styleContext having the HasTextDecoration
property.
This specialcasing raises another issue: I would want this kind of logic in the
GetTextDecoration function; otherwise I'll either need to encumber nsBlockFrame
with another helpermethod or make nsBlockFrame::Paint() longer than I would
like. This means that dbarons suggestion in comment 126 about the aIsBlock will
have to be replaced with a logic based on the current frameType. I'll try to
come up with something reasonable, and then you can pick it apart :o)
On this node: Can somebody create a point to a reasonable complex form testcase
with text-decorations? I'm really quite unfamiliar with forms, so I'm not too
sure I've covered all eventualities. Some table testcases would be neat, too.
Re comment 126 and others: I've implemented Ian suggestion. I'll file a bug
regarding the nsLineBox tomorrow.
I'm unable to find anything on hyatt's code regarding the FONT quirk excepting
the checkin comment to 1.306 of nsTextFrame.cpp stating "rule matching
improvements". This seems to part of a rather large checkin, but scanning this
has given me no new insights. But given the size, I might easily have missed
something. This is the complete checkin, should anyone care :)
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=SeaMonkeyAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=hyatt.*&whotype=regexp&sortby=Date&hours=2&date=explicit&mindate=05%2F31%2F2001+15%3A00&maxdate=05%2F31%2F2001+16%3A00&cvsroot=%2Fcvsrootcheck-in,
That was my 5€. Now I need some sleep, and tomorrow I'll create a new "final"
version of the patch. I'll also attach the a testcase not represented above
regarding nested text-decorations and block frames.
P.S: One thing mystified me about forms: in the form control testcase above
http://bugzilla.mozilla.org/attachment.cgi?id=81295&action=view
the entire tree rendering the form has display property set to block. Shouldn't
this property be inline?! If not, then form controls are invalid in <P></P>
tags, I suppose.
> The criterium I use is that the frame must be an areaFrame with
> NS_FRAME_HAS_VIEW state
Don't do that; there are many ways a frame can have that state set.
> Can somebody create a point to a reasonable complex form testcase
> with text-decorations?
Debug | Viewer Demos | #5 More Styles
Assignee | ||
Comment 130•22 years ago
|
||
Assignee | ||
Comment 131•22 years ago
|
||
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.
Assignee | ||
Comment 132•22 years ago
|
||
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.
Attachment #83043 -
Attachment is obsolete: true
Comment 133•22 years ago
|
||
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.
Assignee | ||
Comment 134•22 years ago
|
||
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•22 years ago
|
||
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 :-) ).
Assignee | ||
Comment 136•22 years ago
|
||
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•22 years ago
|
||
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?
Assignee | ||
Comment 138•22 years ago
|
||
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•22 years ago
|
||
> 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•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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.
Assignee | ||
Comment 144•22 years ago
|
||
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.
Assignee | ||
Comment 145•22 years ago
|
||
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.
Assignee | ||
Comment 146•22 years ago
|
||
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.
Assignee | ||
Comment 147•22 years ago
|
||
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.
Assignee | ||
Comment 148•22 years ago
|
||
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.
Attachment #84175 -
Attachment is obsolete: true
Attachment #85015 -
Attachment is obsolete: true
Attachment #85019 -
Attachment is obsolete: true
Comment 149•22 years ago
|
||
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
Assignee | ||
Comment 150•22 years ago
|
||
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.
Assignee | ||
Comment 151•22 years ago
|
||
Comment 152•22 years ago
|
||
I see the problem. The declaration is hidden inside an |#ifdef DEBUG| section,
and I'm building an optimised build.
Comment 153•22 years ago
|
||
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'
Assignee | ||
Comment 154•22 years ago
|
||
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 :-)
Attachment #85627 -
Attachment is obsolete: true
Attachment #86924 -
Attachment is obsolete: true
Comment 155•22 years ago
|
||
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•22 years ago
|
||
Assignee | ||
Comment 157•22 years ago
|
||
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?
Attachment #86952 -
Attachment is obsolete: true
Comment 158•22 years ago
|
||
Yup, why d'you think I use it? :-)
I'm building now.
Comment 159•22 years ago
|
||
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.
Assignee | ||
Comment 160•22 years ago
|
||
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.
Assignee | ||
Comment 161•22 years ago
|
||
No luck :-( Both pages show up nice and stunningly fast. Even bugzilla flies
Sunday afternoon :-)
Any ideas?
Comment 162•22 years ago
|
||
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•22 years ago
|
||
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.
Assignee | ||
Comment 164•22 years ago
|
||
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 :-)
Keywords: review
Comment 165•22 years ago
|
||
Is the crash on http://off.net/~shaver/diary/ related to this patch?
Comment 166•22 years ago
|
||
No, I don't think so since my nightly 2002060808 without the patch also crashes.
Comment 167•22 years ago
|
||
*** Bug 150949 has been marked as a duplicate of this bug. ***
Comment 168•22 years ago
|
||
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•22 years ago
|
||
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... :-(
Assignee | ||
Comment 170•22 years ago
|
||
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.
Assignee | ||
Comment 171•22 years ago
|
||
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•22 years ago
|
||
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.
Assignee | ||
Comment 173•22 years ago
|
||
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•22 years ago
|
||
Don't worry about the CSS1 test suite. :-)
Assignee | ||
Comment 175•22 years ago
|
||
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•22 years ago
|
||
looks good to me; qa=hixie
Comment 177•22 years ago
|
||
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•22 years ago
|
||
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).
Assignee | ||
Comment 179•22 years ago
|
||
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....
Assignee | ||
Comment 180•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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.
Assignee | ||
Comment 184•22 years ago
|
||
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.
Attachment #88644 -
Attachment is obsolete: true
Attachment #89942 -
Attachment is obsolete: true
Assignee | ||
Comment 185•22 years ago
|
||
Adding patch /sans/ whitespace, as requested.
Comment 186•22 years ago
|
||
Did you send email asking for super-review?
Assignee | ||
Comment 187•22 years ago
|
||
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•22 years ago
|
||
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.
Assignee | ||
Comment 189•22 years ago
|
||
Suggestion above implemented. Sorry for being unable to read english :-[
Assignee | ||
Updated•22 years ago
|
Attachment #92046 -
Attachment is obsolete: true
Attachment #92047 -
Attachment is obsolete: true
Assignee | ||
Comment 190•22 years ago
|
||
Comment 191•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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.
Assignee | ||
Comment 194•22 years ago
|
||
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...
Assignee | ||
Updated•22 years ago
|
Attachment #86988 -
Attachment is obsolete: true
Attachment #93061 -
Attachment is obsolete: true
Attachment #93062 -
Attachment is obsolete: true
Assignee | ||
Comment 195•22 years ago
|
||
Comment 196•22 years ago
|
||
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.
Assignee | ||
Comment 197•22 years ago
|
||
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•22 years ago
|
||
Any chance of a patch that addresses the comments?
Assignee | ||
Comment 199•22 years ago
|
||
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•22 years ago
|
||
I say skip on the caching for now and land the rest. We can then optimize
HasTextFrameChild() as a separate endeavor....
Assignee | ||
Comment 201•22 years ago
|
||
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...
Assignee | ||
Comment 202•22 years ago
|
||
Assignee | ||
Updated•22 years ago
|
Attachment #94210 -
Attachment is obsolete: true
Attachment #94213 -
Attachment is obsolete: true
Assignee | ||
Comment 203•22 years ago
|
||
Comment 204•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
+ * @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 :-)
Assignee | ||
Comment 207•22 years ago
|
||
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.
Assignee | ||
Comment 208•22 years ago
|
||
Attachment #99407 -
Attachment is obsolete: true
Attachment #99408 -
Attachment is obsolete: true
Comment 209•22 years ago
|
||
All I meant was to replace "if the appropriate bit above is set" with "if the
appropriate bit in aDecorations is set".
Comment 210•22 years ago
|
||
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...
Attachment #101439 -
Flags: superreview+
Assignee | ||
Comment 211•22 years ago
|
||
But of course. Your wish is my command :O) Thanks for spelling that last one
out....
Attachment #101439 -
Attachment is obsolete: true
Assignee | ||
Comment 212•22 years ago
|
||
Attachment #101440 -
Attachment is obsolete: true
Comment 213•22 years ago
|
||
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.
Assignee | ||
Comment 214•22 years ago
|
||
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.
Updated•22 years ago
|
Attachment #101870 -
Flags: superreview+
Assignee | ||
Comment 215•22 years ago
|
||
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!
Attachment #101870 -
Attachment is obsolete: true
Attachment #101871 -
Attachment is obsolete: true
Assignee | ||
Comment 216•22 years ago
|
||
Comment 217•22 years ago
|
||
nsBlockFrame.cpp:
+ // This for is standards mode only
shouldn't that be "This is for standards mode only"?
Comment 218•22 years ago
|
||
Comment on attachment 102946 [details] [diff] [review]
Merge conflicts resolved; textual nonsense removed
sr=bzbarsky still applies...
Attachment #102946 -
Flags: superreview+
Updated•22 years ago
|
Updated•22 years ago
|
Attachment #102946 -
Flags: review?(dbaron)
Comment 219•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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.
Attachment #102946 -
Attachment is obsolete: true
Updated•22 years ago
|
Attachment #102947 -
Attachment is obsolete: true
Comment 222•22 years ago
|
||
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.
Updated•22 years ago
|
Attachment #108525 -
Flags: superreview?(bzbarsky)
Updated•22 years ago
|
Attachment #102946 -
Flags: review?(dbaron)
Comment 223•22 years ago
|
||
It would be excellent if this could go into 1.3a to get some testing.
Flags: blocking1.3a?
Comment 224•22 years ago
|
||
I have a final till about 1pm CST tomorrow... I can try to review right after that.
Updated•22 years ago
|
Flags: blocking1.3a? → blocking1.3a-
Assignee | ||
Comment 225•22 years ago
|
||
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•22 years ago
|
||
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. ;)
Attachment #108525 -
Flags: superreview?(bzbarsky) → superreview+
Comment 227•22 years ago
|
||
Filed bug 184702 (more Paint refactoring) and bug 184703 (pre method on style
struct and fixing the XXX comment bz quoted above).
Comment 228•22 years ago
|
||
Taking this bug so I remember to check it in.
Assignee: esben → dbaron
Status: ASSIGNED → NEW
Whiteboard: [Hixie-P1][CSS1-5.4.3] (high profile: css1 test suite) underline not displayed properly across bold modifications [Hixie-B] → [patch][Hixie-P1][CSS1-5.4.3] (high profile: css1 test suite) underline not displayed properly across bold modifications [Hixie-B]
Comment 229•22 years ago
|
||
I removed the firstChildOrigin hack in nsBlockFrame::PaintTextDecorationLines
(and just used line->mBounds.x) since bug 145467 is fixed now.
Comment 231•22 years ago
|
||
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.)
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.3beta
Comment 232•22 years ago
|
||
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 233•22 years ago
|
||
Doh.
+ nscoord offset, size, baseline;
should be
+ nscoord offset, size, baseline = mAscent;
no?
Comment 234•22 years ago
|
||
Fixed.
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
Comment 235•22 years ago
|
||
VERIFIED.
Esben: If I'm ever in Copenhagen, I'll buy you dinner. :-)
Status: RESOLVED → VERIFIED
Comment 236•22 years ago
|
||
This caused regression bug 185581 (which was simple to fix).
Comment 237•21 years ago
|
||
*** Bug 44232 has been marked as a duplicate of this bug. ***
Comment 238•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
||
> 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•18 years ago
|
||
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•18 years ago
|
||
I don't see the issue on branch, so maybe this bug isn't directly responsible; fallout from the paint refactoring perhaps?
Comment 244•18 years ago
|
||
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...
You need to log in
before you can comment on or make changes to this bug.
Description
•