Closed Bug 791434 Opened 7 years ago Closed 7 years ago

Text shadow with transparency is rendered too light

Categories

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

17 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla18
Tracking Status
firefox16 --- unaffected
firefox17 + fixed
firefox18 + verified

People

(Reporter: base12, Assigned: heycam)

References

Details

(Keywords: regression, testcase)

Attachments

(3 files, 2 obsolete files)

Attached file textshadow.html
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 SeaMonkey/2.15a1
Build ID: 20120914003005

Steps to reproduce:

I have a page that uses a 20% opaque text shadow.


Actual results:

In recent trunk nightly builds I notice that the shadow is almost invisible, much lighter than it used to be, and much lighter than in Safari and Opera. Also much lighter than a corresponding box shadow.


Expected results:

The text shadow should be as dark as the corresponding box shadow, and as rendered by other browsers.

See the attached HTML document, containing examples of text shadow and box shadow of the same color but rendered differently.
Attachment #661470 - Attachment mime type: text/plain → text/html
I see this too.

Andy, can you narrow down the regression range? Which build did you first see this?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Component: General → Layout: Text
Product: SeaMonkey → Core
I downloaded Windows Firefox nightlies until I could identify the first build where the bug shows up. The build is 20120809030541, 17.0a1 (2012-08-09). Previous nightly is OK.
Andy, go to about:buildconfig and let me know what "built from" is e.g.
Built from http://hg.mozilla.org/mozilla-central/rev/4e150530c925
Here you go.

