10713 - (text-shadow) Implement CSS3 text-shadow property

Status: VERIFIED FIXED

(Core :: Layout: Text and Fonts, defect, P1)

Description

[tom]

Text Shadow feature for DOM/CSS not implemented. The URL "http://
www.dizoglio.com/~bugzilla/text-shadow.html" contains the test case for
reproducing this problem. This feature is needed for the embedded device we are
porting Gecko too. This feature is part of the DOM Level 2 and CSS Level 2
specification but are not implemented in Gecko.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This is probably just a layout issue.

I don't think it would be that hard to implement, either, because it's up to the
author of the CSS to make sure there's room (I think).

Comment 2

[kipp]

Added {feature} marker. There is no current plan for implementing this in gecko
(we have promised 100% css1 and spotty css2). If you really need it, please
start a dialog in netscape.public.mozilla.layout and maybe somebody will write
the code for you, or the discussion will lead to netscape implementing it.
Thanks.

Comment 3

[Hixie (not reading bugmail)]

Kipp: Shouldn't this bug be marked LATER then?

Comment 4

[Hixie (not reading bugmail)]

Migrating from {css2} to css2 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 5

[buster]

mine now

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

BTW, I think this would be quite easy to implement, except for the blur effects,
which would be quite difficult, and might be best done partly in the view system
/ compositor.

Comment 7

[buster]

accidentally cleared LATER resolution

Comment 8

[[e-mail]]

This would be nice for the Windows Classic skin - I think that the equivalent
CSS style for a disabled Windows control would be

  color:	threedshadow;
  text-shadow:	1px 1px threedhighlight;

Comment 9

[Hixie (not reading bugmail)]

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

Comment 10

[Hixie (not reading bugmail)]

See also: http://bugzilla.mozilla.org/showattachment.cgi?attach_id=18107

Comment 11

[Thomas Swan]

I think the more CSS2 support you have from the start the better you will be. 
Granted, I fear this tag will be abuse; however, I think it is worth having.

Comment 12

[[e-mail]]

Strangely it's also listed in Netscape's list of supported CSS Level 2 styles!

Comment 13

[Hixie (not reading bugmail)]

Nominating this bug for nsbeta1 on behalf of [e-mail].

Comment 14

[Jacob Kjome]

This one is important for those who want to do stuff with text that only images
allow right now.  We can cut down on the unnecessary use of images by
implementing this.  If it is as easy to do as David Baron says, I say let's
implement this thing.  It would be one more cool thing that IE and Opera 5.x
cannot do.  BTW, has anyone tested this in IE6 beta?  If IE6 does this, then
Mozilla *HAS* to do this!!!

Jake

Comment 15

[Jacob Kjome]

I checked IE6 beta and they haven't implemented text-shadow, but they have 
something similar to it implemented in their "filters".  You can see an example 
here:

http://members.nbci.com/iainsweb/shadtextdhtml.htm

it is implemented something like this:

<span style="filter:shadow(color=aqua,direction=45)"><H1>45 shadow</H1></span>

This is, obviously, proprietary stuff...but at least they can do it.  Let's 
implement this in Mozilla in a standard way.  That would be something to brag 
about!

Jake

Comment 16

[Jacob Kjome]

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

Comment 17

[Thomas Swan]

It's really important that we do this...

Comment 18

[Hixie (not reading bugmail)]

Actually on the cosmic scale of things I would say this is one of our least
important bugs...

Comment 19

[Chris Lyon]

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

Comment 20

[Kevin McCluskey (gone)]

Build reassigning Buster's bugs to Marc.

Comment 21

[Thomas Swan]

Any hope of this one any time soon? :*)

Comment 22

[Greg K.]

Thomas, as an enhancement, this isn't likely until after 1.0.

Comment 23

[Thomas Swan]

Just wishful thinking on my part... 

Comment 24

[Dan Rosen]

I'd argue that this isn't exactly an RFE, it's more of a request for standards
support... Not to say this particular part of the CSS2 standard is
overwhelmingly necessary, but it's not an odd feature request.

  http://www.w3.org/TR/REC-CSS2/text.html#propdef-text-shadow

P5/Future seems just right, though :) FYI, IE6 does not support this. Be not afraid.

Comment 25

[nikolai]

IE5.5 Supports this.

Comment 26

[Jacob Kjome]

Nikolai,

I'm almost positive that it is untrue that IE5.5 supports text-shadow.  Please 
produce a testcase that proves this.  The testcase that Hixie referred to does 
nothing in IE5.5.

http://bugzilla.mozilla.org/showattachment.cgi?attach_id=18107

Jake

Comment 27

[Thomas Swan]

I checked with 5.5 this morning after the post, thinking I missed something, and
 IE does not support the CSS2 text-shadow property.   However, IE 5.5 and 6.0, I
think support text shadows but only by using "filters" not the text-shadow property.

Comment 28

[nikolai]

Ups, it was "filters" not the text-shadow.

Sorry.

Comment 29

[rubydoo123]

leaving in future bucket

Comment 30

[Daniel Steinberger]

To the first comment (#1) from David Baron, I just want to add this quote from
the CSS RFC at http://www.w3.org/TR/REC-CSS2/text.html#propdef-text-shadow - it
states that "...Shadow effects do not alter the size of a box, but may extend
beyond its boundaries...". So on how I interpreded this comment he worried about
varying box sizes, which would be obsolete when following the description.

Well, as everybody else, I also think, that it would not be that hard to
implement. Still I understand that due to the Mozilla1.0 manifest, this is a
feature, which costs time and resources better invested on performance and
stability issues. But I'd be strongly pleased, if this would go in shortly after
1.0 is released, since it could really cut down use of images and so reduce
traffic after all, if once IE and Opera implement this one, too =(
But still, someone has to do it first.

Comment 31

[Daniel Steinberger]

sorry for the spam. added myself to CC

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This would be hard to implement fully.  See comment #6.

Comment 33

[basic]

remove self

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 35

[Martin van Dijken]

I very definitely agree that Mozilla 1.0 is a necessary step, just for the
record. I do however feel that if Mozilla 1.0 is going to be released claiming
it fully supports CSS2, that it's required to implement this declaration as
well, since I couldn't find anywhere in the CSS2 declaration that this
declaration is unrequired. I quote from the W3 CSS2 definition:

It must parse the style sheets according to this specification. In particular, it
must recognize all at-rules, blocks, declarations, and selectors (see the
grammar of CSS2 [p. 309] ).

End quote. It in this case is a conforming User Agent. I as a web-developer find
it vey hard to swallow that a browser that claims to support something, doesn't
fully do so. Of course you'll say that Explorer doesn't even support half of
CSS2 and you're right about it. However if I ask Explorer whether it supports
CSS2, I get a solid no in return. In my opninion, Mozilla 1.0 must either
support this without bugs, or state NO when I ask for CSS2 support. I personally
am sick and tired of writing my code and layout browser-dependant.

Comment 36

[Hixie (not reading bugmail)]

Mozilla 1.0 will not be released claiming it fully supports CSS2.

Heck, Mozilla 1.0 will not be released claiming it fully supports CSS_1_.

Comment 37

[Martin van Dijken]

Hmm correct me if I'm wrong, but what does the result of this question mean then?

alert("Browser supports CSS 2:"+document.implementation.hasFeature("CSS","2.0"));

Comment 38

[Hixie (not reading bugmail)]

That's DOM2CSS support, not CSS2 layout support.

Comment 39

[Martin van Dijken]

Oh gross, forgot about that.

I will withdraw my earlier statement seeing as I've lost all ground to stand on.
I would still very much like this feature though:).

Comment 40

[Madhur Bhatia]

shouldn't this come under the Style System component?

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No.  I think the style system part is already implemented.

Comment 42

[Dan Libby]

So 1.0 is out now.  and 1.1.  and 1.2a...   any updates on this bug/RFE?

Comment 43

[Hixie (not reading bugmail)]

Not going to happen before we fix many of the bugs with our _existing_ features.

Comment 44

[Christopher Aillon (sabbatical, not receiving bugmail)]

Not going to happen unless someone steps up to the plate and writes the code.  I
want this feature like the rest of you, but let's face it, it's likely not gonna
happen for quite a long while.

Kicking off to nobody and adding helpwanted so that those that look for this
kind of stuff can find it.

Comment 45

[Quick Silver]

Sorry for spam, added self.

Comment 46

[Boris Zbarsky [:bzbarsky]]

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

Comment 47

[Boris Zbarsky [:bzbarsky]]

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

Comment 48

[Boris Zbarsky [:bzbarsky]]

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

Comment 49

[Marco Perez [:mpk]]

additional information:
The "text-shadow" property has been removed from (or maybe never been part of)
CSS 2.1 (see http://www.w3.org/TR/CSS21/changes.html#q21).

Comment 50

[Scott Thorne]

2.1 captures the state of support for CSS 2 in current browsers.  Like various
aspects of the spec, since text-shadow has never been implemented in browsers,
it gets dropped from the 2.x cycle.  But that doesn't mean it's gone.

It is still in the current draft of the CSS3 Text module.  See
http://www.w3.org/Style/CSS/current-work and esp.
http://www.w3.org/TR/2002/WD-css3-text-20021024/#text-shadows

This touches on the broader topic, to what extent and on what schedule will
gecko support the new CSS3 modules?

CSS3 is much broader than CSS2.  Initially, it would probably easier to be
compliant with only some of the modules, but not necessarily all.  I would hope
the longer term goal would be to support ALL applicable modules, including the
Text module ;-)

Comment 51

[Hixie (not reading bugmail)]

Let's focus on our CSS2.1 bugs and CSS2 bugs before we run off and implement
CSS3. Please. :-)

Comment 52

