438517 - CSS text-shadow not working for XUL elements using nsTextBoxFrame

Status: RESOLVED FIXED

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

Description

[Dão Gottwald [:dao]]

From bug 10713 comment 159:
> 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 1

[Neil Deakin]

What does this mean? That text-shadow isn't implemented for any xul elements, or just not for text using nsTextBoxFrame.cpp? I suspect you mean the latter.

> port XUL to nsTextFrameThebes.

huh? <label>Blah</label> already does so.

Comment 2

[Dão Gottwald [:dao]]

I suspect that too. Michael, can you confirm?

Comment 3

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

xul:text, xul:label with a value attribute, and xul:description with a value attribute use nsTextBoxFrame.  That's what this bug is about.

xul:label without a value attribute and xul:description without a value attribute use a block wrapping a regular text frame.

Comment 4

[Michael Ventnor]

Is there any current work to remove nsTextBoxFrame and make XUL use nsTextFrameThebes? 

Comment 5

[Neil Deakin]

(In reply to comment #4)
> Is there any current work to remove nsTextBoxFrame and make XUL use
> nsTextFrameThebes? 

That doesn't sound like it would be compatible. nsTextBoxFrame doesn't lay out the same way. It would be good to share text rendering functionality though.

Comment 6

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

Is this needed? nsTextBoxFrame doesn't support other CSS properties too. E.g., word-spacing, letter-spacing and text-align: justify;

Comment 7

[sibbl]

i think yes ^^ i think that's one of the only things that could be improved in the ui

Comment 8

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

I would like to get rid of nsTextBoxFrame and use nsTextFrame(Thebes) instead. It would save code, make things more consistent, and add considerable power, like the ability to use text-shadow.

The functionality that's currently only in nsTextBoxFrame is, as far as I know:
-- Ellipsization
-- Access-key underlining
Let me know if I missed something.

Access-key underlining would be fairly easy to add to nsTextFrame.

Ellipsization could be handled by text-overflow, which is somewhat hard but we should implement anyway --- except for the feature where we can put ellipses in the centre of the text. That's rarely used but we might be able to hack it in somehow or another.

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #8)
> except for the feature where we can put ellipses in
> the centre of the text. That's rarely used but we might be able to hack it in
> somehow or another.

As far as I know it's mostly used for URLs, like in the tab labels.

Comment 10

[Robert Johnston]

Confirmed with Minefield Trunk Build 2008072211 (Win32). Setting text-shadow on something like a <menu> doesn't show any kind of shadow (As you would expect).

Adding myself to the CC list on this one, as I'm interested in getting this in.

Comment 11

[Rob Arnold [:robarnold]]

Attachment: v1.0

I'm not entirely sure that this patch handles invalidation of the paint region correctly, but it draws text shadow and doesn't seem to have broken non-shadow text rendering.

Comment 12

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

Whom did you mean to request review from?

Comment 13

[Rob Arnold [:robarnold]]

Comment on attachment 340870 [details] [diff] [review]
v1.0

I meant to ask review from roc...not sure why bugzilla didn't prompt me to correct it.

Comment 14

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

+    if(textStyle->mTextShadow) {

if (

But I think that computing an exact paint rect here is a waste of time. If there's an mTextShadow, just assume that the text needs to be drawn and go ahead. In fact I think probably you should just remove the check that's already here; display list construction should do just fine at skipping painting of anything that doesn't intersect the dirty rect.

However, you *do* need to compute the exact paint rect so you can add it to the overflow area of this frame during layout.

+    DrawText(aRenderingContext, textRect, 0);

The last param should be NS_RGBA(0,0,0,0) I guess. But then it looks like if the shadow color is specified as rgba(0,0,0,0) that will magically change to some other color. I think you need an extra param to say whether there is a color override. Or else you could make the param nscolor* aOverrideColor and let it be nsnull if there is no override.

+  // Conjure an nsIRenderingContext from a gfxContext for DrawText
+  nsCOMPtr<nsIRenderingContext> renderingContext = nsnull;
+  nsIDeviceContext* devCtx = PresContext()->DeviceContext();
+  devCtx->CreateRenderingContextInstance(*getter_AddRefs(renderingContext));
+  if(!renderingContext) return;

if (

But this should all happen before aCtx->Save(), so we don't have unbalanced Save/Restore if this fails.

Need tests.

Comment 15

[[e-mail]]

Comment on attachment 340870 [details] [diff] [review]
v1.0

>+    if(textStyle->mTextShadow) {
Nit: would be nice if you could be consistent with space between if and (

>+      // ie. it is painted first.
i.e. ?

>+      for (PRUint32 i = textStyle->mTextShadow->Length(); i > 0; --i) {
>+        PaintOneShadow(aRenderingContext.ThebesContext(),
>+                       textRect,
>+                       textStyle->mTextShadow->ShadowAt(i - 1),
>+                       GetStyleColor()->mColor);
>+      }
>+    }
>+
>+    DrawText(aRenderingContext, textRect, 0);
Why not just pass GetStyleColor()->mColor here (i.e. make the third parameter aColor instead of aOverrideColor)?

Comment 16

[[e-mail]]

Comment on attachment 340870 [details] [diff] [review]
v1.0

>+        nscoord blurRadius = shadow->mRadius;
>+    
>+        tmpRect.MoveBy(nsPoint(xOffset, yOffset));
>+        tmpRect.Inflate(blurRadius, blurRadius);
>+    
>+        paintRect.UnionRect(paintRect, tmpRect);
Nit: "Blank" lines containing spaces

Comment 17

[Rob Arnold [:robarnold]]

Attachment: v1.1

Fixes style nits. Adds overflow computations to nsTextBoxFrame::DoLayout. Tests to follow in another patch.

Comment 18

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

Comment on attachment 341186 [details] [diff] [review]
v1.1

Looks good, but you have to promise to fix the RGBA(0,0,0,0) issue in a separate patch.

Comment 19

[Rob Arnold [:robarnold]]

Attachment: reftests v1.0

These are just some of the existing text-shadow tests translated to xul labels

Comment 20

[Rob Arnold [:robarnold]]

Pushed to mozilla-central:
(patch) http://hg.mozilla.org/mozilla-central/rev/fe9ee813563d
(tests) http://hg.mozilla.org/mozilla-central/rev/3605e1c34f25

rgba(0,0,0,0) issue still unaddressed.

Comment 21

[Rob Arnold [:robarnold]]

Attachment: rgba(0,0,0,0) fixup

Doesn't seem to have broken anything.

Comment 22

[Rob Arnold [:robarnold]]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/6e39da3e3e20

Comment 23

[[e-mail]]

(In reply to comment #15)
> Why not just pass GetStyleColor()->mColor here (i.e. make the third parameter
> aColor instead of aOverrideColor)?
I see now, the text, underline and strikeout may have different colours.

Comment 24

[[e-mail]]

Comment on attachment 341186 [details] [diff] [review]
v1.1

I see a lot of assertions with this patch, but unfortunately they've scrolled off my console.

Comment 25

[Rob Arnold [:robarnold]]

I noticed the assertions too, but roc seemed to think that they were safe to ignore. Still, it is very annoying and I wonder if there's a way to remove them.

Comment 26

[[e-mail]]

Comment on attachment 341186 [details] [diff] [review]
v1.1

>+      nsPoint origin(0,0);
>+      nsRect textRect = CalcTextRect(*aBoxLayoutState.GetRenderingContext(), origin);
>+      nsRect overflowRect(nsLayoutUtils::GetTextShadowRectsUnion(textRect, this));
>+      FinishAndStoreOverflow(&overflowRect, GetSize());
This seems to be what's causing the assertion - the overflowRect doesn't necessarily include the frame's size.

Comment 27

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

That's a good point, but it wasn't the source of the assertions Rob mentioned on IRC.

Anyway, we should fix that up, I goofed in my review. We just need to union overflowRect with nsRect(nsPoint(0,0), GetSize()).

Comment 28

[[e-mail]]

Attachment: Union

I was already testing with this patch when you commented to the same effect.

Comment 29

[[e-mail]]

By the way, on a slightly related topic I notice that many of the callers to GetTextShadowRectsUnion erroneously(?) union the parameter with the result because they want to update the parameter to include the union of the shadows. (I say erroneously because maybe they want to assign the result or possibly they would prefer their parameter to be inflated to include the shadow area.)

Comment 30

[[e-mail]]

Comment on attachment 341363 [details] [diff] [review]
Union

Pushed changeset 6a0437ba9680 to mozilla-central.

Comment 31

[Boris Zbarsky [:bzbarsky]]

I filed bug 459086 on the assertions this introduces.