Built from http://hg.mozilla.org/mozilla-central/rev/9fff2012b66c
(In reply to Andy Boze from comment #4)
> Here you go.
> 
> Built from http://hg.mozilla.org/mozilla-central/rev/9fff2012b66c

Oops, never mind. I realized the nightly just updated itself. I'll reinstall and be back in a bit.
Here's the right info.

Built from http://hg.mozilla.org/mozilla-central/rev/3199bc043da4
Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/2dfbe4b9403b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120808025427
Bad:
http://hg.mozilla.org/mozilla-central/rev/b99a81e70b06
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120808092345
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2dfbe4b9403b&tochange=b99a81e70b06

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/e001a7b3b817
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120807221927
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/bd8ac25f41c4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120808052727
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e001a7b3b817&tochange=bd8ac25f41c4
Blocks: svgtext
Version: Trunk → 17 Branch
Attached patch patch (obsolete) — Splinter Review
It seems like when there is no blur radius, we need to paint the text with the desired colour (shadowColor), but if there is blurring, we want to paint with solid black (to the alpha surface) and then mask that on to the target context using shadowColor.  I'm not sure how my changes avoided this, or why the nsContextBoxBlur behaves like that.
Attachment #662444 - Flags: review?(roc)
Comment on attachment 662444 [details] [diff] [review]
patch

Review of attachment 662444 [details] [diff] [review]:
-----------------------------------------------------------------

The way this is supposed to work for clients of nsContextBoxBlur is that we draw into shadowContext using its current color --- which will be the text color if we're not blurring, or automatically solid black if we are blurring. But with the new code we're always setting shadowContext's current color to the text color. Can we not do that? Ideally the text drawing methods wouldn't even take a text color as a parameter, but would get the current color from the context if they need it (which they mostly shouldn't).
I would need to be able to get the current colour of the context to pass in to the callbacks for SVG text rendering when we are in callback mode.  Can I actually do that?  There's no GetColor() on gfxContext.
There's GetDeviceColor(), which you could retreive and then pass back to SetDeviceColor() later - would that serve?
Maybe, but the nscolors that I pass around for the text color also use the special color values that mean "foreground color" or "40% of foreground color".  They might not survive being passed through here.
Then aren't we destroying those special values with your patch here?
No, because SVG text doesn't do text shadow (yet).
(And if/when it does, it would be done by invoking the callbacks rather than doing the shadow painting directly.)
Attached patch patch (v1.1) (obsolete) — Splinter Review
Just updated the patch to fix a failing test.
Attachment #662527 - Flags: review?(roc)
Comment on attachment 662444 [details] [diff] [review]
patch

>From 2bd49bd8da4bc6abe2bf77a79d40bb5a6209411a Mon Sep 17 00:00:00 2001
>From: Cameron McCormack <cam@mcc.id.au>
>Date: Tue, 18 Sep 2012 08:24:02 +1000
>Subject: shadow fix
>
>
>diff --git a/layout/generic/nsTextFrameThebes.cpp b/layout/generic/nsTextFrameThebes.cpp
>index 087fee0..6ddd97f 100644
>--- a/layout/generic/nsTextFrameThebes.cpp
>+++ b/layout/generic/nsTextFrameThebes.cpp
>@@ -5248,29 +5248,31 @@ nsTextFrame::PaintOneShadow(PRUint32 aOffset, PRUint32 aLength,
>     shadowColor = aShadowDetails->mColor;
>     decorationOverrideColor = &shadowColor;
>   } else {
>     shadowColor = aForegroundColor;
>     decorationOverrideColor = nullptr;
>   }
> 
>   aCtx->Save();
>-  aCtx->NewPath(); 
>+  aCtx->NewPath();
>   aCtx->SetColor(gfxRGBA(shadowColor));
> 
>   // Draw the text onto our alpha-only surface to capture the alpha values.
>   // Remember that the box blur context has a device offset on it, so we don't need to
>   // translate any coordinates to fit on the surface.
>   gfxFloat advanceWidth;
>   gfxRect dirtyRect(aDirtyRect.x, aDirtyRect.y,
>                     aDirtyRect.width, aDirtyRect.height);
>   DrawText(shadowContext, dirtyRect, aFramePt + shadowOffset,
>            aTextBaselinePt + shadowOffset, aOffset, aLength, *aProvider,
>-           nsTextPaintStyle(this), shadowColor, aClipEdges, advanceWidth,
>-           (GetStateBits() & TEXT_HYPHEN_BREAK) != 0, decorationOverrideColor);
>+           nsTextPaintStyle(this),
>+           blurRadius == 0 ? shadowColor : NS_RGB(0, 0, 0), aClipEdges,
>+           advanceWidth, (GetStateBits() & TEXT_HYPHEN_BREAK) != 0,
>+           decorationOverrideColor);
> 
>   contextBoxBlur.DoPaint();
>   aCtx->Restore();
> }
> 
> // Paints selection backgrounds and text in the correct colors. Also computes
> // aAllTypes, the union of all selection types that are applying to this text.
> bool
>diff --git a/layout/reftests/text-shadow/basic-opacity-ref.html b/layout/reftests/text-shadow/basic-opacity-ref.html
>new file mode 100644
>index 0000000..11aa6d5
>--- /dev/null
>+++ b/layout/reftests/text-shadow/basic-opacity-ref.html
>@@ -0,0 +1,2 @@
>+<div style="position: absolute; top: 33px; left: 33px; color: rgba(0, 0, 255, 0.2); z-index: 0;">Hello</div>
>+<div style="position: absolute; top: 30px; left: 30px; color: green; z-index: 1;">Hello</div>
>diff --git a/layout/reftests/text-shadow/basic-opacity.html b/layout/reftests/text-shadow/basic-opacity.html
>new file mode 100644
>index 0000000..1e2b12d
>--- /dev/null
>+++ b/layout/reftests/text-shadow/basic-opacity.html
>@@ -0,0 +1 @@
>+<div style="position: absolute; top: 30px; left: 30px; color: green; text-shadow: rgba(0, 0, 255, 0.2) 3px 3px;">Hello</div>
>diff --git a/layout/reftests/text-shadow/blur-opacity-ref.html b/layout/reftests/text-shadow/blur-opacity-ref.html
>new file mode 100644
>index 0000000..0039865
>--- /dev/null
>+++ b/layout/reftests/text-shadow/blur-opacity-ref.html
>@@ -0,0 +1 @@
>+<div style="color: transparent; text-shadow: blue 4px 4px 2px; opacity: 0.5">The shadow should be blurred</div>
>diff --git a/layout/reftests/text-shadow/blur-opacity.html b/layout/reftests/text-shadow/blur-opacity.html
>new file mode 100644
>index 0000000..6713bc9
>--- /dev/null
>+++ b/layout/reftests/text-shadow/blur-opacity.html
>@@ -0,0 +1 @@
>+<div style="color: transparent; text-shadow: rgba(0, 0, 255, 0.5) 4px 4px 2px">The shadow should be blurred</div>
>diff --git a/layout/reftests/text-shadow/reftest.list b/layout/reftests/text-shadow/reftest.list
>index a329513..8984946 100644
>--- a/layout/reftests/text-shadow/reftest.list
>+++ b/layout/reftests/text-shadow/reftest.list
>@@ -1,16 +1,18 @@
> == basic.xul basic-ref.xul
> random-if(Android) == basic-negcoord.xul basic-negcoord-ref.xul
> != blur.xul blur-notref.xul
> == color-inherit.xul color-inherit-ref.xul
> == multiple-noblur.xul multiple-noblur-ref.xul
>+== blur-opacity.html blur-opacity-ref.html
> 
> == basic.html basic-ref.html
> == basic-negcoord.html basic-negcoord-ref.html
>+== basic-opacity.html basic-opacity-ref.html
> != blur.html blur-notref.html
> == color-inherit.html color-inherit-ref.html
> == color-parserorder.html color-parserorder-ref.html
> == decorations-multiple-zorder.html decorations-multiple-zorder-ref.html
> == multiple-noblur.html multiple-noblur-ref.html
> == quirks-decor-noblur.html quirks-decor-noblur-ref.html
> == standards-decor-noblur.html standards-decor-noblur-ref.html
> == padding-decoration.html padding-decoration-ref.html
>-- 
>1.7.4.2
>
Attachment #662444 - Attachment is obsolete: true
Attachment #662444 - Flags: review?(roc)
OK, let's go with something like the current patch, but instead of testing blurRadius == 0 let's more directly test whether we're doing pass-through by checking aCtx == shadowContext.
Attached patch patch (v1.2)Splinter Review
OK.
Attachment #662527 - Attachment is obsolete: true
Attachment #662527 - Flags: review?(roc)
Attachment #662568 - Flags: review?(roc)
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [leave open]
Attached patch checked-in patchSplinter Review
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 655877 Part 26
User impact if declined: Text shadows render incorrectly.
Testing completed (on m-c, etc.): Only been on m-c for a few hours, but regression tests have been added.
Risk to taking this patch (and alternatives if risky): Low; test file in bug report verified as working now, and the new regression tests also check this.
String or UUID changes made by this patch: none
Attachment #663967 - Flags: approval-mozilla-aurora?
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Attachment #663967 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Backed out of Aurora for Windows Debug reftest failure:

https://hg.mozilla.org/releases/mozilla-aurora/rev/3440e0b0cdb3
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
And to bring the tweaked tests on trunk in line with what re-landed on Aurora:

https://hg.mozilla.org/integration/mozilla-inbound/rev/dcdda4778cef
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/dcdda4778cef
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0

The testcase works as expected.
Verified on Windows 7, Ubuntu and Mac OS with Firefox beta 6.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.