[Christian :Biesinger (don't email me, ping me on IRC)]

note- CSS 3 Text Module (Candidate Recommendation) includes text-shadow:
http://www.w3.org/TR/2003/CR-css3-text-20030514/#text-shadows
Implementing the blur radius part is optional.

Comment 53

[Gérard Talbot]

MSN-TV supports CSS2 text-shadow property impeccably (not filters like in MSIE
5.5+). I tested this with several pages so far. I think it's the first web-aware
application capable of doing it.

List of CSS support in the MSN-TV browser:
http://developer.msntv.com/develop/elements.asp

You can verify this by downloading the MSN-TV viewer v. 2.8 (build 20).

Comment 54

[Erik Arvidsson]

Safari 1.1 also supports text-shadow so this feature is actully gaining some ground.

Comment 55

[Arthur Wiebe]

It's already at 1.6 Alpha, When is someone going to impliment text-shadow? Safari 1.1 already 
supports it and I use it on my web site. It's a very cool effect. If Mozilla can't impliment all the 
standards it's just going to fall back.
I would do it if I could, but can't.

Comment 56

[Henrik Gemal]

New URL since old URL didn't work anymore.

Comment 57

[Stefan Urbat]

I can't confirm, that Safari 1.1.1(v100.1) due to it's about entry displays
anything of text-shadow. Look at http://www.aadmm.de/ under browser, and scroll
down: it fails as miserably as any other browser out there.

Comment 58

[Mike Utke]

I'm not convinced that's a valid test.  (http://www.aadmm.de/)  The text-shadow
property is specified as 'text-shadow:black;'.  From the CSS2 spec: "Each shadow
effect must specify a shadow offset and may optionally specify a blur radius and
a shadow color."  I'm not seeing an offset.  If/When this is implemented, I
don't think it needs to support this particular case.

Comment 59

[Stefan Urbat]

Please try the essential extract of the mentioned URL with this link directly in
the official w3c validator:

http://jigsaw.w3.org/css-validator/validator?text=.rotschattig++%7B+text-shadow%3Ablack%3B+font-size%3A10pt%3B+color%3Ared%3B+%7D&warning=2&profile=css2&usermedium=all

before you write nonsense: it is verified as 100% conforming to the CSS
standards in the hardest evaluation level there and proves, what the URL, the
German SelfHTML documentation state and my experience confirms: no browser at
all shows anything of this correct coded CSS2 feature, though it is correct.

Comment 60

[Hixie (not reading bugmail)]

CSS2 self-contradicts itself (look closely, the value line disagrees with the
spec). CSS3 clearly states that two lengths must be given.

The CSS validator has many issues, this is just one of them.

Comment 61

[Christian :Biesinger (don't email me, ping me on IRC)]

the grammar for the Value at
http://www.w3.org/TR/CSS2/text.html#text-shadow-props contradicts the text.
presumable the validator checks against the grammar.

the text says:
"Each shadow effect must specify a shadow offset and may optionally specify a
blur radius and a shadow color."

and CSS3 Text Module CR is also clear on that:
<shadow> itself is defined as "[<color> <length> <length> <length>? | <length>
<length> <length>? <color>?]".
and:
Each shadow effect MUST specify a shadow offset and MAY optionally specify a
blur radius and a shadow color.

Comment 62

[Stefan Urbat]

Under these circumstances I would propose, that we let this bug alone, don't use
text-shadow at all and wait, until the w3c has cleared up things without any
remaining doubt with the CSS3 final documents --- anything else seems to be a
waste of time for me. Sorry for any comment I made so far!

Comment 63

[Hixie (not reading bugmail)]

There is no doubt; the CSS3 syntax is what the WG intends.

Comment 64

[Kevin Newman]

This feature, in whatever form the css recommendation takes will require the use
of opacity, and blur features. Since we already have -moz-opacity (now just
opacity), could something like -moz-blur be implemented in the mean time? Then
implementing when it comes time to implement text-shadow, the blur filter/style
will already be implemented.

Also, here is an example where someone made a hacky work around to get this
feature working in MSIE:

http://www.douglas.stebila.ca/code/ie7-text-shadow/
http://www.douglas.stebila.ca/code/ie7-text-shadow/test.html

Comment 65

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #64)
> This feature, in whatever form the css recommendation takes will require the use
> of opacity, and blur features.

"User agents MAY only implement only part of this property by ignoring blur
effects. "
http://www.w3.org/TR/css3-text/#text-shadows

Comment 66

[nrlz]

Attachment: safari with text-shadow

Safari does render the text-shadow style. Here's a screenshot of the page from
comment #10

Comment 67

[tikitype]

There are already a bunch of websites out there, well designed sites too that
use the text-shadow attribute in really nice ways, not only drop shadows. It can
be used in a great way to lift text from a busy background picture when you add
captions to images and left them float in, anything like that, you get the picture.

This is actually my first post here and I joined to make a point on this
particular feature since it is really important to me as a designer. 

Comment 68

[Jonathan Haas]

An implementation would be very nice, because very well looking themes
(especially with engraved text) could be created easily. 

Comment 69

[Anne (:annevk)]

Bugzilla is not intended for encouraging people to implement it. Please don't
post such advocacy comments here.

Comment 70

[tikitype]

(In reply to comment #69)
> Bugzilla is not intended for encouraging people to implement it. Please don't
> post such advocacy comments here.

It's not quite advocacy here. It IS a bug in the rendering engine of Firefox
that prevents it from displaying the text-shadow declaration of valid CSS.

Step down from your knight's horse and put yourself in the perspective of the
web developer that has to find workarounds for this. Or even exclude Firefox
because they got annoyed.

I don't mean to be abrasive here but a bug should be seen as a bug.

Comment 71

[Hixie (not reading bugmail)]

We know it's a bug. This is a bug database, not a forum. Asking for the bug to
be fixed will have no effect. Only submitting a patch or paying someone to fix
the bug for you will have any effect.

Comment 72

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No, it's not a bug.  It's an enhancement request.

You claim that it's a bug that we don't handle all CSS.  Would you make the same
claim if we didn't handle all XForms?  All SVG?  All XYZ?  All ABC?   What makes
it a bug that we don't implement something if we've never claimed or promised to
implement it?  Or are all software products required to implement all standards
ever created?

Comment 73

[Hixie (not reading bugmail)]

IMHO, an enhancement request is either denied, in which case the bug is marked
resolved, or left open, in which case it is a bug. If we don't want to
eventually implement text-shadow (i.e. if we don't think this is a bug) then
this should be marked WONTFIX. Personally, I would put text-shadow on the pile
of things that Web browsers should support, and thus consider this a valid bug.

Comment 74

[tikitype]

(In reply to comment #73)
> IMHO, an enhancement request is either denied, in which case the bug is marked
> resolved, or left open, in which case it is a bug. If we don't want to
> eventually implement text-shadow (i.e. if we don't think this is a bug) then
> this should be marked WONTFIX. Personally, I would put text-shadow on the pile
> of things that Web browsers should support, and thus consider this a valid bug.

I totally agree with you on this, Ian as it would cut down the need for image
replacement techniques by a good chunk.

And, yes David, I know you can't have them all ;)
I'm just looking for stuff that makes life easier for us designers.

I'll shut up now and leave some room for the real coders ;)

Comment 75

[Joseph Birthisel]

Any movement on this?

Comment 76

[Kevin Newman]

I'm guessing that once the cairo backend has been added, we will get more
movement on these types of feature requests. Is that a safe assumption?

Comment 77

[Kevin Newman]

It looks like gaussian blur has been added to the SVG module, so can I assume it
should be possible to use that work to make the drop shadow blurred (bug 301234)?

If adding the gaussian blur to the drop shadow becomes part of the goal of this
bug (or another) then bug 251414 should be set as a duplicate of this (or the
other).

Comment 78

[Christian :Biesinger (don't email me, ping me on IRC)]

(I will not implement blurring as part of this bug)

Comment 79

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

Comment 80

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So, at first glance, the way you only look at the parent because it's a reset
property seems wrong.  It seems like you need to look at all ancestors, or
perhaps all inline ancestors.  It's not clear from the spec -- nor is it clear
what the painting order should be when multiple 'text-shadow's need to be
painted for the same text.  You should post to www-style to get the spec clarified.

Comment 81

[Trejkaz]

"nor is it clear what the painting order should be when multiple 'text-shadow's
need to be painted for the same text."

From the spec:

"The shadow effects are applied in the order specified"

That's pretty clear, IMHO.

Comment 82

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No, that's for when multiple shadows are specified on the same element; I'm
talking about when multiple shadows are specified for different elements.

Comment 83

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That said, the CSS WG had a discussion in July 2003 about whether text-shadow
should be inherited.  I don't think a formal decision was ever reached, but
Safari may have implemented in this way.  If so, we should probably follow; IMO
it's a better model.

Also, you should look into the interaction of text-decoration and text-shadow --
whether the decorations should be shadowed (depending on which element has the
decoration, too).  Testcases for this are also needed, and it's worth testing
interoperability with Safari (which I think is the only other implementation).

Comment 84

[Trejkaz]

I guess you meant "different, nested elements", like this:

<span style="text-shadow: 3px 3px red;">some text and then
  <span style="text-shadow: -3px 3px yellow;">some more</span> text</span>

Intuitively, I don't see why the semantics should be any different from...

<span style="color: red;">some text and then
  <span style="color: blue;">some more</span> text</span>

...except if anything, it's simpler for text-shadow because it explicitly says
in the spec that it isn't inherited, so there is no possible way for the outer
one to leak inward.

I guess, as was said right after, it would be different *if* text shadows were
later changed to be inherited.

Comment 85

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No, that's not what the spec says.  With your interpretation,

  <p style="text-shadow: 3px 3px red">This would have a shadow <span>but this
  would not.</span></p>

which is very broken.

And if we make it inherited (which I already said would make more sense), then
it would be more like 'color', but currently it's not.  Currently it's like
'text-decoration'.

Comment 86

[Trejkaz]

True, it would probably make more sense.  But I was talking about the
specification that exists now, not one which exists after they fix it.

Comment 87

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #80)
> It seems like you need to look at all ancestors, or
> perhaps all inline ancestors.  

If I look at all ancestors, then this is really an inherited property, and if
that was the intent, why wasn't it specified as such? Anyway, I posted to
http://lists.w3.org/Archives/Public/www-style/2005Sep/0193.html

The text of the specification says the text is shadowed... it doesn't mention
decorations...

I'll make some testcases to compare with safari.

Comment 88

[Hixie (not reading bugmail)]

I have attempted to answer your questions based on the discussion to which
dbaron referred earlier.

   http://lists.w3.org/Archives/Public/www-style/2005Sep/0196.html

However, his point about testing Safari to make sure it does indeed do what I
claim it does in the above e-mail still stands. If it turns out Safari doesn't
do as claimed, please post to the list again and berate me!

Hope this helps.

Comment 89

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This patch may render incorrect with smallcaps.

I.e,
<p style="text-shadow: 3px 3px red; font-variant: smallcaps;">

In this case, the patch may not draw the text with smallcaps.
I think that we should use nsTextFrame::RenderString for drawing shadow.

Comment 90

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

And I have a question. Don't need text-decoration for shadow?

Comment 91

[[e-mail]]

So, when will we get this for XUL ;-)

Comment 92

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #89)
> This patch may render incorrect with smallcaps.
> 
> I.e,
> <p style="text-shadow: 3px 3px red; font-variant: smallcaps;">

That needs to be small-caps, and with that, it works for me...

> I think that we should use nsTextFrame::RenderString for drawing shadow.

Eh, it was the RenderString function that I patched...

(In reply to comment #90)
> And I have a question. Don't need text-decoration for shadow?

Yeah, according to hixie's mail. (Not according to the current spec wording, as
best as I can tell)

(In reply to comment #91)
> So, when will we get this for XUL ;-)

I hope that this patch does work for xul, so "as soon as I implement what hixie
described" ;)

Comment 93

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> That needs to be small-caps, and with that, it works for me...
> 
>> I think that we should use nsTextFrame::RenderString for drawing shadow.
> 
> Eh, it was the RenderString function that I patched...

Oops... Sorry, It's my mistake. You are right. PaintTextShadows is called by
RenderString.

Comment 94

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: testcases for safari

Comment 95

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: Safari's rendering of the testcase

thanks to Mnyromyr for the screenshot

Looks like safari does implement text-shadow as described above. Hm... this
testcase doesn't test the drawing order when different elements are involved...

Comment 96

[Jure Repinc (JLP)]

Konqueror from KDE 3.5 SVN renders the testcase similarly to Safari. Except 
that it never creates shadow for underline. 

Comment 97

[Karsten Düsterloh]

Comment on attachment 197060 [details]
Safari's rendering of the testcase

JFTR: it's Safari 2.0.1 (412.5)

Comment 98

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: another testcase for comparison

could someone make a screenshot of this testcase with safari and the konqueror
version that supports text shadows? thanks!

Comment 99

[Mike Utke]

Attachment: Last testcase under Version 2.0.1 (412.5) with a wide window

Comment 100

[Mike Utke]

Attachment: Last testcase under Version 2.0.1 (412.5) with a more narrow window

There is an interesting difference in Safari when resizing the window.	The
difference in wide vs. slightly narrower window for these test cases is so 
dramatic enough that I wanted to include both.

Comment 101

[Christian :Biesinger (don't email me, ping me on IRC)]

Thanks! (It was the wide version I was after. The difference is because I
positioned the shadow at a certain offset from the middle paragraph so that it
appears in the same position as the other ones. If the line wraps that doesn't
work so well, of course)

Comment 102

[Jure Repinc (JLP)]

Attachment: Latest testcase with Konqueror from KDE 3.5 SVN

Comment 103

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: quirks mode testcase

testcase for quirks mode behaviour. I assume this to be only relevant for
gecko, so no screenshots of safari/konqueror needed.

Comment 104

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

implementation as specified above.

Comment 105

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 197362 [details] [diff] [review]
patch v2

>+struct nsTextShadowItem {
>+  nsStyleCoord mXOffset;
>+  nsStyleCoord mYOffset;
>+  nsStyleCoord mRadius; // Can be Unit_Null

Why can it be null?  Unspecified is the same as zero, so just make it zero. 
(Right?  The spec doesn't actually say that, but it sure seems to me like they
should be the same.)

Comment 106

[Christian :Biesinger (don't email me, ping me on IRC)]

ah, yeah... good point. (the CSS3 CR does mention that,
http://www.w3.org/TR/2003/CR-css3-text-20030514/#text-shadows)

Comment 107

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, why use nsStyleCoord at all?  Most uses of nsStyleCoord have comments
explaining which types of nsStyleCoord they can be (although some of the newer
ones don't).  If there's only one type, there's no need to use it.  (I think
there's some existing overuse, which I'll file a bug on after I land the patch I
just attached to bug 205790, but that doesn't mean we need more.)

Comment 108

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hrm, the reason for nsStyleCoord seems to be the _Chars mess.  Probably better
to be consistent with theother code for now, then.

Comment 109

[Christian :Biesinger (don't email me, ping me on IRC)]

seems to me like only nsStyleOutline and nsStylePosition coord members have a
comment describing what kind of units they store... (and
nsStylePosition::mZIndex doesn't have one)

Comment 110

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v3

I couldn't find a way to convert eStyleUnit_Chars to nscoord, except the
protected member of the reflowstate...

Comment 111

[Ryan Parman]

Not to spam, but is there a Firefox build that has this patch included?  Is this
something an end-user could provide feedback on, or is it really just
developer-only at this point?

Comment 112

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Some comments from skimming the patch quickly:
 * the new CalcDifference is wrong; you need to compare the text shadow
information, not just the length
 * if you're not handling radius yet, you should change the parsing code not to
accept it (probably #if 0)

Comment 113

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 197778 [details] [diff] [review]
patch v3

In nsHTMLContainerFrame::PaintDecorationsAndChildren, you need to change
the part that paints strikethrough (after PaintChildren) to also check
  eCompatibility_NavQuirks != aPresContext->CompatibilityMode()
since |decorations| can now be set to non-none even when we are in
quirks mode.

In nsHTMLContainerFrame::PaintTextDecorations, please rename |offset| to
|baseoffset| to avoid confusion.

nsHTMLContainerFrame::PaintTextDecorationLines shouldn't have a default
value for the last parameter.  It's unneeded, and especially confusing
for a virtual function.  (The default value for PaintTextDecorations is
sufficient.)

nsTextFrame.cpp:

  In nsTextStyle's constructor, use one line of whitespace instead of
  two.

  If you're removing nsTextFrame::GetDocument (fine with me), remove it
  from the .h file as well (and make sure things still compile!).
  (Presumably any callers were replaced with
  GetContent()->GetOwnerDoc(); I don't see any removal of callers in
  your patch, and I'd hope you'd have gotten a link error if there were
  any.)

  The basic architecture of what you did here gets the z-order wrong
  according to Ian's message.  For a start, if there's a font change in
  the middle of a text frame, you can get shadow of the later font
  painting on top of the main text of the previous font.  It also has
  the same problem for <span>A<span>B</span>C</span>, where C's shadow
  can paint over B's text (which disagrees with Ian's message).  I think
  to fix these you should pull things out a few layers (and hopefully
  with no need to call PaintTextSlowly when shadows are involved,
  although maybe I'm too optimistic).

  Also, it seems like you might be painting shadows twice in quirks
  mode.  I didn't trace through the code very carefully, though.
  Perhaps the HTMLContainerFrame stuff should stay all
  standards-mode-only, so that you maintain the invariant that the
  shadow lines up with what's actually painted?  Or am I missing
  something?

  The overflow munging in nsTextFrame::Reflow should mess with the
  overflow area in one step and set/clear the NS_FRAME_OUTSIDE_CHILDREN
  bit in another; you don't want to set NS_FRAME_OUTSIDE_CHILDREN for a
  shadow at position zero.  (Something else may even handle the latter
  half already, although it may not for text frames.  But you may need
  to add it, since I think we have an invariant that frames with
  NS_FRAME_OUTSIDE_CHILDREN have a frame property pointing to a rect
  with their overflow area.  I'm not sure if we switched to doing that
  for all frames, though -- please check.)

nsComputedDOMStyle.cpp:

  Please test computed style in DOM inspector, including for complicated
  cases like those in attachment 196151 [details] (esp. the third paragraph) to make
  sure that the computed style value is formatted correctly.

  The computed style code should not generate a value list for the 'none'
  value.  It should just generate a value.  You should probably also
  generate the 'none' value if 'text' is null, instead of generating an
  empty list.

  Your "Wrong unit" assertions all test the same values rather than the
  successive different ones.

nsRuleNode.cpp:

  +      if (arrayLength) {
  This is guaranteed non-null, since you're within an |if (list) {|, so
  don't bother checking it, and undo one level of indent (yay!).

  +          for (nsTextShadowItem* item = text->mShadowArray;
  +               list;
  +               list = list->mNext) {

  Move the ++item into the for() as well: "list = list->mNext, ++item"
  works.

  The 3 "nsStyleCoord()" are probably more efficient as a single named
  variable instead of three anonymous ones.

  +          }
  +        }
  +      }
  +
  +    }
  +  }
  No need for blank lines to prevent it from lining up.

nsStyleStruct.cpp:

  Could you leave nsStyleTextReset::CalcDifference alone?  The changes
  are slightly slower, and I don't want to review them carefully since I
  don't see the advantage of the new way.

  Your nsStyleText::CalcDifference is wrong, as I mentioned in the
  previous comment.  You need to compare the items, since two lists of
  the same length with different content do need to trigger a reflow.

nsCSSParser.cpp (not currently in patch):
  You need to #if 0 out the code that parses the radius, since you're
  not handling it.  (We need to reject shadows with radius as a parser
  error.  You're free to leave the other radius code, though.)

Comment 114

[Christian :Biesinger (don't email me, ping me on IRC)]

now that display lists have landed, some of this code can be simplified, fixing various problems (like that font switching problem mentioned in the review comments)

Comment 115

[Christian :Biesinger (don't email me, ping me on IRC)]

I'm not sure that my nsStyleText::CalcDifference code is wrong. It looks like this:

[if]
+      (mShadowArrayLength == 0) &&
+      (aOther.mShadowArrayLength == 0))
     return NS_STYLE_HINT_NONE;
   return NS_STYLE_HINT_REFLOW;

I.e. it triggers a reflow if either list has a shadow. While this is not as optimal as it could be, I don't think it's wrong. Do you want me to write a more efficient version (i.e. actually compare the items)?

Comment 116

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v4

merged to trunk, some review comments addressed, some left to do

Comment 117

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

blocking+ for 1.8.1.  (Still need to see a final patch.)

Comment 118

[David E. Ross]

I thought Mozilla products were supposed to be compliant with W3C specifications.  So let us look at the current state of CSS specifications.  

The Text module cited in comment #50 (2nd link) has been superceded by a splitting of the module into Text Effects and Text Layout.  The text-shadow property has been relegated to Text Effects.  The current specification for Text Effects is at <http://www.w3.org/TR/css3-text/> and is dated 27 June 2005.  It states: 
"Many sections intended for this module are not yet represented in this draft. In particular, the 'text-justify-trim', 'text-indent', 'text-overflow', 'text-decoration', 'text-transformation', 'punctuation-trim', 'text-autospace', 'text-shadow', 'hanging-punctuation', 'kerning-mode', and related properties have not yet been evaulated."  

Thus, there is no current specification for the text-shadow property, even in draft form (the property in CSS2 having been dropped in the CSS2.1 specification).  To me, that creates enough uncertainty in the final syntax and meaning of the property that this bug should be deferred for now.  

Note that the last scheduled milestone for Text Effects is "last call for comments", which is this year.  The five subsequent milestones for this module are still unscheduled.  

In the meantime, I must agree with comment #51.  There are enough bugs relating to the current (11 April 2006) CSS2.1 specification requiring attention that efforts and resources should not be diverted to implementing a specification that does not have a schedule for final publication, especially for a property that does not even have a current draft specification.  

However, efforts on CSS2.1 bugs should be cautious since that specfication has not yet reached formal Recommendation.  The Implementation Reports are already five months late.  The current specification is merely a third working draft for CSS2.1, published just a month ago.  

Comment 119

[Christian :Biesinger (don't email me, ping me on IRC)]

> Thus, there is no current specification for the text-shadow property, even in
> draft form

CSS 2 is still a recommendation. The version that supercedes it is only a working draft. 

Comment 120

[Kevin Newman]

There is also a working implementation of the text-shadow property in Safari (complete with gausian blur), so feature parity is another reason to consider  this feature (and an implementation to follow in cases where the w3c is unclear if it isn't).

Comment 121

[Bamm Gabriana]

For me the reason for implementing this would be for use in theme development. Particularly, I would use this for creating disabled text, such as in buttons.

Comment 122

[Mike Beltzner [:beltzner, not reading bugmail]]

Taking this off the blockers list for 1.8.1 on behalf of 181-drivers.

Comment 123

[Anne (:annevk)]

(In reply to comment #119)
> CSS 2 is still a recommendation. The version that supercedes it is only a
> working draft.

For everything you're implementing you really want to refer to CSS 2.1. Working Draft or not. This is not to say it's a problem to implement 'text-shadow'. It's been dropped from CSS 2 mainly because there have been no implementations. It's no longer in CSS 3 mainly because that specification is being rewritten (not because the syntax is going to change or something like that).

Comment 124

[Luke-Jr]

> It's been dropped from CSS 2 mainly because there have been no
> implementations.

Exactly. And because the decision to drop it from CSS 2.1 is based on this false premise, that decision should be ignored. This is the tip of the iceberg as to why Konqueror is superior to FireFox (which has taken a role as best of Windows-- but certainly not of other OS).

Comment 125

[Samuel Sidler (old account; do not CC)]

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

Comment 126

[Zibi Braniecki [:zbraniecki][:gandalf]]

Should it work anyhow on trunk after biesi's checkin? It doesn't for me on latest trunk

Comment 127

[Steffen Wilberg]

Which checkin? "Patch v4" is still a work-in-progress, see comment 116.

Comment 128

[Mardeg]

Heads up folks, Opera 10 will have text-shadow.
http://www.css3.info/blog/css3-in-future-opera-builds/#more-67

So along with Konqueror, Safari and iCab this 2.1 spec omission is looking more and more like a false premise.

Comment 129

[Sierk Bornemann]

Apple Safari nightly has it, Opera 10 has it. Firefox should not stay far behind. So, please implement this CSS2 feature and feed it into the trunk, if a working patch is available. Please sooner than too late.

Comment 130

[Mats Ahlgren]

I'd like to similarly voice concern. Text-shadow is an extremely useful tool in terms of making usable UIs (no, really -- it's invaluable for increasing contrast and making webpage elements easier on the eyes).

Every other browser I know (Safari, Konqueror, ...) has had support for text-shadow for awhile now*, except Firefox.

+ Please, don't slow down progress. +

It is very frustrating to want to overlay text onto something with a background, and your only options to increase contrast (i.e. prevent white-text-on-light-part-of-the-picture, or dark-text-...-picture) are 1) text-shadow (not supported) 2) the text-shadow CSS hack for Firefox (goes to show you how desirous users are of this feature... the hack doesn't usually work though) 3) use a semitransparent png background (and semitransparent pngs not only make the background ugly, but also slow down scrolling as per bug 64401).


*(except IE, which supports something similar but proprietary)

Comment 131

[Amy Brodzik]

I am going to vote too... this is a useful feature. It would be nice to see all mainstream browsers supporting this feature.

Comment 132

[Amy Brodzik]

I am going to vote too... this is a useful feature. It would be nice to see all mainstream browsers supporting this feature. This page looks fine on Konqueror: http://icab.clauss-net.de/textshadow.html

Comment 133

[Mats Ahlgren]

I'm shocked that the final alpha7 version of Firefox 3 does not have CSS2. In three days, this bug will have been on file for 8 years...

Firefox 3 cannot claim decent CSS2 support without this incredibly useful feature.

When people start realizing how useful it is, and text-shadow use spreads, Firefox users will be left wondering why they can't see text sometimes and why the contrast is so bad on the Gecko engine. No offense, but this will be why. Please keep up the good work and integrate the patch with Firefox 2 and 3.

Comment 134

[Daniel Brooks [:db48x]]

(In reply to comment #133)
> When people start realizing how useful it is, and text-shadow use spreads,
> Firefox users will be left wondering why they can't see text sometimes and why
> the contrast is so bad on the Gecko engine. No offense, but this will be why.
> Please keep up the good work and integrate the patch with Firefox 2 and 3.

If it is so useful, why is it taking 8 years for it to become wide spread?

Comment 135

[Dão Gottwald [:dao]]

(In reply to comment #129)
> Apple Safari nightly has it, Opera 10 has it.

Opera 9.5 is going to support text-shadow.

Comment 136

[Amy Brodzik]

> If it is so useful, why is it taking 8 years for it to become wide spread?

Because Firefox and IE, the two most popular browsers at the moment, do not support it at all.

Comment 137

[David E. Ross]

CSS 2.1 advanced to "Candidate Recommendation" on 19 July (a week ago), on track to replace CSS 2.  CSS 2.1 does NOT include text-shadow.  

In CSS 3, "Text Effects" (see my comment #118) is now merely "Text".  The latest working draft of "CSS 3 Text" (6 March 2007) still includes text-shadow.  However, comments in the specification clearly indicate that the meaning of the text-shadow property remains in flux.  Further, CSS 3 might not contain a complete text-shadow specification.  The working draft of "CSS 3 Text" states:
"The following features are at risk and may be cut from the spec during its CR period: multiple text shadows, the tibetan text justification mode, the 'text-outline' property, the 'break-strict' value of 'word-break'"

Much of CSS 2.1 is not yet implemented in the Gecko core.  I still argue that complete implementation of CSS 2.1 should have priority over the implementation of speculative features of CSS 3.  I also argue that CSS 2 is already a dead issue (or will be very soon) and that no effort should be made to implement a capability specified in CSS 2 unless that same capability is also specified in CSS 2.1.  

Comment 138

[Dão Gottwald [:dao]]

(In reply to comment #137)
> CSS 2.1 advanced to "Candidate Recommendation" on 19 July (a week ago), on
> track to replace CSS 2.  CSS 2.1 does NOT include text-shadow.

Because nobody implemented it. Now that two vendors implemented it, CSS 2.1 is behind time right from the beginning. The simple truth is that the CSS WG acts too slow to release fully meaningful standard recommendations. Therefore it is probably a bad idea to take it as the strict norm.

Comment 139

[Kevin Newman]

These last few comments are kind of moot, since at the end of the day, these specs need someone to implement them, and if no one shows up that has the skill and motivation, it just will not happen.

That said, there are other specs to consult, and last time I check FireFox was not limited to only CSS 2.1 (or 2, or 3.0). Some of these features are defined in SVG spec as well (text-shadow, @font-face, rgba, and a whole lot of other very useful features).

Honestly, there are two types of arguments against implementation of features that I see constantly on Bugzilla. One is that it isn't in the spec. Well neither is -moz-border-radius or -moz-binding but they're still in the code base, aren't they?

The other reason is that such a feature would be used to make pages "the wrong way". That excuse doesn't really apply to this bug as much as some others (like @font-face), but that excuse is far worse, because it assumes that the Mozilla code base is a kind of standards policing tool, rather than a development platform.

All of these features would only make Mozilla a more attractive platform for developers, but for some reason, spec purists and markup purists can't get out of the way and they keep coming up with horrible excuses for why things should not happen.

If the real reason for excluding a particular feature is a lack of hands available to create said feature, then fine. That's a perfectly acceptable (and understandable) reason for leaving it out. Everything else is just nonsense.

Comment 140

[Patrick Dark]

Attachment: testcase: text-shadow, filter: dropshadow(), and filter: shadow() comparison

(In reply to comment #136)
> Because Firefox and IE, the two most popular browsers at the moment, do not
> support it at all.

It’s true that Internet Explorer doesn’t support the “text-shadow” property. However, it does support two alternatives in the form of “filter: dropshadow()” and “filter: shadow()”. Admittedly, you don’t have the blurring effect and have some anti‐aliasing issues, but you can still produce a basic shadow. One might call this an IE‐parity bug.

I’m adding an attachment that demonstrates the Internet Explorer filters.

(In reply to comment #137)
>  However, comments in the specification clearly indicate that the meaning of
> the text-shadow property remains in flux.  Further, CSS 3 might not contain a
> complete text-shadow specification.  The working draft of "CSS 3 Text" states:
> "The following features are at risk and may be cut from the spec during its CR
> period: multiple text shadows…

I don’t see how this prevents Mozilla from implementing something like “moz-text-shadow” so that people can use it in an experimental form in the meantime.

Comment 141

[Jeff Walden [:Waldo]]

There are a few reasons this hasn't been implemented yet:

1. Nobody with the right knowledge has done it.  Implementing
   new features, correctly, safely, and rigorously, is hard.
2. The people with the right knowledge to do it have been busy
   doing other things:
   - inline-table and inline-block support
   - CSS counters
   - soft hyphens
   - alpha-transparency support for CSS colors
   - significantly better text display in other languages and
     directionalities than English
   - better kerning and ligature support (which, it should be
     noted, some browsers flat-out don't do)
   - better automatic font selection for non-English languages
   - better support for CSS borders, including rounded borders
   - reducing differences in page layout across operating
     systems by switching to a single rendering backend
3. Not everyone implementing new features works on layout, CSS,
   or graphical backend support.
4. In the grand scheme of things, fuzzy text is less important
   than control over many other aspects of page display (cf.
   those mentioned above).
5. IE's not supporting it plays a factor in prioritization of
   feature implementation (but is not itself a reason to not
   implement it).

In the grand scheme of things, I think most developers would pick inline-block support (to name just one new feature) over text-shadow, and I think developer focus has remained off text-shadow for that reason.  Do also keep in mind real-world compatibility, which is also a significant feature requiring constant work which can take away time from implementing new features like text-shadow.


(In reply to comment #140)
> I don’t see how this prevents Mozilla from implementing something like
> “moz-text-shadow” so that people can use it in an experimental form in the
> meantime.

First, if we add support for a feature, it becomes almost impossible to remove it.  If this were added and people came to rely upon its possibly-buggy behavior, it makes it that much harder to fix the implementation because you'll break sites.  Second, and probably more importantly, CSS and page layout are implemented in C++ by Mozilla.  Mistakes here can very easily lead to memory corruption bugs, which can easily be security vulnerabilities.  Do not underestimate the difficulty involved in safely implementing functionality in unsafe languages.


Commenters here, particularly those who have made recent comments, really should read <https://bugzilla.mozilla.org/page.cgi?id=etiquette.html>, especially the first two items in the first section.

Comment 142

[YUKI "Piro" Hiroshi]

I've implemented text-shadow as an extension.

https://addons.mozilla.org/firefox/addon/5410

But it is "fake" implementation. I still hope that text-shadow is implemented natively, and Gecko keeps being one of the top group of web standard support.

Comment 143

[Mardeg]

(In reply to comment #142)
Does this need a whole extension to implement a workaround?
Would it be possible to implement a gm userscript workaround utilising the code found at http://verens.com/archives/2005/02/28/text-shadow/ (or improving on it)?

Comment 144

[YUKI "Piro" Hiroshi]

(In reply to comment #143)
I didn't know the hack http://verens.com/archives/2005/02/28/text-shadow/ before I posted the previous comment #142. The core algorithm of drawing text-shadow seems just same. If you just want to "see" shadowed texts, GM scripts based on the hack will be useful.

FYI, my extension includes some additional hacks solving following problems of the hack above:

* It doesn't support some CSS3 selectors which are available in Gecko, e.g. ":not()". The extension version supports most features of CSS3 selectors.
* When you select and copy shadowed texts, you'll get useless texts. The extension version hides useless texts from your eyes. This is implemented by XBL, like "marquee".

Of course it seems possible to include those additional hacks to the GM-script version, but I think it will be less maintainable than the simple version.

Comment 145

[YUKI "Piro" Hiroshi]

The XBL hack is available in the latest version.

Comment 146

[Wesley Johnston (:wesj)]

Just a note/question/observation. The proto theme for OSX is using an XBL hack all over the place to create a shadow/embossed look for text. Having real CSS shadows would hopefully reduce the memory and performance footprint. It would remove the need for those extra bindings entirely I think. Can this be considered a performance bug because of that?

Anyway, just a note that not having this will soon be affecting Firefox directly.

Comment 147

[Damian Shaw [Quan]]

Target Milestone really needs changing, "Future" or "mozilla2.0"?

Comment 149

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Per http://www.w3.org/mid/[e-mail] the order of the shadows in the list should now be top-to-bottom, not bottom-to-top.

Comment 150

[Jose Fandos]

Might want to update the Target Milestone. It's set to mozilla1.9 alpha1

Comment 151

[Christian :Biesinger (don't email me, ping me on IRC)]

I'd appreciate if you didn't touch the TM of my bugs, thanks

Comment 152

[Dão Gottwald [:dao]]

Per comment 149, you can't implement CSS2 and CSS3 text-shadow at the same time.

Comment 153

[Christian :Biesinger (don't email me, ping me on IRC)]

ah ok

Comment 154

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Sounds like Ventnor has a patch:
http://ventnorsblog.blogspot.com/2008/04/shadows-of-browser-shadows-of-myself.html

Comment 155

[Michael Ventnor]

I can make a WIP when I get home. Although like the blog post said I still need to implement the computed style (though if biesi doesn't mind I'll just read his patch and swipe some code from that :) )

reed, do you have access to the hg repository yet? If this is ready before Firefox 3 ships I only want it to be checked into the next version.

Speaking of which, I need to start learning Mercurial.

Comment 156

[Reed Loden [:reed]]

(In reply to comment #155)
> reed, do you have access to the hg repository yet? If this is ready before
> Firefox 3 ships I only want it to be checked into the next version.

I do, but Hg is not open for free-for-all commits yet (see bug 422754).

Comment 157

[Michael Ventnor]

Shamelessly stealing this bug from biesi.

Comment 158

[Michael Ventnor]

Attachment: New patch

This uses new Cairo-surface-based code along with software blurring (which is pretty fast) and is compliant with the very latest spec that I am aware of.

Please be gentle in your reviews, this is the first time I've touched any layout code :)

Comment 159

[Michael Ventnor]

XUL support is not in this patch, I was thinking of doing it as a followup; or even better, this motivates someone else with a lot more layout experience to port XUL to nsTextFrameThebes.

Comment 160

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'll review this after I'm done with Firefox 3, if that's OK.

Comment 161

[Michael Ventnor]

(In reply to comment #160)
> I'll review this after I'm done with Firefox 3, if that's OK.
> 

Roc, I would be surprised if you did otherwise :)

Comment 162

[Mardeg]

Attachment: test case: text-shadow on form labels should not be on values too

The first build I downloaded via Ventnor's blog (which I can't get past the captcha to comment on) has text-shadow applied to the values of form elements in this test when only the labels should have text-shadow as is the case in Opera, Safari, and the text-shadow extension mentioned in above comments.

Comment 163

[Michael Ventnor]

Does Opera and Safari do it on purpose or is there some technical limitation involved? I don't think I can differentiate between regular text and editable text in the parts of the code I am changing. Either way I can't see anything in the spec that explicitly forbids this, is there?

Comment 164

[Jose Fandos]

What happens is you close the label tag after the label text, rather than after the values? Does it still shadow everything? Doing it this way doesn't go against the spec, AFAIK.

Comment 165

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The CSS3 draft says that text-shadow does not inherit. In the patch, it looks like it does. Looks like text-shadow should be in nsStyleTextReset, next to mTextDecoration, instead of nsStyleText.

Comment 166

[Michael Ventnor]

Roc, how do I get an nsStyleTextReset from nsTextFrameThebes? I tried GetStyleTextReset() and GetStyleContext()->GetStyleTextReset() but neither worked. I can confirm the values are being written out to the nsStyleTextReset, but trying to get them results in initial values every time and therefore no shadows.

Comment 167

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Because it doesn't inherit, text nodes won't get the shadow style set on themselves, since you can only set style on elements, not text nodes.

See nsTextFrame::GetTextDecorations for how the text frame paints quirks-mode decorations by looking up the style context chain for the nearest style context that defines text-decorations. Ugly but necessary.

The spec is a little vague on how this should work. It just says
> This property accepts a comma-separated list of shadow effects to be applied to
> the text of the element.
...
> The shadow is applied to all of the element's text as well as any text
> decoration applied to it.

I guess we could interpret "all the element's text" as those text nodes which have the element as the immediate parent. You might want to email www-style to check. If you take that approach, then I'd look through the ancestor frames to find the first ancestor who's content is the parent element of the text node, and use the style from that frame.

Comment 168

[[e-mail]]

(In reply to comment #163)
> I don't think I can differentiate between regular text and editable
> text in the parts of the code I am changing.
Traditionally I believe this is addressed by adding styles to forms.css c.f. the text-transform styles for example. (Even though text shadow doesn't inherit I'm guessing they'll be needed anyway to stop you finding an outer ancestor with a text shadow style.)

Comment 169

[Michael Ventnor]

Roc: are text decorations inherited even though they're in Reset? I tried doing similar to them yet when I loaded up the label/form testcase the forms still inherited the shadow.

Comment 170

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Does your patch pass the mochitests in layout/style/test , after removing the backend_only: true from the entry for text-shadow in property_database.js?  If not, try getting those to pass first.

Comment 171

[Michael Ventnor]

Roc, are we looking at the same specs here?

Name:  	text-shadow
Value: 	none | [<shadow>, ] * <shadow>
Initial: 	none
Applies to: 	all elements and generated content
>>>>>>>> Inherited: 	yes <<<<<<<<<
Percentages: 	N/A
Media: 	visual
Computed value: 	a color plus three absolute <length>s

http://www.w3.org/TR/css3-text/#text-shadow

Comment 172

[Mardeg]

(In reply to comment #164)
> What happens if you close the label tag after the label text
The spec mentions both methods are valid, but accessibility guidelines at http://www.w3.org/TR/WCAG10-HTML-TECHS/#forms-labels are what I followed for the test case.

Comment 173

[Michael Ventnor]

If you replace the label's text-shadow with a text-decoration rule, the textbox also inherits that rule as well.

Comment 174

[Michael Ventnor]

I got replies on my blog, text-shadow is inherited. I'm going to keep it in nsStyleText.

Comment 175

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Did said replies point to a more authoritative source?  If so, what?

Comment 176

[Michael Ventnor]

Does David Hyatt have a Blogger account? Someone suspiciously named "David" said that Webkit did in fact treat text-shadow as inheritable "as per the spec".

dbaron, were there any mailing list posts changing the inheritance of text-shadow? The latest doc says it is inheritable so thats what I'll be going by if no recent amendments were made otherwise.

Comment 177

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://www.w3.org/TR/1998/REC-CSS2-19980512/text.html#text-shadow-props says it's not inherited while http://www.w3.org/TR/2007/WD-css3-text-20070306/#text-shadow says it is.  Since the change is listed in http://www.w3.org/TR/2007/WD-css3-text-20070306/#changes as a change from the 2003 CR, I'll presume it's intentional.  (The change isn't in CR yet, though, but that's probably OK.)

So, yeah, inherited is fine.

But you should still make sure you pass the style system mochitests with the backend_only line removed.

Comment 178

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I guess I was looking at the wrong version of the spec. Sorry about that.

Given that it inherits, you'll need to set text-shadow:none on form control contents in forms.css.

Comment 179

[Mackenzie]

Using mventnor's April 2nd build from his blog, I'm not getting any text shadows at all on Linux.

Comment 180

[Mackenzie]

Sorry, I'm wrong.  Running ./firefox from inside the directory didn't launch Minefield, just opened up another instance of FF3b5.  It does work.

Comment 181

[Michael Ventnor]

Attachment: Patch 2

Unless more bugs arise or a module owner does the time to do a drive-by review, this should be the final patch. until official review comments.

I'll request review once Fx3 ships, unless someone wants to review now.

Comment 182

[Michael Ventnor]

Attachment: Patch 3

OK, not only did I miss something before, but dbaron emailed me to say that I should request review anyway.

Comment 183

[Michael Ventnor]

Attachment: Patch 3.1

I finally figured out what the delete operator actually does and how to get it to work in my favour without nasty dub-free's. This fixes severe leakage with shadows.

Comment 184

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

A few comments so far:

could you explain the test_inherit_computation.html changes?

You need to make nsStyleText::CalcDifference actually notice when the contents of the arrays change to cause the necessary repaint/reflow.  (I'm guessing the only need for reflow is changing the overflow area, right?)

You might also consider making the array that you put in nsStyleText reference-counted.  That is, you could do something like what nsCSSCompressedDataBlock or nsAtomImpl does (but remember that your array elements are bigger than 1 char) and then just reference-count as you propagate from parent to child, rather than allocate and copy.

Comment 185

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, why the text-shadow:inherit on inputs with type="radio" or type="checkbox"?

Comment 186

[Michael Ventnor]

(In reply to comment #184)
> A few comments so far:
> 
> could you explain the test_inherit_computation.html changes?

A little hard to explain...
The test changes the CSS color attribute in an inconvenient spot. As such it confuses the test when it sets a text-shadow value that relies on reading CSS color. By changing that line we remove that confusion.

> You need to make nsStyleText::CalcDifference actually notice when the contents
> of the arrays change to cause the necessary repaint/reflow.  (I'm guessing the
> only need for reflow is changing the overflow area, right?)

I could, I did try but the code became a little messy and bloated (plus I kept thinking about edge cases I could be missing) so I just kept NONE vs REFLOW in order to keep the code simple and ensure that no change to text-shadow will be buggy.

> You might also consider making the array that you put in nsStyleText
> reference-counted.  That is, you could do something like what
> nsCSSCompressedDataBlock or nsAtomImpl does (but remember that your array
> elements are bigger than 1 char) and then just reference-count as you propagate
> from parent to child, rather than allocate and copy.

I'll try to figure out exactly what this means. :) (First-time layout programming sucks)

(In reply to comment #185)
> Also, why the text-shadow:inherit on inputs with type="radio" or
> type="checkbox"?

These aren't editable elements. Is it possible to put the label within the <input>? That's the case I wanted to consider.

Comment 187

[Michael Ventnor]

(In reply to comment #184)
> You might also consider making the array that you put in nsStyleText
> reference-counted.  That is, you could do something like what
> nsCSSCompressedDataBlock or nsAtomImpl does (but remember that your array
> elements are bigger than 1 char) and then just reference-count as you propagate
> from parent to child, rather than allocate and copy.
> 

Also, where should I be looking at for this? I looked at nsCSSDataBlock.cpp but found no reference to refcounting (or at least AFAICT)

Comment 188

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I meant to look in nsCSSDataBlock for doing the fused allocation, not the reference counting.

And I hadn't noticed those length tests were ==0 rather than != each other.  That said, I think you really should check through the array in CalcDifference; having bogus reflows for all style changes every time text shadow is used could be a problem.  I'd really prefer to at least maintain the invariant that if nothing changed, CalcDifference reports no changes.  (It's ok to report hints that are too strong some of the time, though.)

I don't know what you mean by "the label".

Comment 189

[Dão Gottwald [:dao]]

(In reply to comment #186)
> Is it possible to put the label within the <input>?

No, the label is always a sibling:

<label><input type="checkbox"> label text</label>

<input type="checkbox" id="foo"><label for="foo"> label text</label>

Comment 190

[Michael Ventnor]

(In reply to comment #189)
> (In reply to comment #186)
> > Is it possible to put the label within the <input>?
> 
> No, the label is always a sibling:
> 
> <label><input type="checkbox"> label text</label>
> 
> <input type="checkbox" id="foo"><label for="foo"> label text</label>

I didn't necessarily mean <label>:

<input type="checkbox">adjacent text</input>

Comment 191

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Children of input elements are ignored (and can't even be created in text/html without script).

Comment 192

[Michael Ventnor]

(In reply to comment #191)
> Children of input elements are ignored (and can't even be created in text/html
> without script).
> 

I guess it works in quirks mode, then. Because pasting the above code in a data: url works.

I figured out a nice clean way to almost always return the minimum CalcDifference. But dbaron, I still don't understand what nsCSSDataBlock does that would be useful to the array.

The fact that I didn't write a lot of the text-shadow parsing code doesn't help, of course :) That code is still all pretty much from biesi's old patch, and I still have a tiny bit of trouble working with it.

Comment 193

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #192)
> (In reply to comment #191)
> > Children of input elements are ignored (and can't even be created in text/html
> > without script).
> > 
> 
> I guess it works in quirks mode, then. Because pasting the above code in a
> data: url works.

I'm not sure what you mean by works -- the input doesn't have any children.  <input> creates an empty input element, and </input> is ignored since it's an error.

> I figured out a nice clean way to almost always return the minimum
> CalcDifference. But dbaron, I still don't understand what nsCSSDataBlock does
> that would be useful to the array.

It allocations a structure (in which you could store a reference count) and an array as part of a single allocation.

Comment 194

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 319170 [details] [diff] [review]
Patch 3.1


>+  if (list) {
...
>+      PRUint32 arrayLength = 0;
>+      for (nsCSSValueList *list2 = list; list2; list2 = list2->mNext)
>+        ++arrayLength;
>+
>+      if (arrayLength) {

This if (arrayLength) check is unnecessary; you're inside a null-check of list and therefore arrayLength is >= 1.  Instead of this test, you should assert that arrayLength >= 1.

(Note that if you refcount the arrays, you can also fuse the length into the structure rather than having it as a separate variable.)

>+          for (nsTextShadowItem* item = text->mShadowArray;
>+               list;
>+               list = list->mNext) {

Put the ++item inside the for-expression as well

>+nsTextShadowItem::nsTextShadowItem()
>+{
>+  // The X and Y offset must always be explicitly set and are thus not specially
>+  // initialized here.
>+  mRadius.SetCoordValue(0);
>+  mHasColor = PR_FALSE;

The radius is always explicitly set as well, it looks like.

>Index: layout/style/nsComputedDOMStyle.cpp
> nsresult
>+nsComputedDOMStyle::GetTextShadow(nsIDOMCSSValue** aValue)

I think when the value is none, you should return a primitive value 'none' rather than a value list containing 'none' as a single item (like GetQuotes, GetContent, GetCounterIncrement, GetCounterReset, GetBorderColorsFor, and GetStrokeDasharray).

>+{
>+  nsDOMCSSValueList *valueList = GetROCSSValueList(PR_TRUE);
>+  NS_ENSURE_TRUE(valueList, NS_ERROR_OUT_OF_MEMORY);
>+
>+  const nsStyleText* text = GetStyleText();
>+
>+  if (text) {
>+    if (!text->mShadowArray) {
>+      NS_ASSERTION(text->mShadowArrayLength == 0,
>+                   "Null array but nonzero length?");
>+      nsROCSSPrimitiveValue *val = GetROCSSPrimitiveValue();
>+      if (!val) {
>+        delete valueList;
>+        return NS_ERROR_OUT_OF_MEMORY;
>+      }
>+
>+      val->SetIdent(nsGkAtoms::none);
>+      if (!valueList->AppendCSSValue(val)) {
>+        delete valueList;
>+        delete val;
>+        return NS_ERROR_OUT_OF_MEMORY;
>+      }
>+      return CallQueryInterface(valueList, aValue);
>+    }
>+
>+    for (nsTextShadowItem *item = text->mShadowArray,
>+                      *item_end = text->mShadowArray + text->mShadowArrayLength;
>+         item < item_end; ++item) {
>+      nsDOMCSSValueList *itemList = GetROCSSValueList(PR_FALSE);
>+      if (!itemList || !valueList->AppendCSSValue(itemList)) {
>+        delete itemList;
>+        delete valueList;
>+        return NS_ERROR_OUT_OF_MEMORY;
>+      }
>+
>+      // Color is either the specified shadow color or the foreground color.
>+      nsROCSSPrimitiveValue *val = GetROCSSPrimitiveValue();
>+      if (!val || !itemList->AppendCSSValue(val)) {
>+        delete val;
>+        delete valueList;
>+        return NS_ERROR_OUT_OF_MEMORY;
>+      }
>+      nscolor shadowColor;
>+      if (item->mHasColor) {
>+        shadowColor = item->mColor;
>+      } else {
>+        shadowColor = GetStyleColor()->mColor;
>+      }
>+      SetToRGBAColor(val, shadowColor);
>+
>+      // Our nsStyleCoord objects are all created from lengths, thus their unit
>+      // is eStyleUnit_Coord, so we can get an nscoord directly.

They could be eStyleUnit_Chars, so you should use SetValueToCoord.  And you should fix the same (existing) bug in GetBorderSpacing.

You could probably also turn this code into a loop over an array of either:
 * const nsStyleCoord&
 * nsStyleCoord nsTextShadowItem::* (preferable, but trickier)
to handle mXOffset, mYOffset, and mRadius without code duplication.  (In the latter case your array could be static const.)


While I'm on the topic, if there's a clean way to do it (I haven't thought it through enough to know whether there is), it might be nice to use accessors to make it so that fewer places in the code have access to non-const nsTextShadowItem*.  (That said, the member of nsStyleStruct probably needs to be non-const so you can delete it -- I think.)  This might change a bit if you do the fused array thing.


Are you sure the test issue can't be fixed with a prerequisites line instead?  (See a bunch of other tests that use that to deal with currentColor issues.)

Comment 195

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Could you also add some additional invalid_values tests to property_database.js testing that "none, 2px 2px", "inherit, 2px 2px", and "2px 2px, inherit" don't parse?

Comment 196

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 319170 [details] [diff] [review]
Patch 3.1

[in nsRuleNode.cpp]
>+            SetCoord(arr->Item(2), item->mRadius, nsStyleCoord(),
>+                     SETCOORD_LENGTH, aContext, mPresContext, inherited);

I think you should check if arr->Item(2) is eCSSUnit_Null, and if sot set item->mRadius to a coord value of 0.  I think that's clearer than the 0-initialization that I commented on in nsTextShadowItem's constructor.

Comment 197

[Michael Ventnor]

I'm still looking at murky waters here so forgive my novice-ness :)

Do you mean something like:

struct nsTextShadowArray {
  PRUint32 mLength;
  PRUint32 mRefCnt;
  nsTextShadowItem* mShadowArray; (should this be a pointer?)

  void* operator new(size_t length) {
    return ::operator new(sizeof(nsTextShadowItem) * length);
  }
}

I usually work better with code than with straight English explanations.

Comment 198

[[e-mail]]

(In reply to comment #197)
>   nsTextShadowItem* mShadowArray; (should this be a pointer?)
> 
>   void* operator new(size_t length) {
>     return ::operator new(sizeof(nsTextShadowItem) * length);
>   }
nsTextShadowItem mShadowArray[1];
void* operator new(size_t aBaseSize, size_t aShadowSize) {
  return ::operator new(aBaseSize + (aShadowSize - 1) * sizeof(nsTextShadowItem)); // because aBaseSize already includes one nsTextShadowItem
}

Comment 199

[Dão Gottwald [:dao]]

(In reply to comment #190)
> (In reply to comment #189)
> > (In reply to comment #186)
> > > Is it possible to put the label within the <input>?
> > 
> > No, the label is always a sibling:
> > 
> > <label><input type="checkbox"> label text</label>
> > 
> > <input type="checkbox" id="foo"><label for="foo"> label text</label>
> 
> I didn't necessarily mean <label>:
> 
> <input type="checkbox">adjacent text</input>

I didn't mean <label> either, I meant the label text that you referred to in your question. That text node is always a sibling of the input element.

Comment 200

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

yep, what Neil said (though I'd s/aShadowSize/aShadowCount/g), plus an operator== and operator!= for use in CalcDifference, and AddRef and Release methods for use by nsRefPtr.  And an operator[] for the callers to access the array.  (And probably a Length() getter so that mLength can be protected too.)

Also, nsTextShadowItem needs an operator== so you don't compare uninitialized memory when mHasColor is false.

Comment 201

[Michael Ventnor]

OK, so that's all clear to me now.

But should I use nsRefPtr? That seems to cause a lot of problems, e.g. operator[] doesn't work at all (or I'm making a mistake somewhere).

Comment 202

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

With this approach you need to dereference before using [] anyway, with or without nsRefPtr.

Comment 203

[Michael Ventnor]

Do I need to make an operator* as well? Or is there a special way to dereference an nsRefPtr? (*foo doesn't work)

Comment 204

[Michael Ventnor]

Wouldn't it just be better to make a Get() function? It seems a lot cleaner than having to dereference the array pointer every single time I want to access an element.

Comment 205

[Michael Ventnor]

Plus that would solve a lot of cryptic compile errors that I have no idea what they mean or how to properly fix them.

Comment 206

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Sure.  You might want to call it ShadowAt(PRUint32), though.

Comment 207

[Michael Ventnor]

Attachment: Patch 4

Hopefully I got this right, Neil's code explanation before was really helpful. I use pointers a lot more now, and I implemented reference counting in the array class. We no longer need the copy function.

Comment 208

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 321236 [details] [diff] [review]
Patch 4

>+      // Use reference counting so we can re-use the same array struct in memory
>+      NS_IF_ADDREF(text->mShadowArray);

Why not use nsRefPtr?

>+nsChangeHint
>+nsTextShadowArray::ShadowArrayDiff(const nsTextShadowArray* aOther)
>+{
>+  if (!aOther)
>+    return NS_STYLE_HINT_REFLOW;
>+
>+  // Fast-path in case they're the same pointer
>+  if (this == aOther)
>+    return NS_STYLE_HINT_NONE;
>+
>+  // We have more or less shadows than the other
>+  if (mLength != aOther->Length())
>+    return NS_STYLE_HINT_REFLOW;
>+
>+  nsChangeHint maxHint = NS_STYLE_HINT_NONE;
>+
>+  for (PRUint32 i = 0; i < mLength; i++) {
>+    if (mArray[i].mXOffset != aOther->ShadowAt(i)->mXOffset ||
>+        mArray[i].mYOffset != aOther->ShadowAt(i)->mYOffset ||
>+        mArray[i].mRadius != aOther->ShadowAt(i)->mRadius)
>+      return NS_STYLE_HINT_REFLOW;
>+
>+    // Just remember it instead of returning outright. A later item in the
>+    // text-shadow array might have had its coordinates changed.
>+    if (mArray[i].mHasColor != aOther->ShadowAt(i)->mHasColor) {
>+      maxHint = NS_STYLE_HINT_VISUAL;
>+      continue;
>+    }
>+
>+    if (mArray[i].mHasColor &&
>+        mArray[i].mColor != aOther->ShadowAt(i)->mColor)
>+      maxHint = NS_STYLE_HINT_VISUAL;

You could certainly combine these conditions.  I'm not sure if it's even worth optimizing for reflow vs. visual at all -- I think it's fine to just stuff this logic into an operator== on nsTextShadowItem.

>+  // Check if text shadows were added or removed
>+  // This first check prevents crashing later if one of them is null
>+  if (mShadowArray != aOther.mShadowArray)
>+    return NS_STYLE_HINT_REFLOW;
>+
>+  return mShadowArray->ShadowArrayDiff(aOther.mShadowArray);

Um, this means you only call ShadowArrayDiff if the pointers are equal.  That's not what you want.

>+class nsTextShadowArray {
>+  public:
>+    void* operator new(size_t aBaseSize, PRUint32 aArrayLen) {
>+      // By doing this, we can allocate both this nsTextShadowArray and
>+      // the actual array above in one allocation. The amount of memory
>+      // to allocate is equal to the base size + the number of bytes of one
>+      // less than what we want to allocate (because aBaseSize includes one
>+      // item, as in the above declaration)

"By doing this, we can" -> "We"
"base size" -> "class's size"
"of one less than what we want to allocate" -> "for all but the first array item"

>+    nsTextShadowArray(PRUint32 aArrayLen) : mLength(aArrayLen), mRefCnt(1) {};

Mozilla convention is to initialize reference counts to zero; I'd rather you use nsRefPtr and initialize to 0; the confusion due to the inconsistency isn't worth the saved line (which wouldn't be saved if you switch to nsRefPtr).

>+    nsrefcnt Release() {
>+      mRefCnt--;
>+      if (!mRefCnt) {
>+        delete this;
>+        return 0;
>+      }
>+      return mRefCnt;
>+    }

Release probably shouldn't be inline.

>+  protected:

Probably better off private, barring a reason to make them protected.

>+    PRUint32 mLength;
>+    PRUint32 mRefCnt;
>+    nsTextShadowItem mArray[1];

Add a comment that mArray must be at the end of the structure.

>+  nsRefPtr<nsTextShadowArray> mShadowArray;  // [inherited] NULL in case of a zero-length

Er, you *are* using nsRefPtr.  And you're doing NS_RELEASE/NS_ADDREF all over the place?  You shouldn't need the latter.  And you should still make mRefCnt initially 0.

And you should add MOZ_COUNT_CTOR / MOZ_COUNT_DTOR to the ctor/dtor of both classes, and make sure they don't leak.

>+      // Set the offsets and blur radius
>+      const nsStyleCoord sizeCoords[3] =
>+        {item->mXOffset, item->mYOffset, item->mRadius};

Don't copy.  Either use references or (preferably) pointer-to-member, i.e.

static const nsStyleCoord nsTextShadowItem::*values[] = {
  &nsTextShadowItem::mXOffset,
  &nsTextShadowItem::mYOffset,
  &nsTextShadowItem::mRadius
};

>+      for (int i = 0; i < 3; i++) {

for (PRInt32 i = 0; i < NS_ARRAY_LENGTH(values); ++i) {

(use NS_ARRAY_LENGTH, and prefer pre-increment since it's the logically-simpler operation)

>+        val = GetROCSSPrimitiveValue();
>+        if (!val || !itemList->AppendCSSValue(val)) {
>+          delete val;
>+          delete valueList;
>+          return NS_ERROR_OUT_OF_MEMORY;
>+        }
>+        SetValueToCoord(val, sizeCoords[i]);

s/sizeCoords[i]/item->*(values[i])/

(the parentheses aren't technically necessary, but I think they make things clearer)

Comment 209

[Michael Ventnor]

So nsRefPtr keeps track of refcount and addrefs/releases automatically?

Comment 210

[Michael Ventnor]

Also, when we re-assign an nsStyleText (nsStyleText::nsStyleText(const nsStyleText& aSource)) We only do a memcpy of the data. Should that be a time when NS_ADDREF is necessary, or does nsRefPtr know about memcpy's as well?

Comment 211

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

nsRefPtr does AddRef and Release as needed (and nothing else; that's the point of it)

The nsStyleText copy-constructor should be rewritten to not do a memcpy; use member-initializers for each member instead (as many of the others do already).

Comment 212

[Michael Ventnor]

Attachment: Patch 5

>>+  // Check if text shadows were added or removed
>>+  // This first check prevents crashing later if one of them is null
>>+  if (mShadowArray != aOther.mShadowArray)
>>+    return NS_STYLE_HINT_REFLOW;
>>+
>>+  return mShadowArray->ShadowArrayDiff(aOther.mShadowArray);
>
>Um, this means you only call ShadowArrayDiff if the pointers are equal.  That's
>not what you want.

Yeah, that was leftovers from a previous (re)write of that function. There are probably more of those that I've missed :)

Comment 213

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 321354 [details] [diff] [review]
Patch 5

>+PRBool
>+nsTextShadowItem::IsItemEqual(const nsTextShadowItem aOther)

Why not just call this operator== ?

>+{

>+  if (mXOffset != aOther.mXOffset ||
>+      mYOffset != aOther.mYOffset ||
>+      mRadius != aOther.mRadius ||
>+      mHasColor != aOther.mHasColor)
>+    return PR_FALSE;
>+
>+  // We've checked mHasColor at this point and they're both equal
>+  // (this is why many != checks were used above)
>+  // Check that it is TRUE so we know we can safely read mColor.
>+  if (mHasColor &&
>+      mColor == aOther.mColor)
>+    return PR_TRUE;
>+
>+  return PR_FALSE;

This could just be a single boolean expression:

return mXOffset == aOther.mXOffset &&
       mYOffset == ...
       mRadius == ...
       mHasColor == aOther.mHasColor &&
       (!mHasColor || mColor == aOther.mColor);

>+nsrefcnt
>+nsTextShadowArray::Release()
>+{
>+  mRefCnt--;
>+  if (!mRefCnt) {

I slightly prefer mRefCnt == 0 here.

> nsStyleText::nsStyleText(const nsStyleText& aSource) 
> { 
>-  memcpy((nsStyleText*)this, &aSource, sizeof(nsStyleText));
>+  mTextAlign = aSource.mTextAlign;
>+  mTextTransform = aSource.mTextTransform;
>+  mWhiteSpace = aSource.mWhiteSpace;
>+
>+  mLetterSpacing = aSource.mLetterSpacing;
>+  mLineHeight = aSource.mLineHeight;
>+  mTextIndent = aSource.mTextIndent;
>+  mWordSpacing = aSource.mWordSpacing;
>+
>+  mShadowArray = aSource.mShadowArray;

I'd prefer using member initializers.  (I feel like the compiler is more likely to optimize them to a memcpy, although maybe that's wishful thinking.)

>+nsStyleText::~nsStyleText(void)
>+{
>+  mShadowArray = nsnull;
>+}

Unneeded.

>Index: layout/style/nsStyleStruct.h

>+  nsTextShadowItem() : mHasColor(PR_FALSE) {};

This should use MOZ_COUNT_CTOR / MOZ_COUNT_DTOR as well.

And no trailing semicolon after the {}.

>+  PRBool IsItemEqual(const nsTextShadowItem aOther);

As I said, this could just be operator==

>+  PRBool operator==(const nsTextShadowItem aOther) {
>+    return IsItemEqual(aOther);
>+  }
>+  PRBool operator!=(const nsTextShadowItem aOther) {
>+    return !IsItemEqual(aOther);

... and you can just return !(*this == aOther) here.

>+  }

All 3 (though 2 with proposed changes) of these should take references: "const nsTextShadowItem& aOther", not values.


>+class nsTextShadowArray {
>+  public:
>+    void* operator new(size_t aBaseSize, PRUint32 aArrayLen) {
>+      // We can allocate both this nsTextShadowArray and the
>+      // actual array in one allocation. The amount of memory to
>+      // allocate is equal to the class's size + the number of bytes for all
>+      // but the first array item (because aBaseSize includes one
>+      // item, see the private declarations)
>+      return ::operator new(aBaseSize +
>+                            (aArrayLen - 1) * sizeof(nsTextShadowItem));
>+    }
>+
>+    nsTextShadowArray(PRUint32 aArrayLen) : mLength(aArrayLen), mRefCnt(0) {
>+      MOZ_COUNT_CTOR(nsTextShadowArray);
>+    }
>+#ifdef NS_BUILD_REFCNT_LOGGING
>+    ~nsTextShadowArray() {
>+      MOZ_COUNT_DTOR(nsTextShadowArray);
>+    }
>+#endif

The constructor and destructor here need to call the constructor/destructors of the items in the array that aren't part of the object.  This means neither of them should be inline.  You can do this with placement new and destructor calls:

for (PRUint32 i = 1; i < mLength; ++i) {
  new (mArray + i) nsTextShadowItem;
}
...or you could use placement new[] for this, i.e.,
if (mLength > 1) {
  new (mArray + 1) nsTextShadowItem[mLength - 1];
}

for (PRUint32 i = 1; i < mLength; ++i) {
  mArray[i].~nsTextShadowItem();
}

>+    PRUint32 Length() const { return mLength; }
>+    nsTextShadowItem* ShadowAt(PRUint32 i) { return &mArray[i]; }
>+    const nsTextShadowItem* ShadowAt(PRUint32 i) const { return &mArray[i]; }

Both ShadowAt methods should probably NS_ASSERTION that i < mLength.

>+  nsRefPtr<nsTextShadowArray> mShadowArray;  // [inherited] NULL in case of a zero-length

Probably better to list this next to the other member variables.

nsComputedDOMStyle.cpp:

>+static const nsStyleCoord nsTextShadowItem::*shadowValues[] = {

This can be inside the function, right before use.

>+  &nsTextShadowItem::mXOffset,
>+  &nsTextShadowItem::mYOffset,
>+  &nsTextShadowItem::mRadius


Did you run the style system mochitests?  And you should run them with XPCOM_MEM_LEAK_LOG=file to make sure you haven't introduced new leaks...

Comment 214

[Michael Ventnor]

Attachment: Leak log?

For some reason, when I run XPCOM_MEM_LEAK_LOG, the only thing that shows up is nsTextShadowItem, and the amount of bytes it leaks is worthy to be submitted to TheDailyWTF. dbaron, any ideas what might be happening?

Comment 215

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Try putting a printf in the constructor and the destructor -- figure out why you're calling the destructor more than the constructor.  Or something like that.

(Are your MOZ_COUNT_CTOR and MOZ_COUNT_DTOR calls balanced?  And you didn't cross the C and the D?)

Comment 216

[Michael Ventnor]

Attachment: Patch 6

I figured out the problem. I dereferenced a pointer and in that particular case it caused the destructor to be called but not the constructor. I fixed this by just using the pointer directly.

(In reply to comment #213)
> The constructor and destructor here need to call the constructor/destructors of
> the items in the array that aren't part of the object.  This means neither of
> them should be inline.  You can do this with placement new and destructor
> calls:
> 
> for (PRUint32 i = 1; i < mLength; ++i) {
>   new (mArray + i) nsTextShadowItem;
> }
> ...or you could use placement new[] for this, i.e.,
> if (mLength > 1) {
>   new (mArray + 1) nsTextShadowItem[mLength - 1];
> }
> 
> for (PRUint32 i = 1; i < mLength; ++i) {
>   mArray[i].~nsTextShadowItem();
> }

That makes things worse; the constructor is called multiple times on the same pointer (I verified this by putting addresses in my printf's).

> 
> Did you run the style system mochitests?  And you should run them with
> XPCOM_MEM_LEAK_LOG=file to make sure you haven't introduced new leaks...
> 

No leaks and all the tests pass.

Comment 217

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #216)
> I figured out the problem. I dereferenced a pointer and in that particular case
> it caused the destructor to be called but not the constructor. I fixed this by
> just using the pointer directly.

Ah.  You should probably follow the advice at:
http://developer.mozilla.org/en/docs/C%2B%2B_Portability_Guide#Always_declare_a_copy_constructor_and_assignment_operator
to declare a bogus and private copy-constructor since the class doesn't copy-construct correctly (and the implicit copy-constructor doesn't call MOZ_COUNT_CTOR).

> (In reply to comment #213)
> > The constructor and destructor here need to call the constructor/destructors of
> > the items in the array that aren't part of the object.  This means neither of
> > them should be inline.  You can do this with placement new and destructor
> > calls:
...
> That makes things worse; the constructor is called multiple times on the same
> pointer (I verified this by putting addresses in my printf's).

You still need to do it, and then figure out what's wrong (or convince me that I'm wrong and it's not needed).  Otherwise you're operating on uninitialized memory, and you could crash randomly (whenever you have a shadow list with more than one shadow).  (I'm surprised you don't hit a lot of assertions.)

You should probably also add at least a simple reftest or two that equates text shadow to the equivalent with multiple pieces of text, absolutely positioned.

Note that I've been reviewing only the style system parts; I've been hoping roc will review the rest.  (If needed, I probably could, though...)

Comment 218

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I can review the rest.

Comment 219

[Michael Ventnor]

(In reply to comment #217)
> You still need to do it, and then figure out what's wrong (or convince me that
> I'm wrong and it's not needed).  Otherwise you're operating on uninitialized
> memory, and you could crash randomly (whenever you have a shadow list with more
> than one shadow).  (I'm surprised you don't hit a lot of assertions.)

I have had a grand total of 0 crashes and assertions when testing text-shadow throughout the web and on my own local testcases which do involve multiple shadows.

I do a lot of initialization in nsRuleNode.cpp, perhaps that is doing all the work that causes this to not be needed?

> You should probably also add at least a simple reftest or two that equates text
> shadow to the equivalent with multiple pieces of text, absolutely positioned.

I can do that after I've gotten this checked in. I can't really test the blurring for obvious reasons but since the W3 spec does not define blurring it doesn't really matter.

Comment 220

[Jeff Walden [:Waldo]]

You should change the assertions you've added to use NS_ABORT_IF_FALSE instead of NS_ASSERTION -- the former will go nuclear if it fails once I get that update made to the implementation (bug 429930) while the latter definitely won't do so for awhile, and we don't want assertion outbreaks -- we want squeaky wheels causing fixes or backouts (or, best yet, fixes before the problem even appears outside one developer's tree).

Your local testcases for undefined behavior like blurring can at least be inequality reftests, or if even that's impossible then just crashtests.  Also, no offense, but I find "after I've gotten this checked in" often results in tests simply being forgotten.  Get in the habit of always writing them, and then nobody ever has to worry about forgetfulness.

You don't need the NS_BUILD_REFCNT_LOGGING ifdefs.

nsContextBoxBlur::Begin isn't null-checking allocations.  nsContextBoxBlur::End doesn't null-check a malloc.  I'm not immediately familiar enough with the placement new syntax to remember exactly what versions mean you don't need to worry about null.  nsContextBoxBlur::~nsContextBoxBlur can just delete mContext since delete is null-safe (I can't image the ctor can ever be validly fired twice on an object).

Comment 221

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #219)
> (In reply to comment #217)
> > You still need to do it, and then figure out what's wrong (or convince me that
> > I'm wrong and it's not needed).  Otherwise you're operating on uninitialized
> > memory, and you could crash randomly (whenever you have a shadow list with more
> > than one shadow).  (I'm surprised you don't hit a lot of assertions.)
> 
> I have had a grand total of 0 crashes and assertions when testing text-shadow
> throughout the web and on my own local testcases which do involve multiple
> shadows.

I think there are some low-probability (if the uninitialized data in mUnit is 10 or 11) Windows-only (because VC++ is much more likely to output operations on floats as floating-point instructions) crashes lurking if you don't call the constructor, due to getting denormal floats out of the uninitialized memory and then calling floating point instructions.  I think you should still fix it.

+            item->mRadius = 0;

This should be item->mRadius.SetCoordValue(0).  Even if it happens to work now thanks to the implicit constructors.

And you do need a few reftests to land.

Comment 222

[Michael Ventnor]

(In reply to comment #221)
> (In reply to comment #219)
> > (In reply to comment #217)
> > > You still need to do it, and then figure out what's wrong (or convince me that
> > > I'm wrong and it's not needed).  Otherwise you're operating on uninitialized
> > > memory, and you could crash randomly (whenever you have a shadow list with more
> > > than one shadow).  (I'm surprised you don't hit a lot of assertions.)
> > 
> > I have had a grand total of 0 crashes and assertions when testing text-shadow
> > throughout the web and on my own local testcases which do involve multiple
> > shadows.
> 
> I think there are some low-probability (if the uninitialized data in mUnit is
> 10 or 11) Windows-only (because VC++ is much more likely to output operations
> on floats as floating-point instructions) crashes lurking if you don't call the
> constructor, due to getting denormal floats out of the uninitialized memory and
> then calling floating point instructions.  I think you should still fix it.

So even with all the initializing stuff we do for each item in nsRuleNode.cpp right after we create an array, we can still read uninitialized data? How will that mUnit = 10 or 11 cause a crash?

Comment 223

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It won't right now, but you shouldn't assume that the class will only ever be used that one way.

Comment 224

[Michael Ventnor]

Then wouldn't it be easier and cleaner to put default values for member initializers for all members in nsTextShadowItem?

Comment 225

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The problem isn't in nsTextShadowItem; it's in nsTextShadowArray.  Putting things in nsTextShadowItem's constructor doesn't do any good if that constructor never runs.

Comment 226

[Michael Ventnor]

Attachment: Patch 7

Hmm, the problem mysteriously disappeared when I tried the approach again.

Patch 7, and roc hasn't even given his comments yet :(

Comment 227

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 321455 [details] [diff] [review]
Patch 7

OK, I'm going through the whole thing (style system parts) carefully one more time...

>Index: layout/style/nsRuleNode.cpp

>+    if (text->mShadowArray)
>+      text->mShadowArray = nsnull;

You don't need the if.

>+            SetColor(arr->Item(3), 0, mPresContext, aContext, item->mColor,
>+                     inherited);

Could you comment that the second argument can be bogus since 'inherit' isn't a valid value for the color?

(And probably the likewise for the SetLength calls, by copying the existing comment:
  // OK to pass bad aParentCoord since we're not passing SETCOORD_INHERIT
that shows up in a few places.)

>+nsTextShadowItem*
>+nsTextShadowArray::ShadowAt(PRUint32 i)
>+{
>+  NS_ABORT_IF_FALSE(i < mLength, "Accessing too high an index in the text shadow array!");
>+  return &mArray[i];
>+}
>+const nsTextShadowItem*
>+nsTextShadowArray::ShadowAt(PRUint32 i) const
>+{
>+  NS_ABORT_IF_FALSE(i < mLength, "Accessing too high an index in the text shadow array!");
>+  return &mArray[i];
>+}

These should be inline.  (I was referring to just Release when I said Release shouldn't be inline.)

>+  nsStyleText(const nsStyleText& aOther) :
>+    mTextAlign(aOther.mTextAlign),
>+    mTextTransform(aOther.mTextTransform),
>+    mWhiteSpace(aOther.mWhiteSpace),
>+    mLetterSpacing(aOther.mLetterSpacing),
>+    mLineHeight(aOther.mLineHeight),
>+    mTextIndent(aOther.mTextIndent),
>+    mWordSpacing(aOther.mWordSpacing),
>+    mShadowArray(aOther.mShadowArray) { }

Could you leave this non-inline (like all the others)?

> nsresult
>+nsComputedDOMStyle::GetTextShadow(nsIDOMCSSValue** aValue)
>+{
>+  nsDOMCSSValueList *valueList = GetROCSSValueList(PR_TRUE);
>+  NS_ENSURE_TRUE(valueList, NS_ERROR_OUT_OF_MEMORY);
>+
>+  const nsStyleText* text = GetStyleText();
>+
>+  if (text) {
>+    if (!text->mShadowArray) {
>+      nsROCSSPrimitiveValue *val = GetROCSSPrimitiveValue();
>+      val->SetIdent(nsGkAtoms::none);
>+      return CallQueryInterface(val, aValue);
>+    }

You need to move the allocation of valueList below this early return; otherwise you leak valueList in the normal case.

With those changes, r+sr=dbaron on the layout/style parts.

Comment 228

[Michael Ventnor]

Attachment: Patch 8

Comment 229

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 321545 [details] [diff] [review]
Patch 8

r+sr=dbaron on the layout/style parts

Comment 230

[Michael Ventnor]

Attachment: Patch 8.5

I decided that it's really crappy for text-shadow to influence something quite unrelated and cause inconsistency (the Z-level of underline and overline) so I am therefore moving all underlines and overlines to above text. This will have no effect on web compat at this stage due to text decoration lines always taking on the same colour as the text with current standards, although underlines and overlines will now show up when text is selected (this always happened with strikethrough).

Comment 231

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #230)
> am therefore moving all underlines and overlines to above text. This will have
> no effect on web compat at this stage due to text decoration lines always
> taking on the same colour as the text with current standards, although

That's not true.  Try this in standards mode:

<span style="color:blue;text-decoration:underline">
  <span style="color:orange">text</span>
</span>

(Note that the standards-mode and quirks-mode text decoration code is currently entirely separate, but now that the spec has changed to meet us partway we can merge them -- and there's a bug on it somewhere.  I can give you a longer explanation if needed, but don't have time right now.)

Comment 232

[Michael Ventnor]

Hmm, roc, what do you think is the least cumbersome way? Moving under/overline to the top, moving it to the top only in the presence of a text-shadow (patch 8), or keeping it as is but the text's shadow will be on top of the decoration lines.

Comment 233

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #231)
> (Note that the standards-mode and quirks-mode text decoration code is currently
> entirely separate, but now that the spec has changed to meet us partway we can
> merge them -- and there's a bug on it somewhere.  I can give you a longer
> explanation if needed, but don't have time right now.)

I'm not sure what you mean, so I'd like that explanation :-).

Comment 234

[Michael Ventnor]

roc, in the short term which of the 3 options in comment 232 seems best?

Comment 235

[Michael Ventnor]

Moving the underline and overline to the top as in Patch 8.5 would make us match Opera 9.5b2. IE8 does the wrong thing (I think) by making the underline colour the same as the colour of the text frame it is currently in, but it also seems to be painted above the text.

Comment 236

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think option #3 is best. But I'd like David to explain comment #231. I'm not sure what spec change he's referring to.

http://www.w3.org/TR/CSS21/text.html#lining-striking-props says
> Some user agents have implemented text-decoration by propagating the decoration
> to the descendant elements as opposed to preserving a constant thickness and
> line position as described above. This was arguably allowed by the looser
> wording in CSS2. SVG1, CSS1-only, and CSS2-only user agents may implement the
> older model and still claim conformance to this part of CSS 2.1. (This does not
> apply to UAs developed after this specification was released.)
But that obviously does not apply to us.

http://www.w3.org/TR/CSS21/zindex.html is still normative AFAIK and means that the underline and overline always have to be behind the text.

Comment 237

[Sierk Bornemann]

One dumb question: is it predictable, when this bug will be finally fixed? I
follow this bug now since years, and I strongly hope, that it will be fixed
some day or better in a couple of weeks or months to close the gap to Safari
and Opera.
It seems, that the Safari team and Opera team got it quicker than the Firefox
team, so what is so hard to implement this particular CSS feature into Firefox
and to work **** it over a range of several years? I don't want to nag, you
guys do a good job, but I am just curious and wonder about the long time it
takes to implement this particular CSS capability.

Comment 238

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #237)
> One dumb question: is it predictable, when this bug will be finally fixed? I
> follow this bug now since years, and I strongly hope, that it will be fixed
> some day or better in a couple of weeks or months to close the gap to Safari
> and Opera.

The simple answer is that it'll be fixed when the developers say it's fixed :)

However according to the Post 1.9 planning docs this is targeted at "1.9.next"

Comment 239

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #231)
> (Note that the standards-mode and quirks-mode text decoration code is currently
> entirely separate, but now that the spec has changed to meet us partway we can
> merge them -- and there's a bug on it somewhere.  I can give you a longer
> explanation if needed, but don't have time right now.)

I decided to put the more detailed explanation in bug 403524 comment 3.

Comment 240

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: example where you can see text painting on top of its decorations

Yes, http://www.w3.org/TR/CSS21/zindex.html is quite clear; we need to (and currently do) implement what it says (in http://www.w3.org/TR/2007/CR-CSS21-20070719/zindex.html#painting-order , point 7.2.1.4.1.1), which is that text goes on top of underlines and overlines, and line-through goes on top of the text.  This example demonstrates that (at least with my default sans-serif font).


Also, I think the layout/style parts are ready for landing; there's no reason that landing them needs to wait for the rest of the patch.  (At least assuming that mozilla-central opens before the rest is ready.)  I'm not sure if you have commit access -- I can commit for you if you don't.

Comment 241

[Michael Ventnor]

> Also, I think the layout/style parts are ready for landing; there's no reason
> that landing them needs to wait for the rest of the patch.  (At least assuming
> that mozilla-central opens before the rest is ready.)  I'm not sure if you have
> commit access -- I can commit for you if you don't.
> 

I don't have commit access; go for it.

Comment 242

[Michael Ventnor]

Attachment: Patch 9

Comment 243

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I landed the text-shadow stuff, and then backed it out due to a reproducable crash in test_value_computation.html mochitest on Windows.  (I can't reproduce the crash on Linux, though; I wouldn't be surprised if it shows up in valgrind, though.)

Comment 244

[Michael Ventnor]

dbaron, do you have a stack trace for that, or any other useful information? (I haven't used Windows in ages :-) )

Comment 245

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I had a stack earlier -- it was in a bunch of destructors.  But then office.mozilla.org went down, making me think the machine crashed, I power-cycled it, and now MSVC only gives me a blank exception dialog instead of the heap corruption dialog.

Comment 246

[Michael Ventnor]

Would you have any clue what could be causing it? Might it have something to do with the way I made fused allocation?

Comment 247

[Michael Ventnor]

Can someone please try to get a stack trace on this crash? I don't have access to a Windows machine right now.

Comment 248

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: stack trace for crash

I tried again today, and it worked.  So this is the stack trace for the "Heap Corruption Detected" dialog that the MSVC debug library pops up when running test_value_computation.html.  I'm not sure how useful just this stack will be, though.

Comment 249

[Michael Ventnor]

Looking at that, I think "delete this;" in the array destructor is the culprit, funny how it works perfectly on GCC though.

Comment 250

[Michael Ventnor]

Funny thing is, though, I can't reproduce it visiting any text-shadow site or replicating any text-shadow rules used in the mochitest on my Windows machine using a tryserver build.

Would there be an alternative to "delete this"?

Comment 251

[Michael Ventnor]

Or, could you tell me exactly which rule the test is crashing on?

My Windows machine has no dev environment or Mochitest, sadly :(

Comment 252

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't see a "delete this" in ~nsTextShadowArray.  What is it you're talking about?

Comment 253

[Michael Ventnor]

It's in Release()

Comment 254

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

nsTextShadowArray::Release is correct; "delete this" is the correct thing there.  (I don't think there's any need for ++mRefCnt before the delete this (to prevent recurring into the destructor) since you don't pass |this| from the destructor.  The need for refcount stabilization anywhere is really a bug.)

Comment 255

[Michael Ventnor]

Does removing the --mRefCnt and changing the check to mRefCnt == 1 help at all?

I shouldn't need to explicitly call the destructor since the leak logs are balanced so the destructor should be called already. dbaron, any ideas?

Comment 256

[Michael Ventnor]

What's this?

http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#312

Comment 257

[Michael Ventnor]

Attachment: Could this fix it?

OK, after learning what refcnt stabilizing is all about, can you or someone test to see if this fixes anything?

Comment 258

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No, it doesn't.  (I just applied the new version of Release to my existing tree and rebuilt.)  Nor do I see any reason to think it would.

Comment 259

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think this fixes it, though:

 +    nsTextShadowArray(PRUint32 aArrayLen) :
 +      mLength(aArrayLen), mRefCnt(0)
 +    {
 +      MOZ_COUNT_CTOR(nsTextShadowArray);
-+      if (mLength > 1) {
++      for (PRUint32 i = 1; i < mLength; ++i) {
 +        // Make sure we call the constructors of each nsTextShadowItem
 +        // (the first one is called for us because we declared it under private)
-+        new (mArray + 1) nsTextShadowItem[mLength - 1];
++        new (&mArray[i]) nsTextShadowItem();
 +      }
 +    }


My guess was that placement new[] is putting a marker for the array size somewhere that it shouldn't be.  I want to retest with that exact change (I had something slightly different, plus some printfs).  If it works, I'll reland that way.

Comment 260

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Relanded, and seems to have stuck this time:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/5bd70ded7000

To be clear for the audience -- this is only the style system half of the patch.   It implements things up to getComputedStyle(), but not the rendering.

Comment 261

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+class nsContextBoxBlur {

Could use a comment explaining what this is for and how to use it. Start by moving your comment above nsContextBoxBlur::nsContextBoxBlur() here.

+  Preciseness
+   *                    is not guaranteed so pass in something that looks nice.

I'm not sure what this means.

+  delete mContext;

Just make mContext an nsRefPtr<gfxContext>, then you won't need this. mImageSurface should be an nsRefPtr too.

+  long boxStride = mImageSurface->Stride();
+  unsigned char* boxData = mImageSurface->Data();
+  memset(boxData, 0, boxStride * aRect.Height());

You shouldn't need this, the image surface should start off cleared when created.

+  BoxBlurHorizontal(boxData, tempAlphaDataBuf, aBlurRadius);
+  BoxBlurHorizontal(tempAlphaDataBuf, boxData, aBlurRadius);

Why have you changed the algorithm? There's supposed to be three boxblurs in each direction. I also think you should follow the algorithm in its odd/even selection and choices of lobe size for each boxblur.

   virtual void PaintTextDecorationLine(nsIRenderingContext& aRenderingContext,

Why don't we just get rid of all the PaintTextDecorationLine methods that take nsIRenderingContext?

+  if (decorations != NS_STYLE_TEXT_DECORATION_NONE) {

Probably makes more sense to just "if (no decorations) return;"

I think we could simplify things a bit by moving shared code into nsContextBoxBlur. Make Begin take an nsRect in appunits which is the rectangle you want to draw into (i.e. excluding the blur radius) and specify the blur radius in appunits as an extra parameter to Begin. Begin would expand the temporary surface size to account for the blur radius and put a translation into gfxContext so that drawing into it normally puts the content in the right place. Then instead of End, let's have a PaintShadow method that takes a target gfxContext and a color and does the code that draws the color with the mask.

+  aCtx->Rectangle(gfxRect(0, 0, shadowRect.Width(), shadowRect.Height()));

Do you need this? Seems like things should work the same without it.

BTW I think PaintOneShadow is neglecting to draw the soft-hyphen. I think the code in PaintText that draws the textrun and the soft-hyphen should be factored out into a method that PaintText and PaintOneShadow can call.

Comment 262

[Michael Ventnor]

(In reply to comment #261)

> +  Preciseness
> +   *                    is not guaranteed so pass in something that looks
> nice.
> 
> I'm not sure what this means.

I can't guarantee that the radius value will exactly match the number of pixels you pass it, even though I tried tuning it to look best.

> +  long boxStride = mImageSurface->Stride();
> +  unsigned char* boxData = mImageSurface->Data();
> +  memset(boxData, 0, boxStride * aRect.Height());
> 
> You shouldn't need this, the image surface should start off cleared when
> created.

It doesn't. I noticed a lot of corruption in the shadow when I got Mask() to work. Luckily I learned about how allocation doesn't clear the memory in a uni lecture at that time so that was easy to fix :)

If it should start off cleared, I don't know why it doesn't in this case. But I know for a fact it doesn't.

> +  BoxBlurHorizontal(boxData, tempAlphaDataBuf, aBlurRadius);
> +  BoxBlurHorizontal(tempAlphaDataBuf, boxData, aBlurRadius);
> 
> Why have you changed the algorithm? There's supposed to be three boxblurs in
> each direction. I also think you should follow the algorithm in its odd/even
> selection and choices of lobe size for each boxblur.

I chose it because it is faster to do 2 passes than 3 and because I could not tell the difference. And I adjusted the lobe size calculation because I did my best to try to match the given blur radius in pixels (even though it isn't 100% precise, but Presto and Webkit seem to be even further off).

Comment 263

[Michael Ventnor]

And I checked this by taking a screenshot and counting pixels at 800% in GIMP :-)

Comment 264

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #262)
> (In reply to comment #261)
> > You shouldn't need this, the image surface should start off cleared when
> > created.
> 
> It doesn't. I noticed a lot of corruption in the shadow when I got Mask() to
> work. Luckily I learned about how allocation doesn't clear the memory in a uni
> lecture at that time so that was easy to fix :)

Then let's figure out why it doesn't. gfxImageSurface::gfxImageSurface calls calloc which should initialize the memory to null.

> > +  BoxBlurHorizontal(boxData, tempAlphaDataBuf, aBlurRadius);
> > +  BoxBlurHorizontal(tempAlphaDataBuf, boxData, aBlurRadius);
> > 
> > Why have you changed the algorithm? There's supposed to be three boxblurs in
> > each direction. I also think you should follow the algorithm in its odd/even
> > selection and choices of lobe size for each boxblur.
> 
> I chose it because it is faster to do 2 passes than 3 and because I could not
> tell the difference. And I adjusted the lobe size calculation because I did my
> best to try to match the given blur radius in pixels (even though it isn't
> 100% precise, but Presto and Webkit seem to be even further off).

Does it look bad if you use the standard lobe sizes? If not, I think we should just use them. Same for 2 vs 3 passes. If you're worried about speed, you can do a followup patch where you borrow some of the optimizations I've done to the SVG Gaussian blur code (in my SVG branch).

Comment 265

[Robert O'Callahan (:roc) (email my personal email if necessary)]

But I don't think you should be worried about the performance impact of 2 vs 3 box-blurs.

Comment 266

[Michael Ventnor]

(In reply to comment #264)
> (In reply to comment #262)
> > (In reply to comment #261)
> > > You shouldn't need this, the image surface should start off cleared when
> > > created.
> > 
> > It doesn't. I noticed a lot of corruption in the shadow when I got Mask() to
> > work. Luckily I learned about how allocation doesn't clear the memory in a uni
> > lecture at that time so that was easy to fix :)
> 
> Then let's figure out why it doesn't. gfxImageSurface::gfxImageSurface calls
> calloc which should initialize the memory to null.

Not anymore.
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/src/gfxImageSurface.cpp#52

Comment 267

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oops, I was looking at my branch, I must have fixed it locally! Sorry.

Comment 268

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sorry, filed bug 437563 to fix that.

Comment 269

[Michael Ventnor]

Attachment: Rendering patch

This removed the memset to 0, so it will depend on your s/malloc/calloc patch in that other bug. It has review so could you just land it now or does something else need to be done?

Comment 270

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'll land that patch soon.

Comment 271

[Michael Ventnor]

Attachment: Rendering patch 2

Oops, turns out I didn't take the soft hyphen into consideration at all when calculating the shadow surface size.

Comment 272

[Michael Ventnor]

Attachment: Rendering patch 2.1

Slight cleanup and fix a rounding error (turns out we need Round() in this approach since RoundOut() can cause the placement of shadows to be off by 1 pixel in some places or zoom levels).

Comment 273

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  mContext->NewPath();

Not needed after you've just created the context.

+    unsigned char* tempAlphaDataBuf = (unsigned char*) malloc(boxStride * mRect.Height());

Use nsTArray and SetLength(). (use the Elements() method to get the buffer pointer). Also, you'll be able to save a tiny bit of code by using GetDataSize when bug 437567 lands.

+  gfxContext* Begin(gfxRect aRect, nscoord aBlurRadius, PRInt32 aAppUnitsPerDevPixel);

This should probably be an nsRect not a gfxRect, since it's in appunits.

> (turns out we need Round() in this
> approach since RoundOut() can cause the placement of shadows to be off by 1
> pixel in some places or zoom levels)

It sounds like we don't understand the rounding behaviour here well enough.

I think it will be pretty important to make pixel rounding work the same in the temporary surface as in the destination; you're breaking that by making a non-pixel aligned point in the destination (the top-left of aRect) be pixel-aligned in the temporary surface.

So, I would round out aRect to pixel boundaries to compute mRect, the bounds for the temporary surface, then I would set the device offset of the temporary surface to be -mRect.TopLeft(). Read up on device offsets in cairo and Thebes. Then you do not need
+  mContext->Translate(gfxPoint(mBlurRadius, mBlurRadius));
... callers will just draw to the temporary context using the same coordinates as the destination context, and the right thing will happen. When you use the temporary surface as a source, the device offsets will kick in again so you won't have to do this:
+  aDestinationCtx->Translate(gfxPoint(mRect.X(), mRect.Y()));

+  mImageSurface = nsnull;
+  mContext = nsnull;

You don't need these.

Comment 274

[Michael Ventnor]

(In reply to comment #273)
> +    unsigned char* tempAlphaDataBuf = (unsigned char*) malloc(boxStride *
> mRect.Height());
> 
> Use nsTArray and SetLength(). (use the Elements() method to get the buffer
> pointer). Also, you'll be able to save a tiny bit of code by using GetDataSize
> when bug 437567 lands.

Whats the advantage of that over malloc()? It sounds like making the code slower for no reason.

> +  gfxContext* Begin(gfxRect aRect, nscoord aBlurRadius, PRInt32
> aAppUnitsPerDevPixel);
> 
> This should probably be an nsRect not a gfxRect, since it's in appunits.

The majority of callers use a gfxRect, it might be hard unless a gfxRect can just be casted to an nsRect.

Comment 275

[Michael Ventnor]

(In reply to comment #273)
> > (turns out we need Round() in this
> > approach since RoundOut() can cause the placement of shadows to be off by 1
> > pixel in some places or zoom levels)
> 
> It sounds like we don't understand the rounding behaviour here well enough.
> 
> I think it will be pretty important to make pixel rounding work the same in the
> temporary surface as in the destination; you're breaking that by making a
> non-pixel aligned point in the destination (the top-left of aRect) be
> pixel-aligned in the temporary surface.
> 
> So, I would round out aRect to pixel boundaries to compute mRect, the bounds
> for the temporary surface, then I would set the device offset of the temporary
> surface to be -mRect.TopLeft(). Read up on device offsets in cairo and Thebes.
> Then you do not need
> +  mContext->Translate(gfxPoint(mBlurRadius, mBlurRadius));

But mRect includes the blur radius. If I understand this right, then without the above call the callers will draw in the offset space reserved for blurring.

> ... callers will just draw to the temporary context using the same coordinates
> as the destination context, and the right thing will happen. When you use the
> temporary surface as a source, the device offsets will kick in again so you
> won't have to do this:
> +  aDestinationCtx->Translate(gfxPoint(mRect.X(), mRect.Y()));

Here it looks like we will need the blur radius as part of mRect.

Comment 276

[Michael Ventnor]

Attachment: Rendering patch - help wanted

This addresses review comments but introduces a mysterious bug, at the highest zoom level at this page: http://maettig.com/code/css/text-shadow.html you can see the shadows cut off when letters with heads or tails like h or g are used. Strangely enough this only happens on some shadows.

Comment 277

[Michael Ventnor]

Attachment: Rendering patch 3

I've found a solution which works well. Roc, could you whip up a testcase with a fancy OS X font like Zapfino to see if it fixes the problem there too at maximum zoom level?

This just unions the metrics rect and the frame rect to get the biggest possible height for any text (with a little magic to optimize for performance). It completely fixes the problem in my previous comment.

Comment 278

[Michael Ventnor]

(In reply to comment #277)
> I've found a solution which works well. Roc, could you whip up a testcase with
> a fancy OS X font like Zapfino to see if it fixes the problem there too at
> maximum zoom level?

Not that I know whether Zapfino even suffered from this problem in the first place...

Comment 279

[Michael Ventnor]

Attachment: Rendering patch 3.1

I'm officially very scared now. I don't know what I did differently, but after I removed the unioning code to test a testcase, it just started working.

Comment 280

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The temporary surface sizing and positioning looks great, thanks.

+  gfxContext* Begin(gfxRect aRect, nscoord aBlurRadius, PRInt32 aAppUnitsPerDevPixel);

const gfxRect&

We probably should just call this 'Init'.

+  void DoPaint(gfxContext* aDestinationCtx, gfxRGBA aColor);

const gfxRGBA&

Somewhere in the comments you should state explicitly that you must call Begin/Init first, once, then you call DoPaint, once, then you can't use this object anymore.

+  void DrawTextOntoGfxContext(gfxContext* aCtx,

Just 'DrawText'.

+                              gfxPoint aTextBaselinePt,
+                      PropertyProvider aProvider,
+                      gfxRect aDirtyRect,
+                      gfxPoint aFramePt,
+                      gfxPoint aTextBaselinePt,
+                      gfxRGBA aForegroundColor,

All const references here please. Especially PropertyProvider, that object is huge and we really don't want to copy it.

+                      gfxFloat* aNeedAdvanceWidth);

Why would PaintOneShadow ever need to return the advance width?

+                         (GetStateBits() & TEXT_HYPHEN_BREAK) ? PR_TRUE : PR_FALSE);

just use != 0 instead of the ?:

+                           &advance, (hyphenWidth) ? PR_TRUE : PR_FALSE);

Similarly, use hyphenWidth > 0.

+  if (textStyle->mShadowArray) {
+    nsRect shadowRect(boundingBox);
+
+    for (PRUint32 i = 0; i < textStyle->mShadowArray->Length(); ++i) {
+      nsRect tmpRect(shadowRect);
+      nsTextShadowItem* shadow = textStyle->mShadowArray->ShadowAt(i);
+      nscoord xOffset = shadow->mXOffset.GetCoordValue();
+      nscoord yOffset = shadow->mYOffset.GetCoordValue();
+      nscoord blurRadius = shadow->mRadius.GetCoordValue();
+
+      tmpRect.MoveBy(nsPoint(xOffset, yOffset));
+      tmpRect.Inflate(blurRadius, blurRadius);
+
+      aMetrics.mOverflowArea.UnionRect(aMetrics.mOverflowArea, tmpRect);
+    }
+  }

This code should be shared. It could probably even be moved into UnionTextDecorationOverflow.

+  PRUint32              mBlurRadius; // App units

Should be nscoord since it's appunits.

These variables have unnecessary space between the type and the name.

+  nsRect                mOrigRect;

Document this.

+  nsCOMPtr<nsIFontMetrics> fm;
+  nsLayoutUtils::GetFontMetricsForFrame(mFrame, getter_AddRefs(fm));
+  nsIThebesFontMetrics* tfm = static_cast<nsIThebesFontMetrics*>(fm.get());
+  gfxFontGroup* fontGroup = tfm->GetThebesFontGroup();
+  gfxFont* firstFont = fontGroup->GetFontAt(0);

This occurs in several places already right? Factor it out into a method in nsLayoutUtils.

+  nscoord innerWidthInAppUnits = (mOrigRect.width - bp.left - bp.right);

bp.LeftRight()

+nsRect
+nsDisplayTextShadow::GetBounds(nsDisplayListBuilder* aBuilder)
+{
+  nsRect rect = mFrame->GetOverflowRect() + aBuilder->ToReferenceFrame(mFrame);
+  rect.x -= mBlurRadius;
+  rect.y -= mBlurRadius;
+  rect.width += (2 * mBlurRadius);
+  rect.height += (2 * mBlurRadius);

This can't be right. Display items should not paint outside the overflow rect for the associated frame. The shadow bounds should be included in the overflow rect so this display item can just return the overflow rect (shifted by ToReferenceFrame). You'll probably need a new utility method for that and call it from the Reflow methods of inlines and blocks.

One big thing missing here --- tests. Should be pretty easy to write reftests for colored shadows, including decorations --- duplicate elements and use CSS positioning. The hard thing to test is blur, but you can at least use a != test to check that blur gives a different result to not having blur. So I think we want at least
-- test text with one shadow+offset
-- test text with multiple shadows+offsets of different colors
-- test quirks-mode text-decorations with one shadow/multiple shadows
  -- test multiple decorations and verify that the z-order is as expected
-- test standards-mode text-decorations with one shadow/multiple shadows
  -- test multiple decorations and verify that the z-order is as expected
-- test that text with one shadow+offset+blur is different from just shadow+offset

Comment 281

[Michael Ventnor]

Attachment: Rendering patch 4

(In reply to comment #280)
> +  nsCOMPtr<nsIFontMetrics> fm;
> +  nsLayoutUtils::GetFontMetricsForFrame(mFrame, getter_AddRefs(fm));
> +  nsIThebesFontMetrics* tfm = static_cast<nsIThebesFontMetrics*>(fm.get());
> +  gfxFontGroup* fontGroup = tfm->GetThebesFontGroup();
> +  gfxFont* firstFont = fontGroup->GetFontAt(0);
> 
> This occurs in several places already right? Factor it out into a method in
> nsLayoutUtils.

No, this only occurs once elsewhere, in the text decorations code. I can't move it to nsLayoutUtils because many other clients of nsLayoutUtils complain when I include gfxFont.h, due to gfxFont.h depending on many things that those clients can't access.

Please let those be the last review comments, I'm sick of rewriting this :P I'll attach tests when I get your r+sr.

Comment 282

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+                      gfxRGBA aColor, nsLineBox* aLine, nsRect aOrigRect,
+                      PRUint32 aBlurRadius, gfxPoint aOffset)

some const& needed here

+                      gfxFloat& aAdvanceWidth);

PaintOneShadow is still returning an advance width. But no-one uses it, right? So drop this parameter.

+  nsRect                mOrigRect;   // The frame rect; mRect from within the Container Frame class

Why do you need this? You have access to the frame, it's nsDisplayItem::mFrame.

> The shadow bounds should be included in the overflow rect

You don't seem to have addressed this.

Hang on, we're nearly there :-).

Comment 283

[Michael Ventnor]

(In reply to comment #282)
> > The shadow bounds should be included in the overflow rect
> 
> You don't seem to have addressed this.

What do you mean? What should I do to address this? (I thought I did by changing the GetBounds() of the text shadow display item to return just the overflow rect like the text decoration does)

Comment 284

[Michael Ventnor]

Attachment: Rendering patch 5

Hopefully this is it.

Comment 285

[Michael Ventnor]

Attachment: Rendering patch 6

Addresses IRC comments. Hopefully THIS is it.

Comment 286

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+nsLayoutUtils::GetTextShadowRectsUnion(const nsRect& aInitialRect,

Call it aTextAndDecorationsRect and move if (textStyle->mShadowArray) inside it since all callers need that. Probably should take an nsIFrame* parameter and do the GetStyleText inside the method too.

@@ -1104,6 +1104,14 @@ nsPositionedInlineFrame::Reflow(nsPresCo

You need to add to nsInlineFrame::Reflow, NOT nsPositionedInlineFrame::Reflow. the latter calls the former anyway.

+        nsRect shadowRect = nsLayoutUtils::GetTextShadowRectsUnion(line->GetCombinedArea(),

We could do better here since GetCombinedArea may contain stuff that doesn't get shadows, but I think I'll let this slide since getting this "right" would end up pretty complicated and making the overflow area bigger than necessary in rare cases isn't a significant problem.

Comment 287

[Michael Ventnor]

Attachment: Rendering patch 7

Comment 288

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsLayoutUtils::GetTextShadowRectsUnion should do nothing if we're in quirks mode. You can check that via the prescontext on the frame.

+  // Text-shadow overflows.
+  nsRect shadowRect = nsLayoutUtils::GetTextShadowRectsUnion(aMetrics.mOverflowArea,
+                                                             this);
+  aMetrics.mOverflowArea.UnionRect(aMetrics.mOverflowArea, shadowRect);
 
This actually doesn't work for pathological reasons. The overflow area for inlines can't really be calculated here because effects like text-align:justify are applied later which change the positioning of everything.

You need to add this to nsLineLayout::RelativePositionFrames, a misnamed function which computes the final overflow areas for inlines. Add it in here:
    nsRect adjustedBounds(nsPoint(0, 0), psd->mFrame->mFrame->GetSize());
    combinedAreaResult.UnionRect(psd->mFrame->mCombinedArea, adjustedBounds);
passing adjustedBounds as the initial text+decorations rectangle. That gets executed for all inline spans (but not text frames).

Comment 289

[Michael Ventnor]

Attachment: Rendering patch whatever-we're-up-to-now

Are you sure this is all? The bug is getting bloated with attachments.

Comment 290

[Robert O'Callahan (:roc) (email my personal email if necessary)]

oops, nsLayoutUtils::GetTextShadowRectsUnion should only check quirks mode for the non-textframe callers. You should probably just move the quirks-mode check out to the callers that need it.

The rest is good.

Comment 291

[Michael Ventnor]

Attachment: Rendering patch N+1

Ok

Comment 292

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    if (psd->mFrame->mFrame->PresContext()->CompatibilityMode() != eCompatibility_NavQuirks) {

mPresContext->CompatibilityMode()

+      if (PresContext()->CompatibilityMode() != eCompatibility_NavQuirks && line->IsInline()) {

Hoist the compatibility mode check out of the line loop, assign it to a boolean

+  nsRect shadowRect = nsLayoutUtils::GetTextShadowRectsUnion(*aOverflowRect, this);
+  aOverflowRect->UnionRect(*aOverflowRect, shadowRect);

Do you need to check for quirks mode here? We don't want to do anything if we're not in quirks mode. Use aPresContext for the check.

Comment 293

[Michael Ventnor]

Attachment: Rendering patch N+2

(In reply to comment #292)
> +  nsRect shadowRect = nsLayoutUtils::GetTextShadowRectsUnion(*aOverflowRect,
> this);
> +  aOverflowRect->UnionRect(*aOverflowRect, shadowRect);
> 
> Do you need to check for quirks mode here? We don't want to do anything if
> we're not in quirks mode. Use aPresContext for the check.

We must not put a check here. I did so and in standards mode the text would not repaint (but the decoration did).

Comment 294

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That makes no sense. Please post the patch here with that change and try to debug the problem.

Comment 295

[Michael Ventnor]

What do you mean? We have to include the actual text of the shadow in the overflow rect, right? We can't be selective about that. That function called from nsTextFrame also deals with the overflow rect of the text, not just the decorations.

Comment 296

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 324433 [details] [diff] [review]
Rendering patch N+2

Of course, silly me!

Comment 297

[Michael Ventnor]

Later today I'll try to attach the tests I made (once I figure out the equivalent of cvsdo add on hg) and update the patch to use the new GetDataSize.

Comment 298

[Robert O'Callahan (:roc) (email my personal email if necessary)]

"hg add"

Comment 299

[Michael Ventnor]

Attachment: Rendering patch using GetDataSize()

Comment 300

[Michael Ventnor]

Attachment: Tests, glorious tests

Comment 301

[Michael Ventnor]

Attachment: Length check

Comment 302

[Michael Ventnor]

Attachment: Another test added

Comment 303

[Michael Ventnor]

Attachment: Test patch for real

Sorry, had a repository fail in the last patch

Comment 304

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in

Comment 305

[Michael Ventnor]

Documentation: http://developer.mozilla.org/en/docs/CSS:text-shadow

Comment 306

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I had to back this out due to the reftests failing on Mac.

The problem is pretty simple and should be easy to fix: all shadows, even shadows without a blur, are painted to the A8 buffer and then used as a mask to fill with the text color. This is bad on platforms like Mac that do subpixel antialiasing because a single byte per pixel cannot capture the per-channel-alpha that subpixel AA requires --- this is why painting a shadow isn't giving exactly the same results as painting the same text. It's also unnecessarily slow.

I think we should fix this by having nsContextBoxBlur just pass the rendering through directly to the target context when mBlurRadius is zero. That means we should pass the target context into Init (the caller should just set the color on the context before calling Init, nsContextBoxBlur doesn't need to know about it). If the blur radius is zero, just return the target context from Init as the context to draw into and do nothing in DoPaint. DoPaint doesn't need any arguments anymore.

Comment 307

[Michael Ventnor]

Attachment: Fix Mac

I'm a bit worried that I'm making the code very messy and hard to read. Oh well.

Comment 308

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 324744 [details] [diff] [review]
Fix Mac

+   *                             Make sure it is not destroyed before you call DoPaint(). To set the color
+   *                             of the resulting blurred graphic mask, you must set the color on this
+   *                             context before calling DoPaint().

This comment needs rewrapping or less indenting so it fits in 80 columns.

It probably should say that you should set the color on the destination context before you do any drawing to the context returned by Init, since the context returned by Init may be the destination context.

+  nscolor maskColor;

this should probably be called shadowColor.

I think this is pretty clean actually.

Did you narrow down David's issue with text-decoration being changed? We should sort that out before relanding.

Comment 309

[Michael Ventnor]

Attachment: Nits fixed

Comment 310

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The tests now pass except for decorations-multiple-zorder.html. It fails at one pixel because the rendering order of the test is different from the rendering order of the reference.

The test renders in this order:
  blue underline shadow
  blue underline
  blue text shadow
  blue text
  red overline shadow
  red overline
  red text shadow
  red text
  green underline shadow
  green underline
  green text shadow
  green text

The reference renders in this order:
  blue underline shadow
  green underline shadow
  blue underline
  green underline
  red overline shadow
  red overline
  blue text shadow
  red text shadow
  green text shadow
  blue text
  red text
  green text

These orders are quite different. The difference in the rendering happens because the red text shadow overlaps the green underline by one pixel on Mac, and those are drawn in different orders.

Comment 311

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fixed testcase reference

Here's the fixed testcase. All reftests pass for me with this. I'll reland tomorrow if the tree's open.

Comment 312

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Relanded.

Comment 313

[rlanday]

Is the shadow supposed to disappear when it's selected?

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a1pre) Gecko/2008061302 Minefield/3.1a1pre

Comment 314

[Jonathan Haas]

(In reply to comment #313)
> Is the shadow supposed to disappear when it's selected?

Probably. At least Opera behaves similar and I don't think there's anything wrong with it.

BTW: As far as I tested it, it doesn't work for xul labels. Should Bug #438517 fix this? Would be really nice for Themes.

Comment 315

[u294409]

It doesn't disappear in Opera.

Try here: http://www.css3.info/preview/text-shadow/ ("Multiple shadows are Hot")

Comment 316

[Jonathan Haas]

(In reply to comment #315)
> It doesn't disappear in Opera.

Neither in Firefox. It's just hidden behind the selection background.

Opera has worse repainting problems, too. For example select "shadows", minimize the window and restore it and then the flames behind this word will be missing. Of course this isn't a Mozilla problem.

I'm using Opera 9.50 build 10063 btw.

Comment 317

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We made the selection background draw on top of the shadow. I think that's fine.

Comment 318

[u294409]

yes, it is.

shadow on top of selection would look ugly and strange.

Comment 319

[Jean-Yves Perrier [:teoli]]

Text-shadow gives a very bad effect when using it in combination with a link and hovering over it.

See http://scienceblogs.com/grrlscientist/2008/06/evolving_without_gods_permissi.php for an example (hover over the title, it is a link).

Should we create a new bug for this?

Comment 320

[Felix Hahn]

I noticed another minor problem. When a text-shadow is set to a text which is aligned to the right, and the body margin is set to 0, the horizontal scroll bar appears. -> http://upload.felix-hahn.eu/shadow.htm

And I also agree that a shadow above the underline looks really ugly. The headlines of css3.info are a really good example for this.

Comment 321

[Michael Ventnor]

The first one is expected, the spec says to apply the shadow to any text-decoration as well.

The second one doesn't seem like an issue to me, if it is it isn't major. I'm guessing the scrollbar also pays attention to the overflow rects to make sure you can see all the content.

Comment 322

[Michael Ventnor]

(In reply to comment #320)
> And I also agree that a shadow above the underline looks really ugly. The
> headlines of css3.info are a really good example for this.
> 

This does suck, yes, but we need it in order to both follow the spec of the shadow's Z-level and the spec of the painting order of text and decorations. There are probably also a few technical limitations in the codebase that prevent this from being possible, at least in standards mode.

Comment 323

[philippe (part-time)]

(In reply to comment #321)
 
> The second one doesn't seem like an issue to me, if it is it isn't major. I'm
> guessing the scrollbar also pays attention to the overflow rects to make sure
> you can see all the content.
> 
WebKit shpows a much stronger horizontal scrollbar on that test case
(-> http://upload.felix-hahn.eu/shadow.htm)

As for the underline, it is not pretty, but that seems what the spec requires. WebKit has the same issue.
Opera, on the other hand, doesn't draw the shadow for the underline; that seems wrong.

Comment 324

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I checked the examples from W3C and everything looks fantastic with the following builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a1pre) Gecko/2008070902 Minefield/3.1a1pre

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a1pre) Gecko/2008070903 Minefield/3.1a1pre ID:2008070903

I think that this bug can be marked verified.


(In reply to comment #323)
> As for the underline, it is not pretty, but that seems what the spec requires.
> WebKit has the same issue.
> Opera, on the other hand, doesn't draw the shadow for the underline; that seems
> wrong.

Something that has to be addressed by a follow-up bug? 

Comment 325

[Bamm Gabriana]

Does this work for themes (XUL+CSS)?

Comment 326

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It doesn't work for XUL <label value=""> or some other XUL stuff like trees.

Comment 327

[Zibi Braniecki [:zbraniecki][:gandalf]]

roc: is that bug 438517 or do we need a follow up for XUL+text-shadow?

Comment 328

[Dão Gottwald [:dao]]

(In reply to comment #327)
> roc: is that bug 438517 or do we need a follow up for XUL+text-shadow?

that's bug 438517.